【青木】電気料金シミュレーションAPIの開発 #46

jack20xx · 2024-02-20T15:48:44Z

開発内容

API設計
- ampsとwattsのクエリから値を取得し、計算処理を行う
- 計算結果はjson形式で返す
- 計算結果は電気料金の一般的な計算方法に則り、円未満を切り捨てとする
- 計算処理に使用されるデータはymlファイルで読み込む。データを新たに追加したい場合はymlファイルを追加するだけで済み、ロジックを変更する必要はない
エラー表示の条件
- URLが正しくない時
- パラメータが不足している時
- パラメータに余計な文字が含まれている時
- 入力されたアンペア数がデータに該当しない時
テスト
- RSpecを導入して実行する

今後のタスク

レビューを頂いた後に、さらなるリファクタリングを行う

RyuyaIshibashi · 2024-02-21T07:40:16Z

serverside_challenge_2/challenges/jack20xx/app/models/electricity_charges.rb

+    @charges['basic_charges'][@amps] || nil
+  end
+
+  def usage_charge


このロジックで、例えば東京電力の従量電灯B・Cで使用電力量が180kWhの時に正しく計算できますか？

https://www.tepco.co.jp/ep/private/plan2/chargelist04.html#sec03

ご指摘ありがとうございます。
計算のロジックに対する私の解釈が誤っておりましたので、修正いたしました。

また、下記のURLにおける単価と、課題のスクリーンショットで記載されている単価が異なっておりましたので、今回はスクリーンショットのほうの単価で統一いたしました。

https://www.tepco.co.jp/ep/private/plan2/chargelist04.html#sec03

fucso

controller, service, model とレイヤーを分けて実装していますが、どういった意図でレイヤー分けをしたのか教えていただきたいです。

fucso · 2024-02-22T11:58:39Z

serverside_challenge_2/challenges/jack20xx/app/controllers/api_controller.rb

+    @amps = params.require(:amps)
+    @watts = params.require(:watts)


validation の中でインスタンス変数を定義するのに違和感がありました。
例えばエンドポイントが増えてバリデーションをしない or 別のバリデーションをする場合、ここに書いてあると困ってしまいそうです。

改めて確認し、私も変更すべきだと感じましたので
set_paramsで定義するかたちに変えました。

fucso · 2024-02-22T12:00:09Z

serverside_challenge_2/challenges/jack20xx/app/controllers/api_controller.rb

+  rescue ActionController::ParameterMissing => e
+    render json: { error: "'#{e.param}'が正しくありません" }, status: :bad_request
+  end


せっかく validation service を作っているのに渡されているかどうかのチェックだけここでやってしまうと、バリデーションロジックが複数ファイルに点在してしまって良くない気がしました。

確かに良くないですね。
こちらもサービスに分けておくべきでした。
修正いたします。

fucso · 2024-02-22T12:01:05Z

serverside_challenge_2/challenges/jack20xx/app/services/validate_params_service.rb

+  private
+
+  def integer?(param)
+    /\A[-+]?\d+\z/ === param


これだとマイナスの値も許容してしまいますがパラメーターの内容的にマイナスになることはあり得るでしょうか？

いえ、マイナスになる状況はあり得ませんので、
今回の場合、符号を許容する必要はありませんでした。
修正いたします。

fucso · 2024-02-22T12:33:27Z

serverside_challenge_2/challenges/jack20xx/spec/models/electricity_charges_spec.rb

+      it 'should calculate charges and change conditions correctly' do
+        @bill = ElectricityCharges.new(50, 50, @charges_file)
+        expect(@bill.calculate).to eq 1500
+        expect(@bill.calculate).not_to eq 3500
+
+        @bill = ElectricityCharges.new(60, 50, @charges_file)
+        expect(@bill.calculate).to eq 2500
+        expect(@bill.calculate).not_to eq 4500
+
+        @bill = ElectricityCharges.new(50, 500, @charges_file)
+        expect(@bill.calculate).to eq 26000
+        expect(@bill.calculate).not_to eq 6000
+
+        @bill = ElectricityCharges.new(60, 500, @charges_file)
+        expect(@bill.calculate).to eq 27000
+        expect(@bill.calculate).not_to eq 7000
+      end


it の中に複数のチェックを書いてしまうと一つ失敗したらその先のチェックが実行されずに終わってしまうのでロジックが変わった時にテストを修正するのが大変になってしまいそうです。
また、to eq をチェックした上で not_to eq をする意味がないように感じました。

確かにそうですね。
全体的にテストの書き方を見直し、分割したいと思います。

また、念のためnot_to eqのチェックもしていましたが、
冗長であり、意味を成さないと感じたため併せて修正いたします。

jack20xx · 2024-02-25T12:26:54Z

#46 (review)

controller, service, model とレイヤーを分けて実装していますが、どういった意図でレイヤー分けをしたのか教えていただきたいです。

ご質問ありがとうございます。
私は当初、以下のような考えで分けていました

controller: ブラウザからのリクエストを処理してサービスを制御する
service: 共通の処理や切り分けられたロジックを閉じておく
model: データの処理を担当する

しかし、この考えは誤りであると感じました。
なぜならmodelはデータベースに対応する役割であるからです。
今回の課題の場合、データベースとは連携していないため、
現在modelとして分けられているElectricityChargesは
ロジックを閉じておくものとしてserviceに分けられるべきだと感じました。

keishi1129 · 2024-02-25T22:07:26Z

serverside_challenge_2/challenges/jack20xx/app/controllers/api_controller.rb

+  end
+
+  def validate_params
+    validation_service = ValidateParamsService.new(params)


railsにはvalidatorというクラスが用意されているのでそれを使うのがrails wayかもしれません。
(直しが広範囲に渡りそうなので、できたらで大丈夫です!)

この記事が参考になりそうです。
https://zenn.dev/virginia_blog/articles/1b08dc567b4059

validatorは使っていませんが、下記を記述したクラスを作ることでralisのvalidateメソッドが使えるようになります。こうすることで、rails likeにかけます。

include ActiveModel::Model include ActiveModel::Attributes

rails likeに書くと、railsに慣れている人が読みやすくなるのでおすすめです。

keishi1129 · 2024-02-25T22:14:31Z

serverside_challenge_2/challenges/jack20xx/app/services/calculate_charges_service.rb

+  end
+
+  def calculate_charges
+    electricity_files = Dir.glob(ELECTRICITY_CHARGES_PATH)


リクエストがあるたびにファイル読み込んでますかね？
リクエストごとに変わらない値なのであれば、一度読んで終わりにしたいですね。

確かに現状ですとリクエストのたびに読み込んでしまっていますね。
クラス変数を使い、１度だけ読み込むように修正してみました。

keishi1129 · 2024-02-25T22:19:51Z

serverside_challenge_2/challenges/jack20xx/app/services/get_charges_service.rb

+      prev_limit = index.zero? ? 0 : @charges['usage_charges'][conditions[index - 1]]['limit']
+      curr_limit = @charges['usage_charges'][condition]['limit'] || @watts
+      rate = @charges['usage_charges'][condition]['rate']


以下2つは繰り返し登場するので変数化した方が脳に優しいです。

@charges['usage_charges'] @charges['usage_charges'][condition]

こちらも、まとめられますね。
変数を追加しました。

keishi1129 · 2024-02-25T22:37:37Z

serverside_challenge_2/challenges/jack20xx/config/initializers/constants.rb

+INVALID_NUMBER_ERROR = '値は数字で入力してください。小数点や記号は使用できません。'.freeze
+INVALID_AMP_ERROR = "'正しいアンペア数(#{AMP_NUMBERS.join(', ')})を入力してください'".freeze


こういうエラーメッセージは、ja.ymlとかに書いた方がいいですね。
多言語化しやすいので

確かに、エラーメッセージは定数ファイルではなく言語化用のファイルに分けておくべきでした。
該当箇所をja.ymlおよびen.ymlに移動させ、呼び出せるようにしました。
また、この修正に伴い、テストも追加しました。

keishi1129 · 2024-02-25T22:39:06Z

serverside_challenge_2/challenges/jack20xx/spec/controllers/api_controller_spec.rb

+      it 'should return information correctly' do
+        allow_any_instance_of(ValidateParamsService).to receive(:validate_params).and_return([])
+        allow_any_instance_of(CalculateChargesService).to receive(:calculate_charges).and_return(1500)
+        get :show_charges, params: { amps: 10, watts: 100 }


このgetをsubjectにしてdescribeのすぐ下に持ってくると見やすくなると思いました! (好みかもしれません)

describe 'validate_params method' do subject { get :show_charges, params: params } context 'when parameters are valid' do let(:params) { { amps: 10, watts: 100 } } ... end end

コードの読みやすさに関するご意見、とても参考になります。
修正いたしました。
ありがとうございます。

RyuyaIshibashi · 2024-02-25T22:42:18Z

serverside_challenge_2/challenges/jack20xx/app/services/get_charges_service.rb

@@ -0,0 +1,44 @@
+class GetChargesService


GetChargesServiceを独立したクラスとした意図を教えてください。

ご質問ありがとうございます。

#46 (comment)

前提としまして、こちらは上記のコメントを受けてmodelからserviceに移動させたものになります。

また、移動させる際に下記のことを考慮して独立させることにしました。

・calculate_charges_serviceと統合してしまうと肥大化してしまう可能性があるため
・calculate_charges_serviceとget_charges_serviceで「料金の表示内容を取得する責務」と「料金を計算する責務」を分けたほうが良いと考えたため

ただ、改めてコードを確認し、ここまで細分化する必要はなかったのではないかとも感じています。

keishi1129 · 2024-02-25T22:43:06Z

serverside_challenge_2/challenges/jack20xx/spec/services/calculate_charges_service_spec.rb

+  describe 'calculate_charges method' do
+    context 'when parameters are correct' do
+      it 'should return correct properties' do
+        @service = CalculateChargesService.new(50, 500)


ここでインスタンス化する必要はない

#46 (comment)

このgetをsubjectにしてdescribeのすぐ下に持ってくると見やすくなると思いました! (好みかもしれません)

上記を参考にして修正し、まとめました。

RyuyaIshibashi · 2024-02-25T22:47:55Z

serverside_challenge_2/challenges/jack20xx/app/services/get_charges_service.rb

+      rate = @charges['usage_charges'][condition]['rate']
+
+      if @watts > prev_limit
+        charge += rate * ([curr_limit, @watts].min - prev_limit)


curr_limit = @charges['usage_charges'][condition]['limit'] || @watts

と[curr_limit, @watts].minの処理が離れていて、何が行われているかわかりづらいです。

確かに、変数の宣言はその変数を使用する直前で行われるべきであり、
そのほうが可読性も向上しますね。
修正いたします。

RyuyaIshibashi · 2024-02-25T22:48:26Z

serverside_challenge_2/challenges/jack20xx/app/services/get_charges_service.rb

+      curr_limit = @charges['usage_charges'][condition]['limit'] || @watts
+      rate = @charges['usage_charges'][condition]['rate']
+
+      if @watts > prev_limit


この判定が遅すぎると思います。

prev_limitを宣言した時点で、if @watts > prev_limitの判定を行えますね。
また、判定を早めることで条件のスコープ外の不要な変数を省くこともできそうです。
ご指摘、ありがとうございます。

keishi1129 · 2024-02-25T22:56:59Z

serverside_challenge_2/challenges/jack20xx/spec/services/get_charges_service_spec.rb

+
+RSpec.describe GetChargesService, type: :model do
+  before do
+    @charges_file = Rails.root.join('spec', 'test_data', 'charges.yml')


あえてシンプルな値のデータをテスト用に準備するというアイデアはいいと思いました！
例えば10Aの基本料金は1000にするとか、100kWhまでは10円にし、200kWhまでは20円にするとかですね。いちいちtest_dataの定義を見にいかないでも覚えてられるので良いです！

ただシンプルにしすぎて、小数点とかまで削ってしまうのは端数処理のテストができなくなるのでどうかなと思いました。

確かに端数処理の件がありますので、小数点を削るべきではありませんでした。
修正いたします。

keishi1129 · 2024-02-25T22:58:54Z

serverside_challenge_2/challenges/jack20xx/spec/services/validate_params_service_spec.rb

+RSpec.describe ValidateParamsService do
+  describe 'validate_params method' do
+    let(:service) { ValidateParamsService.new(params) }
+


以下のようにするとわかりやすくなりますし、記述が減りますよ

subject { service.validate_params }

#46 (comment)

このgetをsubjectにしてdescribeのすぐ下に持ってくると見やすくなると思いました! (好みかもしれません)

上記と同様に、修正しました。

keishi1129 · 2024-02-25T23:02:25Z

serverside_challenge_2/challenges/jack20xx/spec/services/validate_params_service_spec.rb

+      let(:params) { ActionController::Parameters.new({ amps: '0', watts: '50' }) }
+
+      it 'should handle unmatched_amp error correctly' do
+        expect(service.validate_params).to include('unmatched_amp')


このincludeは厳密なテストとは言えない気がします。
下記を見てもらうと、いろいろなテストに使えてしまうことがわかります。
https://www.rubydoc.info/gems/rspec-expectations/RSpec%2FMatchers:include#:~:text=Passes%20if%20actual%20includes%20expected,args%20are%20found%20in%20collection.

have_key というmatcherがあるようですよ。

URLを拝見しました。教えてくださり、ありがとうございます。
ユニークなものをテストするにはincludeでないほうが良さそうですね。
該当箇所をhave_keyに変更しました。

fucso · 2024-02-26T03:22:04Z

@jack20xx

#46 (review)

controller, service, model とレイヤーを分けて実装していますが、どういった意図でレイヤー分けをしたのか教えていただきたいです。

ご質問ありがとうございます。私は当初、以下のような考えで分けていました

controller: ブラウザからのリクエストを処理してサービスを制御する
service: 共通の処理や切り分けられたロジックを閉じておく
model: データの処理を担当する

しかし、この考えは誤りであると感じました。なぜならmodelはデータベースに対応する役割であるからです。今回の課題の場合、データベースとは連携していないため、現在modelとして分けられているElectricityChargesはロジックを閉じておくものとしてserviceに分けられるべきだと感じました。

ありがとうございます。
確かに rails の model は ORM や DB 操作の役割が多めなレイヤーなので「データの処理を担当する」という役割だと修正いただいた electricity_charges は service の方が近いかもしれないですね。

これ以降個人的な見解も含まれるし、修正する場合結構全体的な修正になってしまい他の人のレビューコメントも巻き込んでしまう可能性があり修正は不要の参考程度にですが、
チームの考え方にもよるとは思いますが、model に DB が関係ないドメインオブジェクト的なクラスを置くの自体は全然ある構成なのかなとも思います。
ただ今回用意されていた electricity_charges はプランの単価の保持と料金計算までやっており、料金計算に関する要件が増えてくると fat model になる可能性があるのかなと感じました。
今回の場合プランの単価の保持と料金計算でクラスを分けてしまって以下のようにするとメンテしやすいコードになるような気がしました。

プランの単価保持
- model に実装
- プランの名前や単価をフィールドで持つ
- A 数を引数に適切な基本料金を返したりする
料金計算
- service に実装
- model を利用してリクエストにおける単価を取得して料金を計算する

ysk91 · 2024-02-26T07:53:00Z

serverside_challenge_2/challenges/jack20xx/app/services/validate_params_service.rb

+      return errors
+    end
+
+    unless integer?(amps) && integer?(watts)


&&条件だと、片方だけがintegerではない場合にエラー出力できなくなってしまいそうです。

こちらはunlessの条件になりますので、ampsとwattsの両方がintegerとなるまではエラーになります。
よって、片方だけがintegerでない場合、両方ともintegerでない場合、どちらのケースでもエラーを出力することが可能です。

ysk91 · 2024-02-26T08:02:36Z

serverside_challenge_2/challenges/jack20xx/app/services/validate_params_service.rb

+  def validate_params
+    errors = {}
+
+    begin
+      amps = @params.require(:amps)
+      watts = @params.require(:watts)
+    rescue ActionController::ParameterMissing => e
+      errors['invalid_parameter'] = "'#{e.param}'が正しくありません"
+      return errors
+    end


クラスを定義して呼び出すよりも、コントローラ内でストロングパラメータを使用する方が一般的だと思います。

たぶん以前のコメントの対応で service に移していただいたのだと思いますが、service ないで strong parameter としての振る舞いをさせるのはあまり見かけないので、service としては単純に nil かどうかのチェックをするのがちょうどいいと思います。
active model の validater を使ったら以下のような感じになるかと思います。

validates :value, presence: true

@ysk91 @fucso
教えてくださり、ありがとうございます。

#46 (comment)
上記で頂いたご意見も参考に、service内ではnilチェックをするかたちに修正したいと思います。

ysk91 · 2024-02-26T08:16:59Z

serverside_challenge_2/challenges/jack20xx/spec/controllers/api_controller_spec.rb

@@ -0,0 +1,26 @@
+require 'rails_helper'
+
+RSpec.describe ApiController, type: :controller do


テスト設計から見直した方が良いかもしれません。
1つめのテストと2つめのテストで、同じパラメータを与えているのに結果が変わるのはなぜですか？

jack20xx · 2024-02-26T15:10:51Z

@fucso

ありがとうございます。
確かに rails の model は ORM や DB 操作の役割が多めなレイヤーなので「データの処理を担当する」という役割だと修正いただいた electricity_charges は service の方が近いかもしれないですね。

modelに関する私の知識が不足しておりました。
詳しくご説明をしてくださり、ありがとうございます。
レイヤー分けに関する学びを深めることができました。
参考にさせていただきます。

jack20xx added 6 commits February 14, 2024 20:24

Init app

6d721a9

install gem files

08fae4e

Implement api feature

3cc7aaf

Add charges data

9270c30

Add const file

9cc1f3c

Add tests and test data

663f1f6

RyuyaIshibashi reviewed Feb 21, 2024

View reviewed changes

kenji7157 requested review from ysk91, fucso and takutoki February 22, 2024 02:07

fucso reviewed Feb 22, 2024

View reviewed changes

jack20xx added 5 commits February 23, 2024 17:22

Fix how to write tests

2623017

Remove + and - signs from validation

fc3626a

Create set_params method and move ParameterMissing to validation service

a9b24bf

Fix calculation method and create new service for current charges model

0350f91

Fix yml data and variable names

d9a969d

keishi1129 reviewed Feb 25, 2024

View reviewed changes

RyuyaIshibashi reviewed Feb 25, 2024

View reviewed changes

keishi1129 reviewed Feb 25, 2024

View reviewed changes

RyuyaIshibashi reviewed Feb 25, 2024

View reviewed changes

keishi1129 reviewed Feb 25, 2024

View reviewed changes

jack20xx added 3 commits February 26, 2024 13:09

Change timing of loading yml files

54c08e4

Add variables

53ebcac

Move error messages to locale files and configure their settings

93be268

ysk91 reviewed Feb 26, 2024

View reviewed changes

jack20xx added 2 commits February 26, 2024 20:44

Fix tests related to reviewing

50576c6

Add test constants file and fix test data

f047526

jack20xx added 2 commits March 2, 2024 19:52

Fix validate_params method

b5c1d38

Fix params validations in a service

a978416

		@amps = params.require(:amps)
		@watts = params.require(:watts)

		INVALID_NUMBER_ERROR = '値は数字で入力してください。小数点や記号は使用できません。'.freeze
		INVALID_AMP_ERROR = "'正しいアンペア数(#{AMP_NUMBERS.join(', ')})を入力してください'".freeze

		@@ -0,0 +1,26 @@
		require 'rails_helper'

		RSpec.describe ApiController, type: :controller do

【青木】電気料金シミュレーションAPIの開発 #46

Are you sure you want to change the base?

【青木】電気料金シミュレーションAPIの開発 #46

Conversation

jack20xx commented Feb 20, 2024

開発内容

今後のタスク

Choose a reason for hiding this comment

jack20xx Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

fucso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack20xx Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack20xx commented Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

keishi1129 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keishi1129 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keishi1129 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack20xx Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keishi1129 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fucso commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack20xx commented Feb 26, 2024

jack20xx Feb 25, 2024 •

edited

Loading

jack20xx Feb 25, 2024 •

edited

Loading

jack20xx commented Feb 25, 2024 •

edited

Loading

keishi1129 Feb 25, 2024 •

edited

Loading

keishi1129 Feb 25, 2024 •

edited

Loading

keishi1129 Feb 25, 2024 •

edited

Loading

jack20xx Feb 26, 2024 •

edited

Loading

keishi1129 Feb 25, 2024 •

edited

Loading