Skip to content
Open
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
ce0d022
add api routes
nkmr-mty Jul 16, 2025
f0863ab
add api controller with test code
nkmr-mty Jul 16, 2025
5408876
add gem active_hash
Jul 21, 2025
51405de
add csv files
Jul 21, 2025
eaadf5a
add models
Jul 21, 2025
4970ab2
rename controller, add codes
Jul 21, 2025
74a4b2f
add secret_key_base for production
Jul 21, 2025
771639c
update secret_key_base for production
Jul 21, 2025
46c3d9f
revise amp validatemethod, correct method name for error processing
Jul 23, 2025
ec7f650
add rounding decimal process, separate provider management data from …
Jul 23, 2025
4cc5b0e
add provider model
Jul 23, 2025
8eb6b5c
create migration, seed file, executed migration
Jul 24, 2025
e897b3c
rename existing model with 'csv' suffix, add provider model
Jul 24, 2025
f535198
add model for db management
Jul 24, 2025
2371165
correct seed.rb
Jul 24, 2025
061e019
change data reference from CSV to DB on processing calculation
Jul 24, 2025
2cdbfd2
add rubocop
Jul 24, 2025
5909b8b
rubocop code correction
Jul 24, 2025
78237d9
remove csv data and models
Jul 24, 2025
b9f9e59
add empty lines
Jul 24, 2025
14325c5
restore credentials.yml.enc
Jul 25, 2025
e48eade
correct meter charge amount calculation
Jul 25, 2025
56818cc
correct rouding calculation, correct single meter_rate_charge pattern…
Jul 26, 2025
5f7d63e
change column provider_name and plan_name to name
Jul 26, 2025
517b890
correct meter_rate calculation condition for single meter charge step…
Jul 28, 2025
519b5a6
rubocop code correction: db/seeds.rb
Jul 28, 2025
db2b5d6
add service class, spread processing respoisibiities from controller …
Jul 30, 2025
e1890a7
prevent n+1
Jul 31, 2025
a0d952f
add rspec gem
Jul 31, 2025
18d89ba
add spec
Jul 31, 2025
ce69d19
change api method post to get
Jul 31, 2025
344ac7d
truncate total amount
Jul 31, 2025
f437eb7
remove active_hash
Jul 31, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions serverside_challenge_2/challenge/.rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Documentation:
Enabled: false
MethodLength:
CountComments: true
Max: 25
Style/FrozenStringLiteralComment:
Enabled: false
Metrics:
Enabled: false

3 changes: 3 additions & 0 deletions serverside_challenge_2/challenge/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ gem "bootsnap", require: false
# Use Rack CORS for handling Cross-Origin Resource Sharing (CORS), making cross-origin AJAX possible
# gem "rack-cors"

gem "active_hash"
gem 'rubocop-rails', '~> 2.32'

group :development, :test do
# See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
gem "debug", platforms: %i[ mri mingw x64_mingw ]
Expand Down
39 changes: 39 additions & 0 deletions serverside_challenge_2/challenge/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ GEM
erubi (~> 1.4)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.1, >= 1.2.0)
active_hash (3.3.1)
activesupport (>= 5.0.0)
activejob (7.0.8)
activesupport (= 7.0.8)
globalid (>= 0.3.6)
Expand All @@ -66,6 +68,7 @@ GEM
i18n (>= 1.6, < 2)
minitest (>= 5.1)
tzinfo (~> 2.0)
ast (2.4.3)
bootsnap (1.18.3)
msgpack (~> 1.2)
builder (3.2.4)
Expand All @@ -84,6 +87,9 @@ GEM
irb (1.11.2)
rdoc
reline (>= 0.4.2)
json (2.13.0)
language_server-protocol (3.17.0.5)
lint_roller (1.1.0)
loofah (2.22.0)
crass (~> 1.0.2)
nokogiri (>= 1.12.0)
Expand Down Expand Up @@ -111,7 +117,12 @@ GEM
racc (~> 1.4)
nokogiri (1.16.2-x86_64-linux)
racc (~> 1.4)
parallel (1.27.0)
parser (3.3.8.0)
ast (~> 2.4.1)
racc
pg (1.5.4)
prism (1.4.0)
psych (5.1.2)
stringio
puma (5.6.8)
Expand Down Expand Up @@ -148,16 +159,42 @@ GEM
rake (>= 12.2)
thor (~> 1.0)
zeitwerk (~> 2.5)
rainbow (3.1.1)
rake (13.1.0)
rdoc (6.6.2)
psych (>= 4.0.0)
regexp_parser (2.10.0)
reline (0.4.2)
io-console (~> 0.5)
rubocop (1.78.0)
json (~> 2.3)
language_server-protocol (~> 3.17.0.2)
lint_roller (~> 1.1.0)
parallel (~> 1.10)
parser (>= 3.3.0.2)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 2.9.3, < 3.0)
rubocop-ast (>= 1.45.1, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 2.4.0, < 4.0)
rubocop-ast (1.46.0)
parser (>= 3.3.7.2)
prism (~> 1.4)
rubocop-rails (2.32.0)
activesupport (>= 4.2.0)
lint_roller (~> 1.1)
rack (>= 1.1)
rubocop (>= 1.75.0, < 2.0)
rubocop-ast (>= 1.44.0, < 2.0)
ruby-progressbar (1.13.0)
stringio (3.1.0)
thor (1.3.0)
timeout (0.4.1)
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (3.1.4)
unicode-emoji (~> 4.0, >= 4.0.4)
unicode-emoji (4.0.4)
websocket-driver (0.7.6)
websocket-extensions (>= 0.1.0)
websocket-extensions (0.1.5)
Expand All @@ -168,11 +205,13 @@ PLATFORMS
x86_64-linux

DEPENDENCIES
active_hash
bootsnap
debug
pg (~> 1.1)
puma (~> 5.0)
rails (~> 7.0.8)
rubocop-rails (~> 2.32)
tzinfo-data

RUBY VERSION
Expand Down
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全体的に、コントローラーにすべての責務が集中してしまっている印象を受けました。

具体的には、以下のような処理がすべて controller に記述されているため、保守性や可読性の観点から、責務ごとに適切に分離した方が良いと考えています。

  • リクエスト/レスポンスの処理
  • ビジネスロジック(例:料金計算など)
  • パラメータのバリデーションチェック

現時点で大きく実装を変更するのは大変かと思いますので、「今後リファクタリングする場合に、どのように責務を分けて実装するべきか」について、テキストベースで構いませんので、考えを共有いただけると嬉しいです。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コントローラの処理を最小限にし、処理を分離いたしました。db2b5d6

  • 分離先
    • PowerSupplyPlanSimulator(service class)
    • PowerSupplyPlan(model)
    • MeterRateCharge(model)

Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# frozen_string_literal: true

module Api
module V1
class PowerSupplyPlanController < ApplicationController
def simulate_all
amp = params[:amp]
meter_rate = params[:meter_rate]
bad_request_result = build_200_message(amp, meter_rate)
return render json: bad_request_result, status: 400 if bad_request_result.present?

amp = amp.to_i
meter_rate = meter_rate.to_i
plans = PowerSupplyPlan.all
result = []
plans.each do |plan|
basic_charge = plan.basic_charges.where(amp: amp)
next if basic_charge.blank?

meter_charge_price = meter_charge(plan, meter_rate)
next if meter_charge_price.negative?

result << {
provider_name: plan.provider.name,
plan_name: plan.name.to_s,
price: (basic_charge.first.price.to_f + meter_charge_price).floor(0).to_i
}
end
render json: result
end

private

def meter_charge(plan, meter_rate)
price = 0
plan.meter_rate_charges.each do |step|
price += calculate_if_step_applicable(step, meter_rate)
end
price
end

def calculate_if_step_applicable(step, meter_rate)
max_rate = step.max_meter_rate
min_rate = step.min_meter_rate
price = step.price


# 最小ステップ かつ 最大ステップ(従量料金設定が1ステップのみのケース)
return meter_rate * price if min_rate.zero? && max_rate.nil?

# 使用量がステップ請求額に満たない場合0を返す
return 0 if meter_rate < min_rate
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[want]
calculate_if_step_applicableメソッドで、計算が不要なステップでも毎回0を返すループを実行しているのは非効率です。使用量に応じて必要なステップのみを処理するよう、早期リターンや条件分岐の最適化を検討してみていただけると良いと思います。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if max_rate.nil? && min_rate <= meter_rate
# 最大料金ステップの計算
return (meter_rate - min_rate + 1) * price
end

if min_rate.zero?
# 最小料金ステップの計算
if max_rate <= meter_rate
# 該当ステップの満額請求計算
max_rate * price
else
# 該当ステップの使用量までを計算
meter_rate * price
end
elsif (min_rate..max_rate).include?(meter_rate)
# 中間料金ステップの計算
# 該当ステップの使用量までを計算
(meter_rate - min_rate + 1) * price
else
# 該当ステップの満額請求計算
(max_rate - min_rate + 1) * price
end
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

条件分岐が多く、ネストが深くなっているため「ガード節を活用」、「メソッド分割」してネストを浅くしたいです。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MeterRateCharge モデル内にて分岐処理を調整しました。
db2b5d6#diff-aed437e48cb04cf29acee4e9b374fec1823f2c6695befe5eb45120f1bf3c0876R12


def build_200_message(amp, meter_rate)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

内容を見る限り、バリデーション処理およびエラーメッセージの生成を行っているように見受けられます。
一方でメソッド名からは、HTTP 200のレスポンスを構築するような印象を受けるため、命名と実装の責務が一致していないと感じました。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q]
加えて、バリデーションはcontroller内で実施するのが適切でしょうか?意図があれば教えていただきたいです。

Copy link
Copy Markdown
Author

@monakam01 monakam01 Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sugita-seiya
メソッド名を validate_params とし、調整いたしました。
db2b5d6#diff-362c058661917b8d89d680d4aadeea1020721ce0f8fe3d34b943450d01f2be99R34

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mochikichi
モデルに依存しないパラメータのため、controller 内で書いておりましたが、新設したPoweSupplyPlanSimulator サービスクラス内にて行うよう変更いたしました。
db2b5d6#diff-362c058661917b8d89d680d4aadeea1020721ce0f8fe3d34b943450d01f2be99R34

message = ''
if amp.blank?
message += "リクエストパラメータ 'amp' が不足しています。\n"
else
unless valid_amp?(amp)
message += "リクエストパラメータ 'amp' の値が不正です。'10 / 15 / 20 / 30 / 40 / 50 / 60' のいずれかの値を指定してください。\n"
end
end
if meter_rate.blank?
message += "リクエストパラメータ 'meter_rate' が不足しています。\n"
else
unless numeric?(meter_rate) && meter_rate.to_i >= 0
message += "リクエストパラメータ 'meter_rate' の値が不正です。0以上の整数を指定してください。\n"
end
end
return errro_result(message) if message.present?

[]
end

def numeric?(string)
Integer(string)
true
rescue ArgumentError, TypeError
false
end

def valid_amp?(amp)
[10, 15, 20, 30, 40, 50, 60].include?(amp.to_i)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[want]
マジックナンバーになっているので定数化できると良いかと思います。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

def errro_result(result)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits]
errorのタイポかと思われます。

[
{
error: {
code: 400,
message: result
}
}
]
end
end
end
end
5 changes: 5 additions & 0 deletions serverside_challenge_2/challenge/app/models/basic_charge.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# frozen_string_literal: true

class BasicCharge < ApplicationRecord
belongs_to :power_supply_plan
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# frozen_string_literal: true

class MeterRateCharge < ApplicationRecord
belongs_to :power_supply_plan
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class PowerSupplyPlan < ApplicationRecord
belongs_to :provider
has_many :meter_rate_charges
has_many :basic_charges
end
5 changes: 5 additions & 0 deletions serverside_challenge_2/challenge/app/models/provider.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# frozen_string_literal: true

class Provider < ApplicationRecord
has_many :power_supply_plans
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credentialsはどこにも使用されていなさそうです。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

追加項目 の本番デプロイ対応のためmaster.key とセットで追加しておりました。
削除してしまうと本番デプロイに失敗するため、このままとさせていただければと思います。
master.key は本番環境の環境変数に設定(git管理外)しているため、差分としてはございません。

Original file line number Diff line number Diff line change
@@ -1 +1 @@
bZE5VzGsIQoeBEOa79I28cq/Ax0SuIaPhy5+NFktLVZLNGpuzqmkTkAiWc+arbGUDU8aaedg9zUvnBBc0dPrATeWISHtFxVwJMScZQeSW1k7BUgSD6B6YsewlgtpEDFDAXoGZEQoTbM2FY3lDCd0dZF7I1+DsB4I9Z5NamOgOlNEK8gMQD3p3Csqc/mVhmdI3PjBEmblvTjkZIVOdfX1CmqJ/RZm1Lxu3RdNMaiXiOZL7I+oXE1SFfjWm7+CTwb+Vfs6nGMviDHDKh9k3jyb7Vflpx/frY4vYzP8WMzgbJPX4UHmxa7qS51tJ1ECpK265nS1mLsqxL7wRbjyzfUNtXknSSopGD7sDDm7F9ta8/e1nzU79uTH1LKFpWQWGMSbXmSu4EKPu655BA9ckdwnSsf67RYNX7/Nw5pp--9AT1V0z/SBAdVTZ5--JK/4x776vcnN7qGZuylM0g==
aLpSiYFmK7YeuO/q62KJR/UyXPpBWhH+oS6rzGWSQH/9p53vCpRMvfaUsZSneP9wYMioohjDqHdeY0bpOAXjMPy5suQk7yGANivapLmd2qNLt1wpIYzKGJiEwSP2gQD4Hyo/K6qGF2fchngDFhy0WWAe8Q0kcIM7WzUSV+VilcYylHcHHpdYhjsBx4+7ev2dbWLlYxbYuFSDGklGP85oYBmIH2QIzcYmwHhBkjwzFWvIl3xqL+/8iQlYsFyBsTk3tntgD/Au3kv6qYXQnf3ITXgHTKlaooxLTy9ZYwMOhJvPwtkMsWv9CVY+ACkUi5h9WY+rSHiBVCDHJwnEhwpz8/n4kzGXLuGRburdDZkA1IZb+j7+KzX625Y+x4+zR7P1wc8vJRw+/5O4MSndI28y0KkX15Qd8vMYNrowsXx+0WFtvo8LTh0J6p84b4bJyVkD1bGUpvpz+UADtGfTbuDqqRXNDCRM7NITVYcOg74QKiwi57gzRJgKOYt1KhXMLA5y4V+xo2zBZuTzGAqNS2rHcfwil9lcsmsTOQL7felWv2C2qqUllUo3CqTGNlXo3A24guP25tr3vDm2IrH7V/TLSjJcDAI1JUi35wwrCErgxrwK9eG0Y4w=--97QWVDZ45R2Bjr3G--byi/t6soITzi5vJv5PEsvw==
5 changes: 5 additions & 0 deletions serverside_challenge_2/challenge/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,9 @@

# Defines the root path route ("/")
# root "articles#index"
namespace :api do
namespace :v1 do
post "simulate_all_plan", to: "power_supply_plan#simulate_all"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q

POSTメソッドを選定した理由があれば教えていただけると嬉しいです!

Copy link
Copy Markdown
Author

@monakam01 monakam01 Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

深い意図なくPOSTメソッドを使用しておりましたが、
今回の要件ではデータの更新に関連しないこと、送信データも機密情報でも大きなデータでも無いことから、
GETの方が適切と判断し変更いたしました。ce69d19

end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
class CreateBasicCharges < ActiveRecord::Migration[7.0]
def change
create_table :providers do |t|
t.string :provider_name, :null => false
t.string :rounding_amount_method, :null => false

t.timestamps
end

create_table :power_supply_plans do |t|
t.belongs_to :provider
t.string :plan_name, :null => false

t.timestamps
end

create_table :meter_rate_charges do |t|
t.belongs_to :power_supply_plan
t.integer :min_meter_rate, :null => false
t.integer :max_meter_rate
t.decimal :price, precision: 6, scale: 2, :null => false

t.timestamps
end

create_table :basic_charges do |t|
t.belongs_to :power_supply_plan
t.integer :amp, :null => false
t.decimal :price, precision: 6, scale: 2, :null => false

t.timestamps
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class RemoveRoundingAmountMethodFromProvider < ActiveRecord::Migration[7.0]
def up
remove_column :providers, :rounding_amount_method, :string
end

def down
add_column :providers, :rounding_amount_method, :string
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RenameProviderNameColumnToName < ActiveRecord::Migration[7.0]
def change
rename_column :providers, :provider_name, :name
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RenamePlanNameColumnToName < ActiveRecord::Migration[7.0]
def change
rename_column :power_supply_plans, :plan_name, :name
end
end
50 changes: 50 additions & 0 deletions serverside_challenge_2/challenge/db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading