【藤巻】serverside_challenge_2: 電気料金シミュレーション機能の実装 #70
Conversation
tl-yoshirofujimaki
changed the title
| price = plan.electricity_price(ampere, usage) | ||
| next unless price | ||
|
|
||
| { | ||
| provider_name: name, | ||
| plan_name: plan.name, | ||
| price: plan.electricity_price(ampere, usage) | ||
| } |
There was a problem hiding this comment.
提出させていただいた後で申し訳ないのですが、こちら不要に２度価格の計算処理をしているようです
申し訳ありませんが、こちら修正させてください。
There was a problem hiding this comment.
また、合わせてテストコードを追記させていただきます
There was a problem hiding this comment.
修正と本番環境へのデプロイを実施いたしました。
| # frozen_string_literal: true | ||
|
|
||
| class ElectricityPriceCalculateService | ||
| def initialize(ampere, usage) | ||
| @ampere = ampere | ||
| @usage = usage | ||
| end | ||
|
|
||
| def calc | ||
| Provider.order(:id).map { |provider| provider.electricity_prices(@ampere, @usage) }.flatten | ||
| end | ||
| end |
There was a problem hiding this comment.
[COMMENT]
計算ロジックの大半が モデル側にあるため、Controllerに直接記載するか悩みましたが、今後のロジック修正や拡張を考慮し、サービスとして分離しています。
ロジックはモデルのメソッドを呼び出しているだけのため、
モデル側のテストで十分と判断し、このサービスクラスのテストは記載しておりません。
|
簡単にですが、インフラの構成図をPR説明文に追記いたしました |
| # Defines the root path route ("/") | ||
| # root "articles#index" | ||
|
|
||
| get '/health', to: proc { [200, {}, ['OK']] } |
There was a problem hiding this comment.
[COMMENT]
ELBからのヘルスチェック用エンドポイントとして作成しています
| RUN yarn install | ||
| RUN yarn build | ||
|
|
||
| CMD ["yarn", "preview", "--host", "0.0.0.0", "--port", "5173"] |
There was a problem hiding this comment.
[COMMENT]
今回ECSで配信するためにローカルで静的ファイル配信サーバーを立ち上げるpreviewモードを利用していますが、実運用だとbuild済みのファイルを別の方法で配信する方が本当は良いです
公式的にも非推奨
https://ja.vite.dev/guide/static-deploy
例えば、cloudfront+s3で配信するようなやり方の方とかが良さそうかな
今回はサーバーサイドの構築ががメインのため、一旦確認できる状態となることを優先して、このようなやり方をとっております
| <input | ||
| type='text' | ||
| :value='usage' | ||
| @input='event => usage = event.target.value'> |
There was a problem hiding this comment.
[COMMENT]
ここは 0 以上の整数が前提なので、type='number' の方が適切だったかもしれません。
レビュー提出後の修正は混乱を招く可能性があるため、今回は一旦そのままとさせていただきます。
細かい見落としがあり、申し訳ありません。
| end | ||
|
|
||
| def calc | ||
| Provider.order(:id).map { |provider| provider.electricity_prices(@ampere, @usage) }.flatten |
There was a problem hiding this comment.
チャレンジ提出いただきありがとうございます！
Providerに電気料金計算のロジックを持たせたのはどういった意図になりますでしょうか？
There was a problem hiding this comment.
@daichi1991
ご指摘ありがとうございます
Provider モデルに electricity_prices メソッドを持たせたのは、「プランの料金計算結果と provider_name をセットで返す」という要件に対応するためです
加えて、将来的に「特定の Provider のみのプラン情報を出力したい」という要望が出た場合の対応のしやすさも考慮しています
例えば、サービス側等 で次のように記述すれば、特定の Provider のみを対象とする処理が容易になるかと考えました
Provider.find(provider_id).electricity_prices(@ampere, @usage)主要な計算ロジックは Plan に集約しているため、Plan モデルを直接利用するのも一つの選択肢かと思います。
その場合、サービス側では次のような処理になるかと考えています。
Plan.joins(:provider).order(:provider_id).map do |plan|
price = plan.electricity_price(@ampere, @usage)
next if price.nil?
{
provider_name: plan.provider.name,
plan_name: plan.name,
price: price
}
end.compactもし既存のロジックに問題がある場合や、Plan からの直接呼び出しの方が望ましい場合は、修正いたします。
There was a problem hiding this comment.
コメントありがとうございます！
もし既存のロジックに問題がある場合や、Plan からの直接呼び出しの方が望ましい場合は、修正いたします。
いえあくまで考え方をお伺いしたかっただけですので、修正は不要かと思います。
Plan からの直接呼び出しの方が良いことが明らかになった場合にリファクタすれば良いと思います。
| def calc_usage_price(usage) | ||
| usage_rates = electricity_charges_usage_rates.order(:min_usage) | ||
|
|
||
| usage_price = 0 | ||
| remaining_usage = usage | ||
|
|
||
| usage_rates.each do |rate| | ||
| # 使用量の上限がない場合、すべてその単価で計算 | ||
| if rate.max_usage.nil? | ||
| usage_price += remaining_usage * rate.unit_rate | ||
| break | ||
| end | ||
|
|
||
| # 範囲内の使用量のみ計算 | ||
| range_usage = [remaining_usage, rate.max_usage - rate.min_usage].min | ||
| usage_price += range_usage * rate.unit_rate | ||
| remaining_usage -= range_usage | ||
|
|
||
| # 使用量が計算し終わったら終了 | ||
| break if remaining_usage <= 0 | ||
| end | ||
|
|
||
| usage_price | ||
| end |
There was a problem hiding this comment.
可読性が高く、何をやっているのか分かり易いです、ありがとうございます。
| unless ALLOWED_AMPERES.include?(ampere) | ||
| render json: { error: "アンペア数(ampere)は #{ALLOWED_AMPERES.join('/')} のいずれかを指定してください" }, status: :bad_request | ||
| return | ||
| end | ||
|
|
||
| if usage.negative? | ||
| render json: { error: '使用量(usage)は 0 以上の整数を指定してください' }, status: :bad_request | ||
| return | ||
| end | ||
|
|
||
| plans = ElectricityPriceCalculateService.new(ampere, usage).calc | ||
| render json: plans | ||
| rescue TypeError | ||
| render json: { error: 'アンペア数(ampere)と使用量(usage)の両方が指定されていません' }, status: :bad_request | ||
| rescue ArgumentError | ||
| render json: { error: 'アンペア数(ampere)と使用量(usage)を整数で指定してください' }, status: :bad_request | ||
| end |
There was a problem hiding this comment.
@tl-yoshirofujimaki
バリデーションを外部のクラスに切り出すとしたらどのような設計が良いでしょうか？
There was a problem hiding this comment.
@RyuyaIshibashi
コメントありがとうございます。
まず、今回コントローラー側にバリデーションを加えた意図についてですが
サービスクラスに渡す前の「入力データの検証」にあたるため、リクエストのパラメータチェックはコントローラーの責務として記載するのが自然と考えました
一方で、コントローラー側の責務が肥大化していくのを避けるという観点では、バリデーションを サービスクラス に切り出すのが良いかと考えています
今回のバリデーションはシンプルなため、コントローラー側で問題ないと判断しましたが、ビジネスロジックが絡むようなバリデーションについては、サービスクラスに切り出すべきだと考えています
またアンペア数や使用量の「0以上の数値であること」のような条件は、今後別の機能でも共通して利用する可能性があるため、そうしたバリデーションが汎用的に使われるのであれば、concerns への切り出しが DRYの観点では適しているかと思います
|
|
||
| return unless unbounded_count > 1 | ||
|
|
||
| errors.add(:base, '同一プラン内で使用量上限がない項目が2つ以上存在します') |
| <label> | ||
| <div> | ||
| 1ヶ月の使用量(kWh) | ||
| <span v-tooltip="'0以上の整数を入力してください'" class='info'>ℹ️</span> |
| <tr v-for="(plan, index) in electricityPrices" :key="index"> | ||
| <td>{{ plan.provider_name }}</td> | ||
| <td>{{ plan.plan_name }}</td> | ||
| <td>{{ plan.price }} 円</td> | ||
| <td>{{ plan.isCheapest ? '✅' : '' }}</td> | ||
| </tr> |
| usage_price = calc_usage_price(usage) | ||
|
|
||
| # 電気料金（基本料金 + 従量料金）を返却 | ||
| (basic_price + usage_price).floor |
There was a problem hiding this comment.
こちらについては以前弊社と調整を図った上での実装だったのですね！
失礼いたしました🙇
| create_table 'electricity_charges_basic_rates', comment: 'プラン毎の電気基本料金を格納する', force: :cascade do |t| | ||
| t.bigint 'plan_id', null: false | ||
| t.integer 'ampere', null: false, comment: '契約アンペア数(A)' | ||
| t.decimal 'basic_rate', precision: 10, scale: 2, null: false, comment: '基本料金(円)' |
There was a problem hiding this comment.
データ型をdecimal(10,2)にした背景があればお伺いしたいです。
There was a problem hiding this comment.
@daichi1991
小数を含む金額を正確に扱うことが求められるため、浮動小数点型 (float など) ではなく、固定小数点型で誤差のない decimal 型を選択しました
scale: 2 については現行必要とされる小数部2桁に対応できることが理由として設定しています
precision: 10に関しては明確な根拠を持って定めた数値ではないのですが、最大 99,999,999.99 円まで対応できるため、今後料金が増加したとしても余裕がある設定となっているのではと考えているところです
| t.bigint 'plan_id', null: false | ||
| t.integer 'min_usage', null: false, comment: '電気使用量(kWh)の下限値(境界値を含まない)' | ||
| t.integer 'max_usage', comment: '電気使用量(kWh)の上限値(境界値を含む)' | ||
| t.decimal 'unit_rate', precision: 10, scale: 2, null: false, comment: '従量料金単価(円/kWh)' |
There was a problem hiding this comment.
データ型をdecimal(10,2)にした背景があればお伺いしたいです。
There was a problem hiding this comment.
@daichi1991
こちらも electricity_charges_basic_rates のschemaでコメントさせていただいている箇所と同じ理由になります
https://github.com/enechange/coding-challenge/pull/70/files#r2001371168
| **※ `copilot/` ディレクトリが必要ですが、機密情報が含まれるためコミット対象に含めておりません** | ||
| **そのため、本手順は参考としてご確認していただけたら幸いです。** | ||
| **どうするのが正解かは分かりませんが、本当の運用であれば機密情報の管理はAWS Systems Manager 等で管理すべきかと思います。** |
There was a problem hiding this comment.
インフラのデプロイまでいただき、ありがとうございます！
AWS Copilotを採用した理由がありましたら、簡単に教えていただけますでしょうか？
There was a problem hiding this comment.
@RyuyaIshibashi
AWS Copilotを利用するに至った経緯ですが
まず、開発環境をDockerで構築していたため、本番環境も同様にコンテナで運用するのが望ましいと考え、デプロイ先としてECS + fargateを選択しました
次に、ECSへのデプロイ経験がなかったため、デプロイ方法の調査を進める中でAWS Copilotに行き着きました
Copilotは、ELBやVPC、ECRといった周辺インフラの構築からデプロイまで一貫して行えるため、素早く環境を構築したい状況に適していると考え、採用しました
|
@tl-yoshirofujimaki バックエンドのみならずフロントエンドも実装いただき、また各種ドキュメンテーションや注釈も大変わかりやすく、私自身大変勉強になりました。 面談当日もまたよろしくお願いいたします。お会いできることを楽しみにしております！ |
3 participants
概要
お世話になっております。藤巻です。
チャレンジ（コーディング試験）の課題として、
「serverside_challenge_2」の電気料金シミュレーション機能を実装いたしました。
課題の詳細: https://github.com/enechange/coding-challenge/tree/master/serverside_challenge_2
お手数ですが、以下の点についてご確認をお願いいたします。
設計の詳細については、README にまとめています。
よろしくお願いいたします。
実装内容
バックエンド
/electricity_pricesを実装し、契約アンペア数と使用量に応じた電気料金とそのプランをIDの昇順で返却フロントエンド
本番環境確認用URL
http://enecha-publi-yakvtpzir2fc-1409577172.us-east-2.elb.amazonaws.com/frontend/
デプロイ手順: README.md#本番デプロイ手順(aws)
※ チャレンジとしての利用のため、ELBのデフォルトドメインを直で利用しています
インフラの構成としてはこのようになります
ローカル環境での確認
以下手順を参考に環境構築の上、動作確認をお願いします。
環境構築手順: README.md#ローカル環境構築手順