Uh oh!
There was an error while loading. Please reload this page.
shimizu-challenge - #76
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
ご提出ありがとうございます！
細かい点ですがコメントしましたので、ご確認お願いします。
併せて、追加で3点コメントさせてください。
◽️1つ目
弊社ではテストコードを非常に重視しており、可能な範囲でテストの追加をご対応いただけますと幸いです。
もしご都合などでテスト追加が難しい場合は、ファイル単位でテスト観点を箇条書きで共有いただけますでしょうか。
例：
controllers/api_controller.rb
- 正常系
- xxxx
- 異常系
- xxxx
◽️2つ目
計算結果において、電力量としてマイナス値（price: -1）が返却されているため、計算処理の見直しをお願いします。
◽️3つ目
使用量に非常に大きい数値を入力した際、計算結果として不自然に大きな値（例：1.5285e+49 など）が返却されています。
こちらもご確認お願いします。
There was a problem hiding this comment.
コントローラ全体がネストされておらず、不要な空白も多いため、Rubocop を導入してコードスタイルを統一するのがよさそうです。
さらに、ApiController というクラス名は抽象的で、今後 API が増えた際に責務が曖昧になる可能性があります。
There was a problem hiding this comment.
ElectricityApiControllerに変更いたしました
| # 基本料金 + 従量料金 を小数点2桁で丸めて返す | ||
| (fee + usage_fee).round(2) |
There was a problem hiding this comment.
Q
小数点2桁で丸めた理由はなんでしょうか？
実装の意図をお聞きしたいです。
There was a problem hiding this comment.
対象プランと料金表の基本料金、従量料金単価ともに少数2桁で表現してありましたのでそれに揃える形で少数2桁で丸めております。
| Rails.logger.error("Error calculating fee: #{e.message}") | ||
| -1 | ||
| end |
There was a problem hiding this comment.
IMO
-1 がマジックナンバーになっているように見えます。
特別な意味を持たせている場合は、定数化を検討いただくと、実装意図がより明確になり、コードも読みやすくなりそうです。
There was a problem hiding this comment.
\config\initializers\constants.rb を作成し定数としました。
| @@ -0,0 +1,25 @@ | |||
| class Api::ElectricityController < ApplicationController | |||
There was a problem hiding this comment.
どのようなことを試したか残しておりましたが使用していないファイルは削除いたしました
| @@ -0,0 +1,56 @@ | |||
| # app/models/energy_plan_db.rb | |||
There was a problem hiding this comment.
不要なファイルは削除いたしました
energy_plan_db.rbはDBからのデータ取得に使用しております
| get "api/get_price", to: "api#get_price" # JSON API | ||
| get "api/get_price_db", to: "api#get_price_db" # JSON API |
There was a problem hiding this comment.
Q
APIの設計的にGET api/get_priceというpathのget_部分が冗長だと思いました。
また、このパスは何かの価格を取得するということはわかりますが、何をするAPIなのか不明確だと思いました。
お考えお聞きできますでしょうか。
There was a problem hiding this comment.
get_priceとget_price_dbとあり、get_priceは設定値をYAML、get_price_dbは設定値をDBから取得し計算しております。
| @@ -0,0 +1,64 @@ | |||
| class ApiController < ApplicationController | |||
| def get_price | |||
There was a problem hiding this comment.
一つのControllerに３つの異なるpathのget actionが含まれています。
ファイル分割した方が良いと思いました。
There was a problem hiding this comment.
今まではControllerに複数のアクションを持つような設計をしておりました。
Rubyでの開発経験が無いのでRubyでは基本1つのアクションであったり御社の規約がそのようであれば合わせます。
There was a problem hiding this comment.
ご回答ありがとうございます。
Rubyでの開発経験が無いので
こちら伺っています。修正などは不要です。
| - provider_name: 東京電力エナジーパートナー | ||
| plan_name: 従量電灯B | ||
| basic_fee: | ||
| 10: 286.0 | ||
| 15: 429.0 | ||
| 20: 572.0 | ||
| 30: 858.0 | ||
| 40: 1144.0 | ||
| 50: 1430.0 | ||
| 60: 1716.0 | ||
| unit_fee: | ||
| "0-120": 19.88 | ||
| "121-300": 26.48 | ||
| "301-0": 30.57 | ||
| - provider_name: 東京電力エナジーパートナー | ||
| plan_name: スタンダードS | ||
| basic_fee: | ||
| 10: 311.75 | ||
| 15: 467.63 | ||
| 20: 623.50 | ||
| 30: 935.25 | ||
| 40: 1247.00 | ||
| 50: 1558.75 | ||
| 60: 1870.50 | ||
| unit_fee: | ||
| "0-120": 29.80 | ||
| "121-300": 36.40 | ||
| "301-0": 40.49 | ||
| - provider_name: 東京ガス | ||
| plan_name: ずっとも電気1 | ||
| basic_fee: | ||
| 30: 858.0 | ||
| 40: 1144.0 | ||
| 50: 1430.0 | ||
| 60: 1716.0 | ||
| unit_fee: | ||
| "0-140": 23.67 | ||
| "141-350": 23.88 | ||
| "351-0": 26.41 | ||
| - provider_name: Looopでんき | ||
| plan_name: おうちプラン |
There was a problem hiding this comment.
Q
developmentとproductionで同一のデータ定義をしている理由はありますでしょうか。
There was a problem hiding this comment.
developmentはローカル環境、productionはサーバー環境での設定値になりますので同じ値でも分けて定義しております
There was a problem hiding this comment.
承知しました。修正など不要です。
本番、開発用でそれぞれ定義するとマスタ更新時の手間が増えると思いお聞きしました。
yamlの場合設定の共通化が可能ですので、それを使うと良いと思いました。
| unit_fee: | ||
| "0-0": 28.8 | ||
| - provider_name: YAMLtest |
There was a problem hiding this comment.
Q
YAMLで計算をクリックした場合の結果にテスト用らしきデータが出てきますが、要件にないので何を想定して出力しているのかをお聞きしたいです。
{
"provider_name": "YAMLtest",
"plan_name": "テスト",
"price": -1
}
There was a problem hiding this comment.
画面から確認するのにYAMLからデータを取得しているか、DBからデータを取得しているか目に見えて分かるようにYAMLのほうに仮で設定値を追加いたしました。
| # この従量料金が適用される最小使用量 (kWh) | ||
| min = uf.min_kwh.to_f | ||
| # 最大使用量が未設定(nil) または 0 の場合は「上限なし」として扱う | ||
| max = uf.max_kwh.nil? || uf.max_kwh.zero? ? Float::INFINITY : uf.max_kwh.to_f |
There was a problem hiding this comment.
Q
max_kwh,min_kwhはUI上整数値受付となっていますがDBとこの処理ではfloatで扱われています。
整数値ではなくfloatで扱っている理由はありますでしょうか。
There was a problem hiding this comment.
計算する際にfloatで合わせたら楽かなと思いfloatにしました。特に楽ということもありませんでしたね。
| secret_key_base: 022eeea32ce3537c8caa3d494d34cbb5ea55bc5ff97a6f656a329973dd27c408f335ffca6d39184a8ab695062aa1f3cb5a64eb8c4cea22c39d12c651d0590212 | ||
| production: | ||
| secret_key_base: 022eeea32ce3537c8caa3d494d34cbb5ea55bc5ff97a6f656a329973dd27c408f335ffca6d39184a8ab695062aa1f3cb5a64eb8c4cea22c39d12c651d0590212 No newline at end of file |
There was a problem hiding this comment.
Bug: Hardcoded Secret Key Across Environments
The secret_key_base is hardcoded and identical across development, test, and production environments. This is a serious security vulnerability. Secret keys are typically unique per environment and not stored in version control, which exposes the production key and can lead to session hijacking and application compromise.
| [[services]] | ||
| protocol = 'tcp' | ||
| internal_port = 8080 |
There was a problem hiding this comment.
Bug: Port Mismatch in Configuration
The fly.toml config sets internal_port to 8080, but the Dockerfile CMD starts the Rails server on port 3000. This port mismatch makes the application unreachable.
| database: app_test | ||
| host: db | ||
| port: 5432 | ||
| database: challenge_db |
There was a problem hiding this comment.
Bug: Missing Test Environment Configuration
The database configuration is missing the test environment section. Rails applications rely on a separate test database, and its absence will lead to test failures.
shimizu0429
commented
Oct 6, 2025
コメントについて返信させていただきます。 正常系 ●2つ目について ●3つ目について ■energy_plansテーブルについて |
shimizu-challenge
Note
Introduces electricity pricing API (YAML and DB-backed), supporting models/migrations, config/routes, and deployment/docker setup with minor env and gem updates.
ApiControllerwithGET /api/get_price(YAML) andGET /api/get_price_db(DB) returning computed electricity fees.EnergyPlan(YAML-based),EnergyPlanDbwithBasicFeeandUnitFee.energy_plans,basic_fees,unit_fees.config/api_settings.ymland initializerconfig/initializers/load_settings.rb.config/routes.rbto expose API endpoints; add minimal layout andpublic/index.htmltest UI.config/application.rbto include cookies/session/flash middleware.config/database.yml(dev DB host/name; production viaDATABASE_URL).config/environments/production.rb(hosts, logging) andconfig/puma.rb..dockerignore, overhaulDockerfilefor production run, addfly.toml.rails-html-sanitizer; update Rails and related gems inGemfile.lock.Written by Cursor Bugbot for commit 6433bae. This will update automatically on new commits. Configure here.