【植田】料金シミュレーションAPIの実装 #43

Ray-Gee · 2023-12-09T13:22:51Z

概要

serverside_challenge_1 を実装しました、レビューお願いします。

やったこと

ユーザーから契約アンペア数(A)と1ヶ月の使用量(kWh)を受け取って、プランごとの電気料金を返すAPIを実装

確認方法

cd serverside_challenge_1/challenges/ryuichi_ueda
rails s

エンドポイント

ttp://127.0.0.1:3000/api/v1/plans/all/${ampere}/${usage}の形式で実装

http://127.0.0.1:3000/api/v1/plans/all/40/300

動作確認

30アンペア以上
30アンペア未満
無効な使用量(0未満)
無効なアンペア(10, 15, 20, 30, 40, 50, 60以外)
無効なエンドポイント

keishi1129 · 2023-12-11T12:36:00Z

serverside_challenge_1/challenges/ryuichi_ueda/app/models/electric_plan.rb

+  end
+
+  def total_charge(ampere, usage)
+    (basic_charge(ampere) + usage_charge(usage)).round


「電気代端数」で検索すると、「電気料金の合計額は円未満切り捨て」というふうに出てきます。

業界人でなければ普通知らないと思うのですが、業界だと当たり前なので顧客から明示的に仕様が提示されることは稀です。ですのでこの辺はENECHANGEのエンジニアとしては、しっかりと把握しておかなければなりません。

その上で、端数も出すという仕様にすることもあり得るかもしれませんし、プロジェクトには何年もこの業界で働いている人もいれば、入社したばかりの方もいますし、顧客においても不慣れな方がいるかもしれませんので、やはり認識合わせをしておくのが無難ですね。
植田さんも、「明示的ではないな、わからないな」と思った際には自身のコメントとともにレビュー依頼をかけていただくのが無難かなと思います。

なるほど、これは調べれば分かる事だったので自分の判断で四捨五入したのはミスですね、すいません、修正します。

keishi1129 · 2023-12-11T12:44:33Z

serverside_challenge_1/challenges/ryuichi_ueda/spec/models/tokyo_gas_plan_spec.rb

+      it '正しい合計料金が計算される' do
+        expect(tokyo_gas_plan.total_charge(30, 300)).to eq(expected_charge_30_300)
+      end
+    end


アンペア20を入れた時の異常系のテストも欲しいと思ったのですがいかがでしょうか。

そうですよね、このファイルより手前で30未満が渡らないようにしていたので、そのバリデーションがない場合はここでエラーになってしまっていたのでどう書けば良いか分かりませんでした。
今回は設計ごと変更してこのファイルがなくなるので次回からテストが書けない場合は設計を疑おうと思います、ありがとうございます。

keishi1129 · 2023-12-11T12:51:58Z

serverside_challenge_1/challenges/ryuichi_ueda/app/models/jxtg_plan.rb

+  def initialize
+    super('JXTGでんき')
+  end
+end


弊社では何千というプランを管理しています。

この設計にしてしまうと、今後プランが増えた時に毎回プランモデルを作らないといけない気がしますがいかがでしょうか。

チャレンジの説明の中にも下記のポイントが記載されていますので、すでに工夫されていたら、もっと楽になるようにお願いします！

「同様の料金体系のプランが対象プランに追加された場合も、ロジックを追加せずに済むようにしてください。」

了解です、ファクトリーパターンで書き直してみようと思います。

keishi1129 · 2023-12-11T13:33:50Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/provider_service.rb

+  def self.providers_info(ampere)
+    info = {
+      tokyo_electric: TokyoElectricPlan.new.provider_info,
+      loop: LoopPlan.new.provider_info


あまり固有名詞をハッシュのキーにしてハードコーディングしない方がいいと思います。プランが増えるごとに修正しないといけないので。

了解です、他の点も含めて他の設計を考えてみます。

ここは動的生成で書き換えました。

keishi1129 · 2023-12-11T13:41:41Z

serverside_challenge_1/challenges/ryuichi_ueda/app/models/electric_plan.rb

+
+class ElectricPlan
+  def initialize(provider)
+    @data = InitializeData.new(provider)


プランの数だけYAMLがロードされてしまいませんか？

providerを入れたら、基本料金や従量料金がinitializeされるという流れになっていますが本当にそうでしょうか。
東京電力さんは複数のプランがあり、それぞれ基本料金や従量料金が違いますよね。

重複ロードの部分はキャッシュで書き直しました。

initialize部分は迷走してるのでもう少々お待ちを。

keishi1129 · 2023-12-11T13:52:34Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/initialize_data.rb

+  private
+
+  def prepare_provider_data(provider)
+    load_provider(provider)


なぜload_providerはここで呼び出すのでしょうか。

load_providersで複数のプロバイダーが混合してる状態のものを取り出し、
load_providerでプロバイダーごとに情報を分け、
prepare_provider_dataでプロバイダーごとに情報を整形、
のつもりでしたがこだわりはないので他の方法考えてみますね。

keishi1129 · 2023-12-11T14:16:51Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/validator.rb

+      return false
+    end
+    true
+  end


このくらいのバリデーションであればactivemodelのvalidationを使った方がいいのかなと思うのですが、このようにすると何がメリットとしてありますか？

ここについては単にrailsの知識の無さからきてるものなので特に意図があったわけではありません。勉強して書き直します。

keishi1129 · 2023-12-11T14:18:46Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/initialize_data.rb

+  end
+
+  def load_provider(provider)
+    @provider_data = @providers_data[provider]


prepared_dataとに似ていて紛らわしい気がしました。

名前が紛らわしいというのもありますが、データの出どころだったり使い分けの仕方だったりが不明確な気がしました。他のエンジニアがprepared_dataからではなく@provider_dataからデータを取ってくる実装をしてきそうだなと思いましたが、多分そういう意図ではなくprepared_dataから必ず最終outputを出したいんですよね？

であれば命名やデータの作り方を少し工夫したいですね。

load_providersで複数のプロバイダーが混合してる状態のものを取り出し、
load_providerでプロバイダーごとに情報を分け、
prepare_provider_dataでプロバイダーごとに情報を整形、
のつもりでしたがこだわりはないので他の方法考えてみますね。

おっしゃる通り、時間空いて見返したら自分以外が見たら直感的じゃないなと感じました。
不要な細分化をまとめて書き直してみます。

keishi1129 · 2023-12-11T14:19:43Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/initialize_data.rb

+
+  def generate_tiers
+    @provider_data['tiers'].transform_keys do |key|
+      ['Infinity', '.inf'].include?(key) ? Float::INFINITY : key


'Infinity' なのか '.inf' なのかってどこで決まりますか？
揃えられるなら揃えてしまった方が、脳に優しい気がしますがいかがでしょうか。

.infで渡ってくる場合にも対応できるように合わせて書いた事があったのでそのまま前回と同じ書き方をしてしまってました。今回のデータでは全てInfinityなので書き換えます。

keishi1129 · 2023-12-11T14:31:28Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/initialize_data.rb

+    {
+      basic_charges: generate_basic_charges,
+      tiers: generate_tiers,
+      **{ provider => generate_plan }


ここって以下の方が直感的じゃないですか?

plan: generate_plan

そうですね、書き直します。

keishi1129 · 2023-12-11T14:49:04Z

serverside_challenge_1/challenges/ryuichi_ueda/app/models/electric_plan.rb

+
+class ElectricPlan
+  def initialize(provider)
+    @data = InitializeData.new(provider)


providerを入れたら、基本料金や従量料金がinitializeされるという流れになっていますが本当にそうでしょうか。
東京電力さんは複数のプランがあり、それぞれ基本料金や従量料金が違いますよね。

keishi1129 · 2023-12-11T14:50:22Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/initialize_data.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+class InitializeData


Dataというのは可能な限り避けた方が良いです。具体的に何を初期化するのかに合わせて考えた方がよく、何とも名付けにくい場合は初期化するデータが雑多すぎないか、責務が広すぎないか疑った方が良いです。

なるほど、了解です！今回ymlから渡されるデータは料金プランに関係するものがメインなのでInitializeChargePlanで書き直してみようと思います。

キーを動的生成に書き換え planを引数に取ったが未使用

RyuyaIshibashi · 2023-12-15T13:06:02Z

serverside_challenge_1/challenges/ryuichi_ueda/Dockerfile

このDockerfile作成の意図はなんですか？
（説明を見る限り、ローカルマシン上で動作させているようですが。）

これはrails newで自動作成されたのかなと。意図的に作ったものではありません。

RyuyaIshibashi · 2023-12-15T13:12:09Z

serverside_challenge_1/challenges/ryuichi_ueda/app/services/validator.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class Validator


私はこれがModel層にある方が自然だと感じたのですが、なんでだと思いますか？
（Service層でも全然問題ないので、実装はそのままで良いです！）

なるほど、Activeモデルのバリデーション機能を使っているからでしょうか？

RyuyaIshibashi · 2023-12-15T13:16:00Z

serverside_challenge_1/challenges/ryuichi_ueda/app/controllers/api/v1/plans_controller.rb

+        @ampere = params[:ampere].to_i
+        @usage = params[:usage].to_i


私はこの変換が乱暴だと思うんですが、なんでだと思いますか？

おっしゃる通りですね、ここに.to_i出来ない値が入る場合というのはエラーを出したい時かなと感じました。
今試したらhttp://127.0.0.1:3000/api/v1/plans/all/30/あで"あ"が0に変換されて意図しないレスポンスをしていたので修正します。

パラメータに文字列が渡ってきた場合のバリデーションとそのテスト追加

Ray-Gee added 3 commits December 9, 2023 21:47

初期設定と整形

2792a85

実装メイン

a9839ca

テスト

bb8c0f3

Ray-Gee marked this pull request as ready for review December 9, 2023 13:23

keishi1129 reviewed Dec 11, 2023

View reviewed changes

Ray-Gee added 3 commits December 12, 2023 20:51

12/12レビュー対応部分

482d949

ファクトリーパターンで書き直し

240dfd0

ロードをキャッシュで書き換え

41867c5

キーを動的生成に書き換え planを引数に取ったが未使用

RyuyaIshibashi reviewed Dec 15, 2023

View reviewed changes

Ray-Gee added 2 commits December 17, 2023 11:48

.to_iメソッドをより良いと思う位置へ変更

c346712

パラメータに文字列が渡ってきた場合のバリデーションとそのテスト追加

不要な細分化をまとめた

df90fd2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

【植田】料金シミュレーションAPIの実装 #43

【植田】料金シミュレーションAPIの実装 #43

Ray-Gee commented Dec 9, 2023

keishi1129 Dec 11, 2023 •

edited

Loading

Ray-Gee Dec 12, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 14, 2023

keishi1129 Dec 11, 2023 •

edited

Loading

keishi1129 Dec 11, 2023

Ray-Gee Dec 14, 2023

keishi1129 Dec 11, 2023 •

edited

Loading

Ray-Gee Dec 12, 2023

Ray-Gee Dec 15, 2023

keishi1129 Dec 11, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 15, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 14, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 12, 2023

keishi1129 Dec 11, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 14, 2023

Ray-Gee Dec 17, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 12, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 15, 2023

keishi1129 Dec 11, 2023

keishi1129 Dec 11, 2023

Ray-Gee Dec 12, 2023

RyuyaIshibashi Dec 15, 2023

Ray-Gee Dec 17, 2023

RyuyaIshibashi Dec 15, 2023

Ray-Gee Dec 17, 2023

RyuyaIshibashi Dec 15, 2023

Ray-Gee Dec 17, 2023

		@@ -0,0 +1,51 @@
		# frozen_string_literal: true

		class InitializeData

		@@ -0,0 +1,30 @@
		# frozen_string_literal: true

		class Validator

【植田】料金シミュレーションAPIの実装 #43

Are you sure you want to change the base?

【植田】料金シミュレーションAPIの実装 #43

Conversation

Ray-Gee commented Dec 9, 2023

概要

やったこと

確認方法

エンドポイント

動作確認

keishi1129 Dec 11, 2023 • 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 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keishi1129 Dec 11, 2023 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keishi1129 Dec 11, 2023 •

edited

Loading

keishi1129 Dec 11, 2023 •

edited

Loading

keishi1129 Dec 11, 2023 •

edited

Loading