Conversation
sugita-seiya
left a comment
There was a problem hiding this comment.
@monakam01
提出ありがとうございます!
内容はこれから詳しく確認させていただきますが、確認したところテストが見当たらなかったため、テストの追加をお願いできればと思います。
弊社ではテストコードを非常に大事にしており、実装と同様にテストも品質を支える重要な要素と考えていますので、ぜひご対応お願いします。
There was a problem hiding this comment.
全体的に、コントローラーにすべての責務が集中してしまっている印象を受けました。
具体的には、以下のような処理がすべて controller に記述されているため、保守性や可読性の観点から、責務ごとに適切に分離した方が良いと考えています。
- リクエスト/レスポンスの処理
- ビジネスロジック(例:料金計算など)
- パラメータのバリデーションチェック
現時点で大きく実装を変更するのは大変かと思いますので、「今後リファクタリングする場合に、どのように責務を分けて実装するべきか」について、テキストベースで構いませんので、考えを共有いただけると嬉しいです。
There was a problem hiding this comment.
コントローラの処理を最小限にし、処理を分離いたしました。db2b5d6
- 分離先
- PowerSupplyPlanSimulator(service class)
- PowerSupplyPlan(model)
- MeterRateCharge(model)
| end | ||
| end | ||
|
|
||
| def build_200_message(amp, meter_rate) |
There was a problem hiding this comment.
内容を見る限り、バリデーション処理およびエラーメッセージの生成を行っているように見受けられます。
一方でメソッド名からは、HTTP 200のレスポンスを構築するような印象を受けるため、命名と実装の責務が一致していないと感じました。
There was a problem hiding this comment.
[Q]
加えて、バリデーションはcontroller内で実施するのが適切でしょうか?意図があれば教えていただきたいです。
There was a problem hiding this comment.
@sugita-seiya
メソッド名を validate_params とし、調整いたしました。
db2b5d6#diff-362c058661917b8d89d680d4aadeea1020721ce0f8fe3d34b943450d01f2be99R34
There was a problem hiding this comment.
@mochikichi
モデルに依存しないパラメータのため、controller 内で書いておりましたが、新設したPoweSupplyPlanSimulator サービスクラス内にて行うよう変更いたしました。
db2b5d6#diff-362c058661917b8d89d680d4aadeea1020721ce0f8fe3d34b943450d01f2be99R34
| if max_rate.nil? && min_rate <= meter_rate | ||
| # 最大料金ステップの計算 | ||
| return (meter_rate - min_rate + 1) * price | ||
| end | ||
|
|
||
| if min_rate.zero? | ||
| # 最小料金ステップの計算 | ||
| if max_rate <= meter_rate | ||
| # 該当ステップの満額請求計算 | ||
| max_rate * price | ||
| else | ||
| # 該当ステップの使用量までを計算 | ||
| meter_rate * price | ||
| end | ||
| elsif (min_rate..max_rate).include?(meter_rate) | ||
| # 中間料金ステップの計算 | ||
| # 該当ステップの使用量までを計算 | ||
| (meter_rate - min_rate + 1) * price | ||
| else | ||
| # 該当ステップの満額請求計算 | ||
| (max_rate - min_rate + 1) * price | ||
| end | ||
| end |
There was a problem hiding this comment.
条件分岐が多く、ネストが深くなっているため「ガード節を活用」、「メソッド分割」してネストを浅くしたいです。
There was a problem hiding this comment.
MeterRateCharge モデル内にて分岐処理を調整しました。
db2b5d6#diff-aed437e48cb04cf29acee4e9b374fec1823f2c6695befe5eb45120f1bf3c0876R12
There was a problem hiding this comment.
追加項目 の本番デプロイ対応のためmaster.key とセットで追加しておりました。
削除してしまうと本番デプロイに失敗するため、このままとさせていただければと思います。
master.key は本番環境の環境変数に設定(git管理外)しているため、差分としてはございません。
| # root "articles#index" | ||
| namespace :api do | ||
| namespace :v1 do | ||
| post "simulate_all_plan", to: "power_supply_plan#simulate_all" |
There was a problem hiding this comment.
Q
POSTメソッドを選定した理由があれば教えていただけると嬉しいです!
There was a problem hiding this comment.
深い意図なくPOSTメソッドを使用しておりましたが、
今回の要件ではデータの更新に関連しないこと、送信データも機密情報でも大きなデータでも無いことから、
GETの方が適切と判断し変更いたしました。ce69d19
mochikichi
left a comment
There was a problem hiding this comment.
課題に取り組んでいただきありがとうございます!
いくつかコメントさせていただきましたのでご確認よろしくお願いします!
| return meter_rate * price if min_rate.zero? && max_rate.nil? | ||
|
|
||
| # 使用量がステップ請求額に満たない場合0を返す | ||
| return 0 if meter_rate < min_rate |
There was a problem hiding this comment.
[want]
calculate_if_step_applicableメソッドで、計算が不要なステップでも毎回0を返すループを実行しているのは非効率です。使用量に応じて必要なステップのみを処理するよう、早期リターンや条件分岐の最適化を検討してみていただけると良いと思います。
There was a problem hiding this comment.
| end | ||
| end | ||
|
|
||
| def build_200_message(amp, meter_rate) |
There was a problem hiding this comment.
[Q]
加えて、バリデーションはcontroller内で実施するのが適切でしょうか?意図があれば教えていただきたいです。
| end | ||
|
|
||
| def valid_amp?(amp) | ||
| [10, 15, 20, 30, 40, 50, 60].include?(amp.to_i) |
There was a problem hiding this comment.
[want]
マジックナンバーになっているので定数化できると良いかと思います。
There was a problem hiding this comment.
| [10, 15, 20, 30, 40, 50, 60].include?(amp.to_i) | ||
| end | ||
|
|
||
| def errro_result(result) |
There was a problem hiding this comment.
[nits]
errorのタイポかと思われます。
|
@sugita-seiya @mochikichi ご確認のほどよろしくお願いいたします。 |
|
修正対応いただきありがとうございました!内容確認させていただきました。 |
|
@monakam01 |
概要
serverside_challenge_2の 課題(Readme)における下記を対応料金データをDBで管理できるようにするは DB管理へ移行済み。(最終実装はコミット: #2371165(コミットメッセージ: correct seed.rb)に
checkoutすることで確認いただけます。)料金データをDBで管理できるようにする本番環境へのデプロイ2025/7/282025/7/31)実装内容(仕様)
POSTGET10 / 15 / 20 / 30 / 40 / 50 / 60のいずれか/api/v1/simulate_all_plan[{ provider_name: [電力会社名], plan_name: [プラン名], price: [電気料金] }, …]東京電力エナジーパートナー、東京ガス、Looopでんき最終的な金額から小数点第一位以下を切捨て。DB設計
エラー関連
[ { "error": { "code": 400, "message": "リクエストパラメータ 'amp' が不足しています。\nリクエストパラメータ 'meter_rate' の値が不正です。0以上の整数を指定してください。\n" } } ]メッセージパターン
リクエストパラメータ 'amp' が不足しています。\nリクエストパラメータ 'meter_rate' が不足しています。\nリクエストパラメータ 'meter_rate' の値が不正です。0以上の整数を指定してください。\nリクエストパラメータ 'amp' の値が不正です。'10 / 15 / 20 / 30 / 40 / 50 / 60' のいずれかの値を指定してください。\n特記事項
データのCSV管理にActiveHashを使用checkoutすることで確認いただけます/api/v1を使用。今後対応したい課題
テスト実装(Rspc)rake db:migrate:reset && rake db:seed)するため、料金設定以外のデータをDB管理する必要が出た場合に運用方法の再検討が必要動作確認
サーバー環境
ローカルツール
APIツール(Postman等)
POSTGETamp (契約アンペア)
10 / 15 / 20 / 30 / 40 / 50 / 60いずれかの値meter_rate (使用量)
Curlコマンド
curl -X GET "localhost:3000/api/v1/simulate_all_plan?amp=[契約アンペア]&meter_rate=[使用量]"レスポンス
[ { "provider_name": "東京電力エナジーパートナー", "plan_name": "従量電灯B", "price": 8898 }, { "provider_name": "東京電力エナジーパートナー", "plan_name": "スタンダードS", "price": 12038 }, { "provider_name": "東京ガス", "plan_name": "ずっとも電気1", "price": 8874 }, { "provider_name": "Looopでんき", "plan_name": "おうちプラン", "price": 8668 } ]動作確認メモ
テスト
備考
小数点以下の金額の処理について
1. 東京電力エナジーパートナー
出展: スタンダードプラン(関東)|電気料金プラン|東京電力エナジーパートナー株式会社
2. 東京ガス
料金について>(2) 料金計算方法に注意書きあり出展: 電気料金メニュー「基本プラン」契約・料金の仕組み
3. Looop電気
出展: 電気供給約款【低圧】株式会社 Looop