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

新クトゥルフ神話TRPGでTens D10を先に振るようにする #267

Merged
merged 3 commits into from
Aug 27, 2020

Conversation

ysakasin
Copy link
Member

ボーナスダイス処理時に今までは一の位を先にダイスロールしていた。
しかし、結果ダイスを画像で表示させた時に十の位のダイスが先にある方が視認しやすいため、Tens D10を先に実行する。

結果ダイスを画像で表示させた時に十の位のダイスが先にある方が
視認しやすいため、Tens D10を先に実行する。
@ysakasin ysakasin requested a review from ochaochaocha3 August 22, 2020 14:24
@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #267 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
- Coverage   87.88%   87.88%   -0.01%     
==========================================
  Files         225      225              
  Lines       22905    22902       -3     
==========================================
- Hits        20131    20128       -3     
  Misses       2774     2774              
Impacted Files Coverage Δ
src/diceBot/Cthulhu7th.rb 99.59% <100.00%> (-0.01%) ⬇️
src/test/test_detailed_rand_results.rb 100.00% <100.00%> (ø)

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 e5d5773...5e20d84. Read the comment docs.

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.

処理内容は問題ないと思います。細かい点ですが、ご確認お願いします。

Comment on lines 151 to 152
dice_list = tens_list.map { |tens| tens + ones }
dice_list.map! { |dice| dice == 0 ? 100 : dice }
Copy link
Member

Choose a reason for hiding this comment

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

連続で map しているので、ひとつにまとめられないでしょうか?

Suggested change
dice_list = tens_list.map { |tens| tens + ones }
dice_list.map! { |dice| dice == 0 ? 100 : dice }
dice_list = tens_list.map { |tens|
dice = tens + ones
dice == 0 ? 100 : dice
}

Copy link
Member Author

Choose a reason for hiding this comment

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

mapの中で複数のことをやると読みづらいだろう思って分割していましたが、大丈夫そうなのでまとめました

@ysakasin ysakasin requested a review from ochaochaocha3 August 23, 2020 05:23
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.

LGTM!

@ysakasin ysakasin merged commit caacc10 into master Aug 27, 2020
@ysakasin ysakasin deleted the coc7th_roll_tens_d10_first branch August 27, 2020 03:37
ysakasin added a commit that referenced this pull request Aug 30, 2020
新クトゥルフ神話TRPGでTens D10を先に振るようにする
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