Conversation
| def index | ||
| companies = Company.all | ||
| simulation_list = [] | ||
| companies.each do |company| |
There was a problem hiding this comment.
ご指摘いただきましてありがとうございます。
ログを確認したところ、指摘いただいた通り、ループのたびにplansテーブルへのSQLが実行されていたため、下記のように修正しました。
14d1c7f
| end | ||
|
|
||
| def electricity_fee(unit_price) | ||
| unit_price * params[:amount_used].to_i |
There was a problem hiding this comment.
従量料金の計算方法ですが、案内が紛らわしかったかもしれませんが、間違っています。後出しにはなりますが、正しい情報をとってくる能力もみたいので、調査して修正していただきたいです。
There was a problem hiding this comment.
あとこの計算はelectricity_fee_instanceに任せた方がいいかもしれないです。controllerは肥大しやすいので、どこまでが責務とするのがベストか考えてみてほしいです。
| basic_charge_instance = plan.basic_charges.find_by(ampere: params[:ampere]) | ||
| next if basic_charge_instance.nil? | ||
| electricity_fee_instance = plan.electricity_fees.find_by(classification_min: ..params[:amount_used], classification_max: params[:amount_used]..) | ||
| electricity_fee_instance = plan.electricity_fees.find_by(classification_min: ..params[:amount_used], classification_max: nil) if electricity_fee_instance.nil? |
|
|
||
| def calc_result(basic_charge, electricity_fee) | ||
| basic_charge + electricity_fee | ||
| end |
There was a problem hiding this comment.
152a767
こちら、合計金額の計算ということで、責務的にはplanが持つべきと判断したため、Planモデルへと移させていただきました🙏
| electricity_fee_instance = plan.electricity_fees.find_by(classification_min: ..params[:amount_used], classification_max: nil) if electricity_fee_instance.nil? | ||
|
|
||
| simulation_list << {provider_name: company.name, plan_name: plan.name, price: calc_result(basic_charge_instance.price, electricity_fee(electricity_fee_instance.price))} | ||
| end |
There was a problem hiding this comment.
細かいですが、providerかcompanyかはどちらかに合わせたほうがいいと思います。
There was a problem hiding this comment.
ご指摘いただきましてありがとうございます。
おっしゃる通り、アプリの中でこういう、名称の違いがあると混乱しかねないため、companyに合わせさせていただきました。
096506e
| 4,30,1,858.00 | ||
| 4,40,1,1144.00 | ||
| 4,50,1,1430.00 | ||
| 4,60,1,1716.80 No newline at end of file |
There was a problem hiding this comment.
純粋な質問ですが、ここのunitってどういう用途で利用されますか？恐らく「単位」を指していると思いますが、会社によって、kwhだったり、契約数だったりまちまちのものを一緒くたにしてunitと管理してしまってもよいのでしょうか。
| 1,1,従量電灯B | ||
| 2,2,おうちプラン | ||
| 3,3,ずっとも電気1 | ||
| 4,4,従量電灯Bたっぷりプラン No newline at end of file |
There was a problem hiding this comment.
ここのidとcompany_idを分けている意図について教えてください。一緒の値であれば、まとめてしまってもいいと思います。
| t.integer :classification_min, null: false | ||
| t.integer :classification_max | ||
| t.integer :unit, null: false | ||
| t.decimal :price, null: false, scale: 2, precision: 6 |
There was a problem hiding this comment.
整数型となっていますが、例えば小数が入るplanなどがある可能性も考慮すると、decimalにしたほうがいいのではないかと思いました。
| usage_upper_limit = instance.classification_max.present? && instance.classification_max < amount_used ? instance.classification_max : amount_used | ||
| result += (usage_upper_limit - usage_lower_limit) * instance.price | ||
| end | ||
| result |
There was a problem hiding this comment.
ここのロジックが一番複雑になるので、テストをしっかり書いてほしいです。期待値がなんであるか、その通りに出力されているか、テストに書いてあることでレビュワーは実際に動かさなくても結果が見えてきやすいので業務を効率よく行うためにテストは不可欠です。
There was a problem hiding this comment.
electricity_fee_instance.calcで呼び出していると思うのですが、そのelectricity_fee_instanceって既にplanから呼び出しているものなのに、ここでまたplan_idから引っ張ってくるのって二度手間ではないですか？
find_byとwhereで似たようなことを2回してしまっていると思いました。
3 participants
電気料金比較APIの提出になります。
ローカルでの動作確認手順
docker-compose builddocker-compose run web rails db:createdocker-compose up -d下記にアクセス（parameterは適切な数値）
http://localhost:3000/api/v1/electricity_charges_simulators?ampere=10&amount_used=200
テストでの確認方法
controllerのテスト
docker-compose exec web rspec spec/controllers/electricity_charges_simulators_controller_spec.rbmodelのテスト（従量料金計算）
docker-compose exec web rspec spec/models/electricity_fee_spec.rb