Conversation
…e-coding-challenge
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…g range validation in UsageCharge
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
sugita-seiya
left a comment
sugita-seiya
left a comment
There was a problem hiding this comment.
チャレンジ提出ありがとうございます。
拝見させていただき、読み手への配慮が随所に感じられる実装がとても素晴らしいと感じました！
- 適切にDB設計されている点
- YARDドキュメントが丁寧に記載されている点
- テストが充実しており、仕様や想定ケースがしっかりとカバーされている点
細かな点をいくつか質問させていただきましたので、お手隙の際にご確認・ご回答いただけますと幸いです。
| def self.calc_price(basic_fee:, usage_charge:, usage:) | ||
| fee = basic_fee.fee | ||
| charge = usage_charge.calc_charge(usage) | ||
| (fee + charge).floor # TODO: 小数点以下の扱い |
| basic_fee = plan.basic_fees.first | ||
| usage_charge = plan.usage_charges.first |
There was a problem hiding this comment.
Q
1つのプランに対して複数のbasic_feesやusage_chargesが存在する場合、どのレコードが選択されるかが不明確ですが、firstで最初のレコードを取得する意図はなんでしょうか？
There was a problem hiding this comment.
コメントいただきありがとうございます。
今回 Plan.by_ampere_and_usageによって取得されるひとつのplanに対するbasic_feesとusage_chargesは、必ず単一になる想定でした。
（現在は、こちらのコメント通り計算ロジックの変更に伴い、usage_chargesは複数取るように変更しました）
ここからは旧計算ロジック前提のお話になりますが、
まずbasic_feesはampareでユニーク制約があり、usage_chargesはレコード間でusage_lowerからusage_upperの範囲が重複しないようにアプリ側でバリデーションを設けております。
また、Plan.by_ampere_and_usageではbasic_feesとusage_chargesに対してjoinsとincludesとmergeを用いることで、先読みした関連レコードは条件で絞り込まれたものがキャッシュされます。
（またINNER JOINとなるため、basic_feesまたはusages_chargesがヒットしなかったplansは結果に含まれない）
そのため下記該当コードの通り、先読みした関連レコードは1件のみ取得される想定だったため、firstを用いていました。
basic_fee = plan.basic_fees.first # plan.basic_feesの時点で1レコードのみ
usage_charge = plan.usage_charges.first # plan.usage_chargesの時点で1レコードのみ私も正直なところこのfirstを用いている箇所は、分かりにくくあまり綺麗じゃないなあと思いつつ書いていました...。
調べたところ、今回のケースではfirstの代わりにsoleメソッドを使うのが良さそうだったので、そちらに変更しました 🙇
cbec282
（soleによりエラーが出るタイミングとしては、APIを実行した時にデータが不整合状態だった場合になるので、ユースケースの想定によっては、単一のレコードじゃない場合エラーではなくスキップにしたほうがいいかもしれません）
| @@ -0,0 +1,5 @@ | |||
| id,provider_id,name | |||
| 1,1,従量電灯B | |||
There was a problem hiding this comment.
NITS
細かい内容ですがCSVにidが入っておりidを明示的に指定した場合、createしたレコードのidがsequenceのcurrvalとずれてしまいます。
idの採番はpsqlに任せた方が良いと思いました。
Seed後に以下のようにnameだけ指定した場合uniq制約エラーが発生します。
Provider.create!(name: 'aa')
TRANSACTION (0.5ms) BEGIN
Provider Create (2.2ms) INSERT INTO "providers" ("name", "created_at", "updated_at") VALUES ($1, $2, $3) RETURNING "id" [["name", "aa"], ["created_at", "2025-10-06 11:22:09.290308"], ["updated_at", "2025-10-06 11:22:09.290308"]]
TRANSACTION (0.4ms) ROLLBACK
/usr/local/bundle/gems/activerecord-7.0.8/lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `exec_params': PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "providers_pkey" (ActiveRecord::RecordNotUnique)
DETAIL: Key (id)=(1) already exists.
/usr/local/bundle/gems/activerecord-7.0.8/lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `exec_params': ERROR: duplicate key value violates unique constraint "providers_pkey" (PG::UniqueViolation)
DETAIL: Key (id)=(1) already exists.There was a problem hiding this comment.
ありがとうございます。
たしかにIDはseedで固定すべきではありませんでした 🙇
今回はcodeというカラムを追加し、各外部キーもcodeを見るようにカラム名ならびに定義を変更いたしました。
この変更に伴い今回は既存migrationファイルを直接変更したため、お手数ですが以下の手順で更新をお願いいたします。
docker compose run --rm web rails db:reset
| basic_fee = plan.basic_fees.first | ||
| usage_charge = plan.usage_charges.first | ||
|
|
||
| { |
There was a problem hiding this comment.
私の手元で計算しましたが、従量料金の計算ロジックが間違っているように見えます。
120kwhは同じ結果でしたが、121kwhで大きく計算が異なっています。
段階的な料金計算ロジックが実装されていないように見受けられます。
- このアプリケーションの実行結果
40A,120kwh,東京電力エナジーパートナー,従量電灯B,3529円
40A,121kwh,東京電力エナジーパートナー,従量電灯B,4348円
- 私の手計算
40A,120kwh,東京電力エナジーパートナー,従量電灯B,3529円
40A,120kwh,東京電力エナジーパートナー,従量電灯B,3556円
添付の画像（READMEにリンクがあります）を参照すると121kWhの場合以下の計算ロジックになります。
- 基本料金 = 1144.00
- 19円88銭 x 120kWh = 3529.6
- 26円48銭 x （使用電力量-120kWh） = 26.48
=> 3556.08 となりました。
There was a problem hiding this comment.
ありがとうございます。
仰るとおり従量料金の計算ロジックを間違えておりました...。
ご共有いただいた参照先を確認したところ、仰る通りの計算方法が正として理解しました。
これに伴い以下の通りコードを修正いたしました（主な修正点）
- 従量料金の計算ロジックを正しいものに変更
- 計算の都合上、
usage_charge.usage_lowerについて、一つ前のレベルのusage_charge.usage_upperと同値となることを前提に変更- たとえば
[0, 120], [121, 300]→[0, 120], [120, 300] - 従来のデータ定義では、
usage_lower = 0とそれ以外で従量料金の計算ロジックが少し異なってしまうため、同じように扱えるようにした
- たとえば
コメントいただきました40A、121kWhのパターンの実際の計算結果のスクリーンショットを添付いたします。
There was a problem hiding this comment.
ありがとうございます。
実装および計算結果を確認させていただきました。
|
コメントありがとうございます！ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .where("(usage_lower < ? AND (usage_upper IS NULL OR usage_upper > ?))", | ||
| upper, | ||
| usage_lower) | ||
|
|
There was a problem hiding this comment.
Bug: Validation Flaw: Incorrectly Flags Adjacent Ranges
The no_overlapping_ranges validation has a logical flaw. Its current query incorrectly flags non-overlapping ranges, including valid adjacent ranges (like [0,120] and [120,300]), as overlaps. This contradicts the intended allowance for touching boundaries.
There was a problem hiding this comment.
(like [0,120] and [120,300])
おそらくこのテストケースが該当するが、期待値validとしてrspecはパスしているため問題なく動作している認識です。
|
お世話になっております。 なお、今回の修正でDB定義とseedの変更がありますので、動作確認の際には以下のコマンドでDBを更新していただけますと幸いです。 docker compose run --rm web rails db:reset（更新後のER図） |
3 participants
概要
お世話になっております。重冨です。
ご案内いただきましたチャレンジ課題「serverside_challenge_2」電気料金シミュレーション機能を実装いたしました。
課題内容
こちらに技術構成、開発環境用のセットアップ方法、シミュレーションページURLなど記載しております。
README.md
以上ご確認のほどよろしくお願いいたします。
対応内容
GET /plans/pricesGET /plans動作確認手順：初回セットアップ～シミュレーションまで
$ git clone git@github.com:shigenius/enechange-coding-challenge.git $ cd enechange-coding-challenge/serverside_challenge_2/challenge $ ./scripts/setup.shhttp://localhost:3000/plans にアクセス
必要なパラメータを入力または選択し、シミュレーション実行