Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

challengeA/okamoto #314

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

mochikichi
Copy link

@mochikichi mochikichi commented Jun 18, 2021

担当者様

challengeAの課題を提出させていただきます。
ご確認よろしくお願いします。

クラス設計

Simulatorクラス

  • 条件をPlansへ渡して対象プランを受け取る
  • (今回の用件にはないが)対象プランに対して手を加える場合(並び替えなど)があれば本Classで対応する

Plansクラス

  • dataを読み込み、Planへデータを渡す
  • Planインスタンスの配列を作る

Planクラス

  • Planインスタンスを作成
  • planの探索や必要な要素を抽出

テスト観点

  • プラン毎に契約アンペアの対象範囲が異なる
  • 従量制の境界値ごと
  • 条件に合致するプランがない場合

工夫した点

  • クラス、メソッドの単一責任を意識
  • プランデータの従量制の部分では、各レンジの最小値だけを持つようにした(レンジの最大値=次のレンジの最小値)
  • テストは可読性を犠牲にしない範囲で保守性を高めるために共通化した
  • コード中にコメントを記載しなくてもわかるように変数やメソッドに意味のある名称を意識

@ghost
Copy link

ghost commented Jun 18, 2021

Sider has detected 1 error and 2 warnings on analyzing the commit 3d2a545.

If the errors persist even after retrying, the following actions may resolve them:

If you still have problems, feel free to ask us via chat. 💬


You can turn off such notifications if unnecessary.

JSON.load(File.open(json_file_path))
end

def available?(plan)
Copy link
Contributor

Choose a reason for hiding this comment

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

この行以下のメソッドですが、planを引数に持つメソッドとなっています。
この辺りから、より良いクラス設計が考えられるのではないかと感じました。もしお時間があれば考えてみてください・・!

Copy link
Author

@mochikichi mochikichi Jun 21, 2021

Choose a reason for hiding this comment

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

@yuyasat
コメントありがとうございます。
おっしゃるとおり、すべてにplanを引数に持つということはplanのインスタンスを作る設計とした方が良さそうだと気付きました。

PlansクラスとPlanクラスへ分離してみましたので再度ご確認いただけると幸いです。

Plansクラス

  • dataを読み込み、Planへデータを渡す
  • Planインスタンスの配列を作る

Planクラス

  • Planインスタンスを作成
  • planの探索や必要な要素を抽出

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants