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

Rerollダイスのリファクタリングと振り足し条件指定の仕様変更 #217

Merged
merged 6 commits into from
Jul 2, 2020

Conversation

ysakasin
Copy link
Member

@ysakasin ysakasin commented Jun 12, 2020

やったこと

  • Rerollダイスのリファクタリング
  • 振り足しの条件の演算子を指定できるように

振り足し条件について

従来だと振り足しの判定に用いる演算子に成功条件で使う演算子を用いるような書き方がされていた。無限ループ防止のチェックでそれがみられた。しかし、実際には振り足しの個数の判定は BCDice#roll に移譲され、 >= でのみ振り足しの判定がされていた。

当PRではそれを修正し、振り足し判定の演算子を成功条件とは独立に指定できるようにし、指定されていない場合には成功判定に用いる演算子を流用するようにした。

変更後の仕様

コードに記載したドキュメントが以下の通り

# 個数振り足しダイス
#
# ダイスを振り、条件を満たした出目の個数だけダイスを振り足す。振り足しがなくなるまでこれを繰り返す。
# 成功条件を満たす出目の個数を調べ、成功数を表示する。
#
# 例
#   2R6+1R10[>3]>=5
#   2R6+1R10>=5@>3
#
# 振り足し条件は角カッコかコマンド末尾の @ で指定する。
# [>3] の場合、3より大きい出目が出たら振り足す。
# [3] のように数値のみ指定されている場合、成功条件の比較演算子を流用する。
# 上記の例の時、出目が
#   "2R6"  -> [5,6] [5,4] [1,3]
#   "1R10" -> [9] [1]
# だとすると、 >=5 に該当するダイスは5つなので成功数5となる。
#
# 成功条件が書かれていない場合、成功数0として扱う。
# 振り足し条件が数値のみ指定されている場合、比較演算子は >= が指定されたとして振舞う。

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2020

Codecov Report

Merging #217 into master will increase coverage by 0.02%.
The diff coverage is 95.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   87.58%   87.60%   +0.02%     
==========================================
  Files         216      216              
  Lines       22306    22310       +4     
==========================================
+ Hits        19536    19544       +8     
+ Misses       2770     2766       -4     
Impacted Files Coverage Δ
src/dice/RerollDice.rb 94.18% <95.71%> (+5.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0217b5...0474d31. Read the comment docs.

@ysakasin ysakasin force-pushed the refactor_reroll_dice branch from 880aafa to 0474d31 Compare June 12, 2020 12:49
@ysakasin ysakasin added the refactoring 内部構造の改良 label Jun 24, 2020
@ysakasin ysakasin force-pushed the refactor_reroll_dice branch from 0e5887e to 05b01b5 Compare June 30, 2020 18:33
[skip ci]
@ysakasin ysakasin added the bug バグってる! label Jun 30, 2020
@ysakasin ysakasin marked this pull request as ready for review June 30, 2020 18:42
@ysakasin ysakasin requested a review from ochaochaocha3 June 30, 2020 18:43
@ysakasin ysakasin changed the title Rerollダイスのリファクタリング Rerollダイスのリファクタリングと振り足し条件指定の仕様変更 Jun 30, 2020
Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

処理は問題ないと思います。細かい点ですが、確認よろしくお願いします。

# @param target_number [Integer, nil]
# @return [Integer]
# @return [nil]
def decide_reroll_threthold(captured_threthold, target_number)
Copy link
Member

Choose a reason for hiding this comment

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

スペルミスでした。

Suggested change
def decide_reroll_threthold(captured_threthold, target_number)
def decide_reroll_threshold(captured_threshold, target_number)

Comment on lines 130 to 137
op =
if m[2] && m[3]
m[2]
elsif m[6] && m[7]
m[6]
else
m[4]
end
Copy link
Member

Choose a reason for hiding this comment

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

正規表現から離れた位置にあるので、マッチデータの番号と部分の対応が分かりにくく感じました。一度変数に受けて名前をつけるか、あるいは単にparseメソッドの中に入れてしまっても良いかもしれません。

Copy link
Member Author

Choose a reason for hiding this comment

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

メソッド内で一度変数に受ける方式で改修しました。

@ochaochaocha3
Copy link
Member

グリッチのメッセージから " > " を取り除く処理については、シャドウラン4th/5thのみと影響範囲が少ないため、ダイスボット側から " > " を取り除いても良いかもしれません。他に呼び出している箇所はバラバラロールの1行のみなので、全体への影響も少ないです。

@ysakasin
Copy link
Member Author

ysakasin commented Jul 1, 2020

ダイスボット側から " > " を取り除いても良いかもしれません

このRPはRerollDiceで完結させたくて、あまり別の基本ダイスに手を広げたくないので今回はこのままで行こうと思います。
バラバラロールを直すタイミングがあったらその時に直します。

@ysakasin ysakasin requested a review from ochaochaocha3 July 1, 2020 17:39
@ochaochaocha3
Copy link
Member

ochaochaocha3 commented Jul 2, 2020

このRPはRerollDiceで完結させたくて、あまり別の基本ダイスに手を広げたくないので今回はこのままで行こうと思います。
バラバラロールを直すタイミングがあったらその時に直します。

了解です!

@ysakasin ysakasin merged commit 1b16d6e into master Jul 2, 2020
@ysakasin ysakasin deleted the refactor_reroll_dice branch July 2, 2020 15:49
@ysakasin
Copy link
Member Author

ysakasin commented Jul 2, 2020

@ochaochaocha3 いつもレビューありがとうございます

ysakasin added a commit that referenced this pull request Aug 30, 2020
Rerollダイスのリファクタリングと振り足し条件指定の仕様変更
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug バグってる! refactoring 内部構造の改良
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants