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

ナイトウィザードのリファクタリング #252

Merged
merged 7 commits into from
Aug 10, 2020

Conversation

ysakasin
Copy link
Member

@ysakasin ysakasin commented Aug 3, 2020

やったこと

  • DiceBot#changeText での変換をしない
  • パーサーメソッドを用意して構造体で返す
  • NWコマンド使用時には、受け入れたコマンドをNW形式で表示する
  • 処理の単純化
  • 2R6形式のテストを追加

追加機能のための布石としてのリファクタリング

NWコマンドの仕様概要

  • 2D6の合計がクリティカル候補リストに含まれる時、クリティカルとなる
    • @6,7,9 のように指定する
    • ファンブルも同様に #3,5
  • ファンブルのとき、常時発動ではない(能動的な)効果による修正値は適用されない。
    • 「ナイトウィザード The 3rd Edition」ではファンブルでも適用される

@ysakasin ysakasin added the refactoring 内部構造の改良 label Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #252 into master will increase coverage by 0.14%.
The diff coverage is 98.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
+ Coverage   87.86%   88.01%   +0.14%     
==========================================
  Files         220      224       +4     
  Lines       22942    23270     +328     
==========================================
+ Hits        20158    20481     +323     
- Misses       2784     2789       +5     
Impacted Files Coverage Δ
src/diceBot/DiceBotLoaderList.rb 100.00% <ø> (ø)
src/diceBot/Fiasco_Korean.rb 96.55% <96.55%> (ø)
src/diceBot/GoblinSlayer.rb 97.05% <97.05%> (ø)
src/diceBot/StratoShout_Korean.rb 97.72% <97.72%> (ø)
src/diceBot/NightWizard.rb 98.09% <97.91%> (+1.97%) ⬆️
src/diceBot/DoubleCross_Korean.rb 99.08% <99.08%> (ø)
src/bcdiceCore.rb 81.60% <100.00%> (ø)
src/configBcDice.rb 100.00% <100.00%> (ø)
src/diceBot/Nechronica.rb 97.59% <100.00%> (+0.12%) ⬆️
src/diceBot/NightWizard3rd.rb 100.00% <100.00%> (ø)
... and 7 more

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 c830596...70d3a0e. Read the comment docs.

@ysakasin ysakasin requested a review from ochaochaocha3 August 4, 2020 13:56
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 number [Integer]
# @return [String]
def modify_number(number)
Copy link
Member

Choose a reason for hiding this comment

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

英語では一単語のmodifierの方が自然だと思います。

Terms used to play role-playing games

  • Modifier: A number added to or subtracted from a die roll.

https://en.wikipedia.org/wiki/Role-playing_game_terms#M

Copy link
Member Author

Choose a reason for hiding this comment

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

修正しました。

# @return [Symbol, nil] 比較演算子
# @!attribute target_number
# @return [Integer, nil] 目標値
attr_accessor :critical_numbers, :fumble_numbers, :active_modify_number, :cmp_op, :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.

このままで動作上は問題ないですが、@!attributeやむを得ない場合(例えばメタプログラミングで動的にメソッドが定義されるとき)のみ使用するように書かれているので、単純に以下のように書くのはいかがでしょうか。重複もなくなります。

# @return [Array<Integer>] クリティカルになる出目の一覧
attr_accessor :critical_numbers

# @return [Array<Integer>] ファンブルになる出目の一覧
attr_accessor :fumble_numbers

# ...

# @return [Integer, nil] 目標値
attr_accessor :target_number

Copy link
Member Author

Choose a reason for hiding this comment

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

@return が使えたんですね!

この記法でかけるならこちらの方が読みやすいので修正しました。

src/diceBot/NightWizard.rb Outdated Show resolved Hide resolved
@ysakasin ysakasin requested a review from ochaochaocha3 August 10, 2020 09:38
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 713dc2b into master Aug 10, 2020
@ysakasin ysakasin deleted the refactor_NightWizard branch August 10, 2020 13:54
ysakasin added a commit that referenced this pull request Aug 30, 2020
ナイトウィザードのリファクタリング
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring 内部構造の改良
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants