【杉浦】サーバーサイドチャレンジ #64
【杉浦】サーバーサイドチャレンジ #64
Conversation
- rubocop-rails-omakaseを利用
| @@ -0,0 +1,26 @@ | |||
| 電力会社,プラン,契約アンペア数(A),基本料金(円) | |||
There was a problem hiding this comment.
確認事項です。
課題に提示されている料金表と電力会社のサイトに記載されている料金に差異がある物の対応 課題に記載の設定で実装しておりますが、対応が必要な場合はご指示ください。
以下の料金が異なっていました。
- 東京電力エナジーパートナー / 従量電灯B
- 基本料金
- 従量料金
- 東京ガス / ずっとも電気1
- 基本料金
- 従量料金
- Looopでんき / おうちプラン
- 従量料金
| # frozen_string_literal: true | ||
|
|
||
| class MeasuredRate < ApplicationRecord | ||
| MAX_SMALL_INT_VALUE = 32767 |
There was a problem hiding this comment.
上限無しの段階の電気使用量の最大値を定義しました。
段階の追加時のバリデーションの判定をしやすくする為、nullではなく必ず値を設定したいと考えました。
| # root "articles#index" | ||
| namespace :api, { format: "json" } do | ||
| namespace :electricity do | ||
| resources :calculate, only: [ :create ] |
There was a problem hiding this comment.
今回のAPIでは副作用がないためgetでも良いのですが、リクエストパラメータに住所等の個人情報に近い情報が入る可能性を考えるとpostの方がパラメータをセキュアなためpostを採用しました。
| # Character.create(name: "Luke", movie: movies.first) | ||
| require 'csv' | ||
|
|
||
| puts '* Start BasicPrice data' |
There was a problem hiding this comment.
CSVからDBへデータを投入するように対応しました。
| CMD ["rm", "f", "tmp/pids/server"] | ||
| EXPOSE 3000 | ||
| CMD ["bundle", "exec", "rails", "s", "-b", "0.0.0.0"] |
There was a problem hiding this comment.
ローカル環境とデプロイ環境でDoclerfileを共通化するため、コマンド実行を変更しました。
| @@ -0,0 +1,8 @@ | |||
| FROM ruby:3.1.2 | |||
There was a problem hiding this comment.
Cloud環境用のMigrationとDB seed実行用のimageです。
| @@ -0,0 +1,14 @@ | |||
| data = @prices[:plans].map do |row| | |||
There was a problem hiding this comment.
APIレスポンスのjsonを生成するためのSerializerです。
| const requestCalcPrices = async () => { | ||
| if (amperage === undefined) return; | ||
| try { | ||
| const response = await fetch(`${import.meta.env.VITE_API_URL}/api/electricity/calculate`, { |
There was a problem hiding this comment.
デプロイ環境にてスペックを低く指定しているため特に初回のAPIレスポンスが遅くなっております。
フロントエンドに関しては最低限とする方針で実装致しましたが、自分で単純に使いづらいと思いました。
もしよろしければPRを提出済みであるものの対応させていただけますでしょうか。
| rates = self.class.where(plan: plan).where.not(id: id) | ||
| validate_electricity_usage_min(rates) | ||
| validate_electricity_usage_max(rates) | ||
| validate_electricity_usage_contain(rates) |
There was a problem hiding this comment.
csv の上から順に create! を実行する = 上から順にバリデーションされて順番に登録される = 同じプランの既存レコードに使用量レンジがかぶっているものがないかのバリデーションを行う、
という実装であると理解しました。
ちゃんと想定通りの処理が行われそうだとは思いつつ、その上で以下の点が気になりました。
- 今回のように一括の登録をする場合にバリデーションで既存レコードの fetch が発生すると DB アクセスが増えてパフォーマンスに影響しそう
- レンジの被りがないかを3メソッドに分けて実装しているが1メソッドにできそう
(a[:min] <= b[:max]) && (b[:min] <= a[:max])
- 初見だと読むのに少し苦労した
There was a problem hiding this comment.
今回のように一括の登録をする場合にバリデーションで既存レコードの fetch が発生すると DB アクセスが増えてパフォーマンスに影響しそう
こちらは自分も少し気になった点ではありました。
実稼働において大量データの投入等によりパフォーマンスに影響する可能性はありますでしょうか。
データ量が多く実際に負荷がかかる可能性が高ければ、対応させていただきます。
現状はパフォーマンスへの影響とコード量のバランスを考え実装致しました。
- もし大量投入時の独自のvalidationを持った場合、実装によってはmodelのvalidation処理と冗長になり問題だと考えた
- 現時点のデータ量は少量かつ実際の稼働システムでどの程度影響がある事なのか判断がつかない
また、パフォーマンスに関しては以下も考えましたが現時点で過剰に対策しないように考えました。
- Readのパフォーマンスに関してはRedisやメモリーでキャッシュを利用する
There was a problem hiding this comment.
レンジの被りがないかを3メソッドに分けて実装しているが1メソッドにできそう
(a[:min] <= b[:max]) && (b[:min] <= a[:max])
初見だと読むのに少し苦労した
こちらは、おっしゃる通りですので再検討し提案させていただきます。
There was a problem hiding this comment.
レンジの被りがないかのチェックを簡略化しました。
問題点としてはメソッドが分離されていることによりコード量が多く把握が難しくなっている点だと認識しております。
AとBをmin, maxのrangeに定義し、A,B相互に範囲に重複が無いか確認するようにいたしました。
実装的にはrangeのinclude?で判定を行うようにいたしました。コード的にはかなり短くなりました。
こちら該当commitとなります。
| def price_calculate | ||
| @prices = Plan.calc_prices(create_params[:amperage], create_params[:electricity_usage_kwh]) | ||
| end |
There was a problem hiding this comment.
コントローラーはリクエスト、レスポンスを扱うのに徹する、という設計はいいと思いました。
action が一つだけだし only もついているので害はないですが、このメソッドを before_action にしたのはどういった意図からでしょうか？
There was a problem hiding this comment.
こちらの実装意図は無く手癖になります。
普段はshow,update,destroy時の共通のfind処理をbefore_actionで実装しています。
検索処理は共通処理ではないですがbefore_actionで同様に実装する事があり、今回同様に実装していました。
私からの提案としてはbefore_actionを廃止しresponse前に移動致しました。
こちら別途Pull Requestとして作成致しました。
|
|
||
| plans = Plan.all.includes(:provider) | ||
| basic_prices_hash = BasicPrice.calc_prices(amperage) | ||
| measured_rates_hash = MeasuredRate.calc_prices(electricity_usage_kwh) |
There was a problem hiding this comment.
先に全部のプラン分の従量料金を計算してから基本料金を持っていたプラン分のみ利用する、という実装だと MeasuredRate を取得する DB アクセスは減りますがプラン数が増えてリクエストのアンペアを含まないプランが増えた場合に無駄が多くなる気がしました。
また、今の実装だとどちらかというと計算の主体がプランではなく単価の方になっている感じがしてドメイン的に少し違和感を感じました。
（今回の要件だとプラン単体の計算をすることはないので今の実装でも問題はないと思います）
There was a problem hiding this comment.
今の実装だとどちらかというと計算の主体がプランではなく単価の方になっている感じがしてドメイン的に少し違和感を感じました。
こちらは実装時に計算の主体は何になるだろうと悩んだ部分でした。
プランが主体であることは確かなものの、おっしゃる通り実態は基本料金の結果となっております。
プラン単体の計算という可能性が考えられていませんでした。
こちら再検討させていただきまして、現状より簡潔な方法が考えられた場合提案させていただきます。
以下も併せて検討いたします。
先に全部のプラン分の従量料金を計算してから基本料金を持っていたプラン分のみ利用する、という実装だと MeasuredRate を取得する DB アクセスは減りますがプラン数が増えてリクエストのアンペアを含まないプランが増えた場合に無駄が多くなる気がしました。
There was a problem hiding this comment.
こちら現時点で私が想像できる内容で再検討し一つの案として実装してみました。
別途Pull Requestとして作成致しました。
現在把握していない計算項目やデータ量など想像できない部分があり、この実装の妥当性がわからないため別途PRとさせて頂きました。
やりたかったこととしては以下です。
- 各計算の実装を共通化
- 計算の全体的な制御としては計算ロジックの呼び出しとデータ整形に徹する
もし可能でしたらご意見お聞きしたいです。
|
ご多忙の中チャレンジの提出ありがとうございます！ 弊社からいくつかレビューコメントしますが、可能な範囲でご回答・対応いただけば大丈夫です。 |
| def check_parameters(amperage, electricity_usage_kwh) | ||
| errors = [ | ||
| BasicPrice.check_amperage?(amperage), | ||
| MeasuredRate.validate_electricity_usage?(electricity_usage_kwh) | ||
| ] | ||
| errors.select { |error| error[:is_error] } | ||
| .map { |error| error[:error_object] } | ||
| end |
There was a problem hiding this comment.
check_parameters メソッドは、他のクラスや外部からのアクセスが不要な内部処理なので、
private に設定して外部からの意図しないアクセスを防いだ方が良いかと思いました。
There was a problem hiding this comment.
こちらは当初Controllerから呼び出す想定も考えPublicに設定しそのままとなっておりました。
おっしゃる通り現状は外部からのアクセスがないため対応致しました。
併せて他のクラスで同様の問題がないか確認いたしました。
対応のcommitはこちらです。
|
|
||
| expect(response).to have_http_status(400) | ||
| body = JSON.parse(response.body, symbolize_names: true) | ||
| expect(body[:message]).to include('リクエストパラメーターが正しくありません。') |
There was a problem hiding this comment.
リクエストパラメーターが正しくありません。の前後に意図しない文字列が入ってきた場合でもテストがpassしてしまうため、eqマッチャーを使って完全一致のテストをした方が良さそうです。
There was a problem hiding this comment.
メッセージに関するテストは以下の2つのテストでeqで確認しております。
そのためAPIのメッセージは簡略的にしたいという意識があり判定が厳密ではない形になっておりました。
内容はおっしゃる通りですので対応いたします。
参考のtestコード。
-
BasicPrice.check_amperage?
https://github.com/kunihiko-sugiura/coding-challenge/blob/8f5a0b93952978550fc1b3d51c993d934f41a6e8/serverside_challenge_2/challenge/spec/models/basic_price_spec.rb#L148
対応内容は以下です。
- 「リクエストパラメーターが正しくありません。」のメッセージ判定をincludeからeqへ変更
- A数判定メッセージ「のいずれかを指定してください。」も併せてeqで判定できるように対応
- メッセージを複数箇所で利用するため、BasicPrice.ERR_MESS_INVALID_AMPERAGEにメッセージ定義を追加
- BasicPrice.ERR_MESS_INVALID_AMPERAGEのメッセージに対するtest codeを追加
対応のcommitはこちらです。
| return if electricity_usage_max.nil? || electricity_usage_min.nil? | ||
|
|
||
| if electricity_usage_max < electricity_usage_min | ||
| errors.add(:electricity_usage_max, "must be greater than or equal to electricity_usage_min") |
There was a problem hiding this comment.
アプリケーション全体のエラーメッセージとして日本語や英語で混じっていて
表示揺れがあるので統一させたいです。
こちらは日本語表記
There was a problem hiding this comment.
こちら日本語に統一する方針で対応させていただきます。
There was a problem hiding this comment.
エラーメッセージが英語と日本語が混在していたため日本語に統一いたしました。
対応内容は以下となります。
- i18nを導入
- locale設定を日本語に指定
- テストコードの修正
対応のcommitはこちらです。
| ADD . /app | ||
|
|
||
| CMD ["rm", "f", "tmp/pids/server"] |
There was a problem hiding this comment.
CMDは1 つだけ記述でき、複数あった場合は最後のみ適用されるのでこちらは実行されていない認識です。
https://docs.docker.com/reference/dockerfile/#cmd
こちらの実装意図は何でしょうか？
There was a problem hiding this comment.
こちらおっしゃる通りで対応漏れとなります。
普段はCMDでshellファイルを指定して対応しております。
こちらのcommitにて対応いたしました。
This reverts commit 7cf00d2.
| @@ -120,7 +120,7 @@ | |||
| res = Plan.send(:check_parameters, 0, 1000) | |||
| expect(res.size).to eq 1 | |||
| expect(res[0][:field]).to eq 'amperage' | |||
| expect(res[0][:message]).to eq "#{BasicPrice::AMPERAGE_LIST.join('/')}のいずれかを指定してください。" | |||
| expect(res[0][:message]).to eq BasicPrice::ERR_MESS_INVALID_AMPERAGE | |||
There was a problem hiding this comment.
実コードをテストに用いてしまうと以下が起きてしまいます。
- 意図しないメッセージでもpassしてしまう
- どんなメッセージが返却されるのかがわからない
テストも設計書だと考えることができ、わかりやすいテストを書くことで仕様理解をスムーズにできると思うのですがどのようにお考えでしょうか？
There was a problem hiding this comment.
上記は理解しておりますが、普段実コードをテストに利用する事があります。
ただ上記の内容を覆すほど、今回の実装が良いという明確な理由は持っていません。
以下懸念に対して個別に考えを記載いたします。
- 意図しないメッセージでもpassしてしまう
私もその懸念を持ちましたため、メッセージ単体のテストを追加し意図しない結果にならないように対応いたしました。
https://github.com/kunihiko-sugiura/coding-challenge/blob/cfeacf9380c2f70bc0f1fd1751bf8b8cc1eebbcf/serverside_challenge_2/challenge/spec/models/basic_price_spec.rb#L134-L146
- どんなメッセージが返却されるのかがわからない
- テストも設計書だと考えることができ、わかりやすいテストを書くことで仕様理解をスムーズにできる
上記2件に関してはeditorのコードジャンプやgrepが利用できるので現職で協力会社のメンバーも含めて問題として聞いたことがないため私には無い観点でした。
今後のテスト実装に参考にさせて頂きます。
- i18nを導入 - locale設定を日本語に指定 - テストコードの修正
|
|
||
| expect(body[:details].size).to eq 2 | ||
| expect(body[:details][0][:field]).to eq 'amperage' | ||
| expect(body[:details][0][:message]).to include('いずれかを指定してください。') |
There was a problem hiding this comment.
エラーメッセージをeq BasicPrice::ERR_MESS_INVALID_AMPERAGEではなくincludeでテストしていますが、意図的でしょうか？
There was a problem hiding this comment.
おっしゃる通り対応漏れとなります。
こちらのcommitにて対応いたしました。
またメッセージを検索し他に対応漏れがないことを確認しました。
| <input | ||
| type="text" | ||
| id="electricity-usage" | ||
| maxLength={5} |
There was a problem hiding this comment.
契約A数は選択にしており、同様に何かしら入力制限を行いたいと考えました。
制限したい理由
- UI崩れ等の考慮が減る
例）現実的ではない大きな数字を入力した際に画面が崩れる可能性を下げたい - 無制限にすることにより数値型の上限を超える可能性を潰したい
APIにリクエストしてからエラーを発生させる必要が無いものは、Frontendで制限をかけた方が良いと個人的には考えています。
5桁に指定した理由
上限を指定する弊害としては、ユーザが制限により計算ができないという点があります。
そのため一般的な電気料金を調べました。
4人家族の場合 436kwhとの情報が出てきました。現実的に1000kwh程度はあり得るのかと考えました。
では4桁で制限をかけるかと言うと業務知識もないため妥当か判断がつきませんでした。
DBの型定義の制限でも経験がありますが、予想を超えて大きな値が発生する事がありますため
明確に決まった形式がないため余裕を持たせたいと考え5桁といたしました。
| /> | ||
| </div> | ||
| <div> | ||
| <button onClick={requestCalcPrices} disabled={amperage == undefined}>計算</button> |
There was a problem hiding this comment.
24行目の===（厳密等価演算子）ではなく、==による条件式になっていますが意図的でしょうか？
There was a problem hiding this comment.
こちらおっしゃる通りです。
こちらのcommitにて対応いたしました。
こちら通常ですとEslint + Prettierで対応を行いますため、忘れておりました。
今回レビューの回答として上記の導入も検討いたしましたが、アプリケーションコード以外のファイル変更も増えますし割愛させて頂きました。
対応課題
serverside_challenge_2の対応を行いました。
デプロイした成果物
https://app.three-chairs.com/
対応内容
対応内容等の情報をwikiに記載しました。
ローカルの実行環境構築
READMEに記載しました。
その他
補足や確認事項をコメントに追加しております。