Conversation
05fcbd6 to
e4d0de2
Compare
06.bowling_object/game.rb
Outdated
| # ここでfirst_frame, second_frame・・・と宣言したら全体の記述が長くなり、 | ||
| # むしろ可読性が落ちると判断したので配列で宣言 | ||
| @frames = framed_scores.map do |score| | ||
| # 基本は10フレームだが、10フレーム目でスペアかストライクを出すと11フレーム生成される。 |
There was a problem hiding this comment.
オブジェクト指向で作るのであれば、現実世界のボウリングをできるだけ忠実に再現したいので11フレーム目を作成するのではなく10フレーム目に3投存在する形にしたいです 🙏
オブジェクト指向を用いる目的の一つに、「現実世界のルールやビジネスロジックを表現する」というものがあり、そこにギャップがある状態だとオブジェクト指向のメリットが薄れてしまうためです。
There was a problem hiding this comment.
ボウリングの仕様を正確に表現するために修正&クラスの責務の見直しにて修正しました!他の修正と切り分けることができず、コミットに複数の修正が混ざってしまっています。確認にお手数をおかけします。
オブジェクト指向を用いる目的の一つに、「現実世界のルールやビジネスロジックを表現する」
この点は自分が理解できていない所でした。ありがとうございます!
06.bowling_object/game.rb
Outdated
| frame_count.times do |index| | ||
| frame = @frames[index] | ||
| total_score += frame.total_score | ||
| total_score += calc_strike_bounus(index) if frame.strike? |
There was a problem hiding this comment.
calc_strike_bounus や calc_spare_bounus もろとも Frame#toral_score に移動させてはどうでしょうか?
今の実装だとGameクラス内の処理で「このフレームはストライクなのかスペアなのか」といったことや、「ストライクの場合は次のフレームを参照して...」といったことを気にしないといけなくなっていますが、その責務は Frame に持たせた方が全体的にコードがスッキリするはずです。
There was a problem hiding this comment.
ボウリングの仕様を正確に表現するために修正&クラスの責務の見直し
にて対応しました!コードがすっきりしたように思います。ありがとうございます!
06.bowling_object/game.rb
Outdated
| def total_score | ||
| total_score = 0 | ||
| frame_count = 10 | ||
| frame_count.times do |index| |
There was a problem hiding this comment.
Enumerable#sum を使うと total_score = 0 や total_score += といったコードを書かなくて良くなるはずです。利用を検討ください。
There was a problem hiding this comment.
ボウリングの仕様を正確に表現するために修正&クラスの責務の見直し
書式の修正
にて修正しました🙇♂️
06.bowling_object/shot.rb
Outdated
| private | ||
|
|
||
| def to_int_score(string_score) | ||
| return 10 if string_score == 'X' |
There was a problem hiding this comment.
ここは三項演算子としても良いですね。
そうすると1行で書けるので to_int_score を定義する意味は薄れる(initizlizeに全部書ける)かもしれません。
|
|
||
| require_relative 'game' | ||
|
|
||
| def main |
06.bowling_object/frame.rb
Outdated
| end | ||
|
|
||
| def total_score | ||
| return [@first_shot.score, @second_shot.score, @third_shot.score].sum unless @third_shot.nil? |
There was a problem hiding this comment.
ここは次のように書くと少しだけコードの重複を減らせます 👍
[@first_shot, @second_shot, @third_shot].map(&:score).sum
06.bowling_object/frame.rb
Outdated
| after_the_next_frame = @referable_frames[1] | ||
| if next_frame.strike? | ||
| # 9フレーム目の場合 | ||
| return next_frame.first_shot.score + next_frame.second_shot.score if after_the_next_frame.nil? |
There was a problem hiding this comment.
1投目と2投目のスコアの合計のみを取得するメソッドを Frame に追加すると、ここや total_score の処理を共通化できそうですね。
06.bowling_object/frame.rb
Outdated
| # 9フレーム目の場合 | ||
| return next_frame.first_shot.score + next_frame.second_shot.score if after_the_next_frame.nil? | ||
|
|
||
| next_frame.first_shot.score + after_the_next_frame.first_shot.score |
06.bowling_object/game.rb
Outdated
| def parse_scores(row_scores) | ||
| scores = [] | ||
| row_scores.split(',').each do |score| | ||
| scores.length < 18 && score == 'X' ? scores.push('X', '0') : scores << score |
There was a problem hiding this comment.
18 という数値は 2 * 9 という計算の結果導出された値だと思いますので、そのように記載した方がわかりやすいですね。
There was a problem hiding this comment.
そしてこの 18 という数値は複数箇所で使われているので、定数にまとめるとミスを減らせますし、定数名によって更に意図がわかりやすくなります。
There was a problem hiding this comment.
共用の数値を定数に修正にて対応しました!
18 という数値は 2 * 9 という計算の結果導出された値だと思いますので、そのように記載した方がわかりやすいですね
コードはできる限り仕様を直接表現した方が良いということですかね?ご指摘の通りだと思いました!
06.bowling_object/game.rb
Outdated
| scores.length < 18 && score == 'X' ? scores.push('X', '0') : scores << score | ||
| end | ||
| tenth_frame_scores = scores[18...scores.count] | ||
| framed_scores = scores[0..17].each_slice(2).to_a |
There was a problem hiding this comment.
0..17 のところは 0...18 と記載すると、ここも 18 という数値に統一できますね。
There was a problem hiding this comment.
|
|
||
| def parse_scores(row_scores) | ||
| scores = [] | ||
| row_scores.split(',').each do |score| |
There was a problem hiding this comment.
ここを row_scores.split(',').take(18).each とすると、このブロックの内側の scores.length < 18 の判定や、 framed_scores = scores[0..17].each_slice(2).to_a の [0..17] をなくせそうですね。
There was a problem hiding this comment.
ここを row_scores.split(',').take(18).each とすると
今回、row_scores.split(',')の結果返される配列の要素数に揺れがある(ストライク=Xと表現されていることが要因)ため、takeメソッドでの処理は難しいと判断しました。
現状の処理をより分かりやすく記述する方法が見つけられなかったため、この修正を保留しています。
もし何か良い方法があればご教授願いたいです🙇♂️
There was a problem hiding this comment.
なるほど、確かにこの時点ではxが未処理でしたね💦失礼いたしました。現状のままとしましょう。
06.bowling_object/game.rb
Outdated
| @frames.each_with_index do |frame, index| | ||
| # 9フレーム目であれば | ||
| if index.equal?(8) | ||
| frame.referable_frames = [@frames[index + 1]] |
There was a problem hiding this comment.
これは予め渡すのではなく Frame#total_score に引数として渡す方針をとってはどうでしょうか?
Frame にとって、スコア計算のとき以外で次のフレームや次の次のフレームが欲しくなるシチュエーションがおそらくないため、インスタンス変数としてもたせる必要はないと考えます。
There was a problem hiding this comment.
ご指摘の通りです。不必要に変数のスコープが広いため要修正だと思いました。
必要以上に変数のスコープが広かったため修正にて修正しました!
06.bowling_object/game.rb
Outdated
| total_score = 0 | ||
| @frames.each_with_index do |frame, index| | ||
| # 10フレーム目の場合nil | ||
| next_frame = !index.equal?(9) ? @frames[index + 1] : nil |
There was a problem hiding this comment.
ここは三項演算子を使わずとも、最終フレームの場合は自ずとnilが代入されるはずです。
There was a problem hiding this comment.
9フレーム目も10フレーム目も自ずとnilが代入されました👀
条件分岐が必要無いことが分かったのですっきりさせました
必要のない条件分岐であったため修正にて対応しています🙇♂️
を実装しました。
正しい合計点数を返すことができるかをチェックするテストコードも作成しました
確認お願い致します