ボウリングのスコア計算プログラムの演習問題 by 1mori · Pull Request #3 · 1mori/ruby-practices

1mori · 2024-05-13T06:24:39Z

No description provided.

yuuu · 2024-05-19T20:03:07Z

+
+source "https://rubygems.org" # rubocop:disable Style/StringLiterals
+
+# gem "rails"


bundle init した時に自動生成されたコードだと思いますが、このプログラムにおいては今後 rails を使うことはないのでこの行は削除しておきましょう。

yuuu · 2024-05-19T20:04:26Z

+
+score_list = ARGV[0].split(',')
+
+total = 0 # 合計得点を入力する変数


各変数の説明コメントですが必要でしょうか？
変数名から用途が想像できるコメントについては削除した方がコードの見通しが良くなります。少なくとも total は削除して問題ないかと。

yuuu · 2024-05-19T20:05:39Z

+# ストライク・スペアの時の加算得点を計算するプログラム
+def cal_addscore(score, flag, running_strike)
+  if flag > 0 # rubocop:disable Style/NumericPredicate
+    add_score = score # 得点を加算


add_score がメソッド外のスコープのものと名前が被っていますね。メソッド外の処理もメソッド化してスコープを分けられないでしょうか？

yuuu · 2024-05-19T20:07:44Z

+  # ストライクだった時の処理
+  if score_s == 'X'
+    flame[0] += 1 # フレームを更新
+    strike_spare_flag = 2


_flag という名前からは true または false という値が類推されますが、実際には 1 2 が格納されていますね。
別の名前を検討しましょう。

また、 1 2 がマジックナンバーになっているので定数を定義するようにしましょう。

strike_spare_flag と pre_strike の役割が被ってそうな点も気になりました 👀

yuuu · 2024-05-19T20:10:33Z

+  if flag > 0 # rubocop:disable Style/NumericPredicate
+    add_score = score # 得点を加算
+    flag -= 1 # フラグの数を減らす
+    add_score += score if running_strike == true


== true は省略可能ですね。

yuuu · 2024-05-19T20:11:28Z

+  else
+    add_score = 0
+  end
+  return add_score, flag, false # rubocop:disable Style/RedundantReturn


rubocop:disable を許容してしまうと、 Rubocopのルールセットを定義している意味が薄れるので、使わない方向で修正をお願いします 🙏

yuuu · 2024-05-19T20:12:50Z

+end
+
+# 得点加算部分の実装
+score_list.each do |score_s|


score_s と score_i のスコープが被っているので、こういった型に依存した変数名にせざるを得なくなっていますが、あらかじめすべて convert_str2int に変換し終えてからスコア計算の処理に移ることで、スコープが被ることを防げないでしょうか？

yuuu · 2024-05-19T20:13:25Z

+  # 得点を合計得点に加算
+  total += add_score + score_i
+
+  # 10フレーム目の処理を別で実装


10フレーム目の処理 が別途あるかのようなコメントですが、実際には存在しないので、コメントの修正をお願いします。

yuuu · 2024-05-22T21:32:55Z

 inherit_gem:
  rubocop-fjord:
-    - "config/rubocop.yml"
+    - "config/rubocop.yml"


細かいですが行末の改行が消えているので元に戻しておきましょう 🙏

yuuu · 2024-05-22T21:33:22Z

@@ -0,0 +1,39 @@
+#!/usr/bin/env ruby # rubocop:disable Style/FrozenStringLiteralComment


ここも rubocop:disable しない方向で修正をお願いします。

yuuu · 2024-05-22T21:35:55Z

+# ストライク・スペアの時の加算得点を計算するプログラム
+def calculate_add_score(flame, score, score_list, ball_number)
+  add_score = 0
+  return add_score if flame[0] == 10


ここはストライク・スペアの加算得点を計算しているわけではないですね。呼び元で「最終フレームでは calculate_add_score を呼ばないように」修正するのはどうでしょうか？

yuuu · 2024-05-22T21:38:05Z

+#!/usr/bin/env ruby # rubocop:disable Style/FrozenStringLiteralComment
+
+total = 0
+flame = [1, 1]


変数名が flame ですが中に入っている値はフレームではありませんね。また、flame[0] と flame[1] が全く関係のない値を保持しているので、なぜここだけ配列変数にしているのかが読み取れませんでした。

別々の変数にして適した名前を付けるようにしましょう。

あと、ボウリングにおけるフレームのスペルは frame ですね。
flame だと炎という意味になってしまいます。

yuuu · 2024-05-22T21:40:40Z

+score_list = score_list.map { |score| score == 'X' ? 10 : score.to_i }
+
+# ストライク・スペアの時の加算得点を計算するプログラム
+def calculate_add_score(flame, score, score_list, ball_number)


score_list にはすべての投球が含まれていますが、このメソッドの処理にすべての投球は不要なはずです。判定に必要な直近3投球のみを渡すようにしてはどうでしょうか？

それができると ball_number も不要になるはずです。

この方法であれば score も渡す必要がないように見えました。

yuuu · 2024-05-22T21:43:16Z

+end
+
+# 得点加算部分の実装
+score_list.each_with_index do |score, ball_number|


以下のように Enumerable#sum を使うと total の宣言や加算が不要になりますね。

score_list.each_with_index do |score, ball_number| # 戻り値を `total` に加算する値にする end

記載がないので憶測ですが、

total = score_list.each_with_index.sum do |score, ball_number| # 省略 score + add_score end

という認識でよろしいですか？

yuuu · 2024-05-25T05:21:23Z

+# ストライク・スペアの時の加算得点を計算
+def calculate_add_score(throw, score_group)
+  add_score = 0
+  add_score += score_group[1] + score_group[2] if throw == 1 && score_group[0] == 10 # ストライク判定


ここは add_score += score_group[1..2].sum とも書けます。参考まで。

yuuu · 2024-05-25T05:22:09Z

+def calculate_add_score(throw, score_group)
+  add_score = 0
+  add_score += score_group[1] + score_group[2] if throw == 1 && score_group[0] == 10 # ストライク判定
+  add_score += score_group[2] if throw == 1 && score_group[0] + score_group[1] == 10 # スペア判定


ここの条件部分も score_group[0..1].sum と書けますね 👍

yuuu · 2024-05-25T05:24:11Z

+
+total = score_list.each_with_index.sum do |score, index|
+  score_group = score_list[index, 3]
+  add_score = if frame == 10


frame == 10 がこのブロックの中で2回登場しています。1箇所にまとめられないでしょうか？

最終フレームの場合はボーナススコアの計算も不要なので、ここでアーリーリターンならぬアーリーnextをすると、以降の行では最終フレームのケースを考慮しなくてよくなりコードの見通しが良くなりそうです 👍

yuuu · 2024-05-25T05:25:26Z

+  # 投球数、フレーム数の更新
+  if frame == 10 # 10フレーム目はそれ以上フレーム数が更新されないので、個別に投球数を進める
+    throw += 1
+  elsif throw == 1 && score == 10


この判定は calculate_add_score の中でも同じ判定をしています。1箇所にまとめられないでしょうか？

こちらは修正後のコードを見てもまだ重複している状況なので、1箇所にまとめられないか検討をお願いします 🙏

yuuu · 2024-05-25T05:27:13Z

+  elsif throw == 1 && score == 10
+    frame += 1 # ストライク処理
+  elsif throw == 1
+    throw = 2


ここは frame += 1 とすると都合が悪いでしょうか？ frame += 1 でいいなら他の条件分岐とまとめられそうに見えます。

yuuu · 2024-05-29T21:39:21Z

+    add_score += score_group[2]
+    throw = 2
+  else
+    frame += 1 if throw == 2


throw == 2 が2回登場するので1回にまとめたいです 🙏

指摘箇所の中にはthrow == 2は2回出てきていないと思います。
恐らく一行下のthrow = (throw == 2 ? 1 : 2)を指しているのだと思うのですが合っていますか？

yuuu · 2024-05-29T21:42:25Z

+score_list = ARGV[0].split(',')
+score_list = score_list.map { |score| score == 'X' ? 10 : score.to_i }
+
+def update_game_state(frame, throw, score_group)


3つの引数を渡して、3つの戻り値を得るのであれば、おそらくそれはメソッドの切り出し方があまりよくないです。「結合度が高く、凝集度が低い」という言い方をします。一旦、ここはメソッドに切り出さずに呼び出し元と統合してはどうでしょう？

その上で、「ストライク判定」や「スペア判定」といった単位で新しいメソッドを切り出してはどうでしょうか？

1mori

確認お願いします。

1mori · 2024-05-30T03:30:31Z

+    add_score += score_group[2]
+    throw = 2
+  else
+    frame += 1 if throw == 2


指摘箇所の中にはthrow == 2は2回出てきていないと思います。
恐らく一行下のthrow = (throw == 2 ? 1 : 2)を指しているのだと思うのですが合っていますか？

1mori added 9 commits May 13, 2024 10:58

add Gemfile

6f336af

スコアを合計するだけのプログラムを追加

6a933e0

ストライク・スペア時に追加得点を記録するコードを作成

13755e2

コマンドライン上の数値変換と得点加算時の挙動をメソッドとして実装

0dd22be

10フレーム目の処理を実装

16af2b2

加算処理を最初に移動させてコードを簡潔化

873dfbc

連続ストライクの対応

9c9584e

合計得点加算処理を一つに統合

c90af82

if文処理を1行に圧縮

d01afb7

yuuu reviewed May 19, 2024

View reviewed changes

1mori added 19 commits May 20, 2024 15:15

Gemfileから不要な記述を消去

d296606

変数名定義の際のコメントを削除

4d6b63c

不要なpメソッドを削除

d642b10

スコープ外のadd_scoreは必要ないので削除

5653e4f

予めボウリングのスコアリストをint型に変換するように変更

65ae1fb

score_listを予めintに変換するようにしたため、score_i, score_sの記載を削除

e734884

== trueを削除

4233449

不要なコメントを削除

408c0c0

int型に変換したscore_listを格納してなかったので、その修正

f5c19da

== trueの記述を消去

a816300

変数pre_scoreを削除

fc36983

スコア点数の処理方法を変更

a36e850

変数pre_strikeを削除

817ea14

変数running_strikeを削除

ac6c000

フレーム数・投球数の処理とストライク処理を統合

1d51126

投球数・フレーム数の書き方を変更

d5c09ff

ストライク・スペア時のスコア加算処理を後から実行していたものを、先に実行するように変更

b13bcc5

必要ないコメントの削除とメソッド名をわかりやすくなるように変更

74d5fc9

加算得点がない際に0を返していたのをadd_score(中身は0)を返すように変更

c7547e6

yuuu reviewed May 22, 2024

View reviewed changes

1mori added 6 commits May 23, 2024 15:14

改行を追加

4220490

frozen_string_literal: trueの項目を追加

8139ac4

flame Arrayをframeとthrowに分割

a3d09f3

calculate_add_scoreの外部でフレーム数を計算するように変更

ea4ca31

Enumerable#sumを使用

b93e2a2

calculate_add_scoreに渡す値を計算に必要な要素に絞ってscore_groupに格納

45be9b5

yuuu reviewed May 25, 2024

View reviewed changes

1mori added 7 commits May 27, 2024 09:57

ストライク・スペア時のスコア加算式を短い表現に変更

3693dd4

.sumのつけ忘れを追加

35bd1af

10フレーム目の処理を短縮し、同一ブロック内でframe == 10の処理を一つにまとめた

3b654fa

フレーム数・投球数の処理を簡潔化

38b5ad4

メソッド名をcalculate_add_scoreからupdate_game_stateに変更

28401e2

フレーム数・投球数の更新をupdate_game_state内で実行するように変更

68ca318

ストライク時はthrow=1なので、ストライク時にthrowを更新する処理が必要ないため削除

8a39b9b

yuuu reviewed May 29, 2024

View reviewed changes

update_game_state内の処理を呼び出し元に戻してメソッドを削除

1c73078

1mori force-pushed the my-bowling branch from 39d8a16 to 1c73078 Compare May 30, 2024 03:20

1mori commented May 30, 2024

View reviewed changes

1mori added 2 commits May 30, 2024 12:32

throw==2の処理が重複していたので一つに統合

66e2630

ストライク・スペア・その他判定の最適化

49a0656


		source "https://rubygems.org" # rubocop:disable Style/StringLiterals

		# gem "rails"


		score_list = ARGV[0].split(',')

		total = 0 # 合計得点を入力する変数

		@@ -0,0 +1,39 @@
		#!/usr/bin/env ruby # rubocop:disable Style/FrozenStringLiteralComment

Conversation

1mori commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1mori May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1mori left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1mori May 23, 2024 •

edited

Loading