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

Refactor BCDice#check_suc #145

Merged
merged 5 commits into from
Apr 2, 2020
Merged

Refactor BCDice#check_suc #145

merged 5 commits into from
Apr 2, 2020

Conversation

ysakasin
Copy link
Member

@ysakasin ysakasin commented Mar 29, 2020

BCDice#check_suc 関連のメソッドが遠回しな処理を多くするため、リファクタリングを実施した。 結果、不要なメソッドとインスタンス変数を削除した。

コア機能の変更は 6bdb593 に、その変更に伴う各ダイスボットの修正は 0180772 にまとめています。

変更点

6bdb593

  • DiceBot#check_suc に移動
  • 引数の修正と厳格化
  • DiceBot#getDiceList, @diceText, @diffText を廃止

0180772

  • 各ダイスボットの対応
  • GranCrest: 目標値?の挙動修正
  • RuneQuest: 成功度の範囲を確認するテストを追加

@ysakasin ysakasin added the refactoring 内部構造の改良 label Mar 29, 2020
@codecov-io
Copy link

codecov-io commented Mar 29, 2020

Codecov Report

Merging #145 into master will increase coverage by 0.05%.
The diff coverage is 94.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   86.35%   86.41%   +0.05%     
==========================================
  Files         197      201       +4     
  Lines       22249    22372     +123     
==========================================
+ Hits        19214    19332     +118     
- Misses       3035     3040       +5     
Impacted Files Coverage Δ
src/bcdiceCore.rb 70.52% <ø> (-1.75%) ⬇️
src/diceBot/ArsMagica.rb 75.26% <0.00%> (ø)
src/diceBot/NightWizard.rb 92.15% <0.00%> (-0.37%) ⬇️
src/diceBot/NightmareHunterDeep.rb 75.00% <55.55%> (ø)
src/diceBot/ChaosFlare.rb 86.95% <70.00%> (-4.35%) ⬇️
src/diceBot/InfiniteFantasia.rb 76.00% <70.00%> (-1.28%) ⬇️
src/diceBot/Hieizan.rb 78.94% <71.42%> (ø)
src/diceBot/TokumeiTenkousei.rb 95.65% <75.00%> (-4.35%) ⬇️
src/diceBot/ShinobiGami.rb 95.91% <77.77%> (ø)
src/diceBot/DoubleCross.rb 97.03% <85.71%> (ø)
... and 69 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 2f4153f...bdd9b90. Read the comment docs.

@ysakasin ysakasin force-pushed the refactor_check_suc branch from 4773d4c to 199b119 Compare March 29, 2020 12:53
@ysakasin ysakasin changed the title [WIP] Refactor BCDice#check_suc Refactor BCDice#check_suc Mar 29, 2020
@ysakasin ysakasin changed the title Refactor BCDice#check_suc [WIP] Refactor BCDice#check_suc Mar 29, 2020
- DiceBot#check_suc に移動
- パラメータを厳格化
- DiceBot#getDiceList, @diceText, @diffText を廃止
- GranCrest: 目標値?の挙動修正
- RuneQuest: 成功度の範囲を確認するテストを追加
@ysakasin ysakasin force-pushed the refactor_check_suc branch from 199b119 to 0180772 Compare March 30, 2020 03:52
@ysakasin ysakasin changed the title [WIP] Refactor BCDice#check_suc Refactor BCDice#check_suc Mar 30, 2020
@ysakasin ysakasin requested a review from ochaochaocha3 March 30, 2020 04:21
@ochaochaocha3
Copy link
Member

ocha今日 22:26
check_suc というメソッド、多分「成功判定を行う」ということだと思うのですが、もう少し分かりやすい英語がないかなぁ
ysakasin今日 22:30
👀 https://www.chaosium.com/content/FreePDFs/RuneQuest/CHA4027%20-%20RuneQuest%20Quickstart.pdf
RuneQuestの場合ですが、判定結果そのもののことは Result で、ダイスロールのことは XXX Checkといっているみたいですね
ocha今日 22:32
なるほど
ysakasin今日 22:32
Ruby的には #result_text とかが妥当なのかなぁ
ocha今日 22:33
結果の文章については、それが良さそう
(中略)
動詞にすると、「check」とか「test」とか一単語で抽象的な名前になりそうだから、check_result_text あたりですかねぇ
ysakasin今日 22:42
👀 https://media.wizards.com/2018/dnd/downloads/DnD_BasicRules_2018.pdf
#check_result の方が良いかも
ocha今日 22:46
そうしますか
ysakasin今日 22:47
#check_suc -> #check_result
としてみるとあんまり変わってない気も……
ocha今日 22:48
suc が3文字で、ひと目ではよくわからず
ysakasin今日 22:48
なるほど
ocha今日 22:49
check に「判定」の意味があれば、「判定結果」で意味は通じそうかな、と

src/utils/normalizer.rb Outdated Show resolved Hide resolved
src/utils/normalizer.rb Outdated Show resolved Hide resolved
src/utils/normalizer.rb Outdated Show resolved Hide resolved
@ochaochaocha3
Copy link
Member

Discordの会話で出てきた、Normalize.comparison_operator が良さそうでした(長いので、6456840c820943991946539600a23bfa8a3fd84a と合わせて Normalize.comparison_op あたりがちょうど良いかも)。モジュール名を「Normalize」に変更するだけなので、目標値の方は Normalize.target_number になります。

@ochaochaocha3
Copy link
Member

Normalizerモジュールの名前以外の部分は、これで大丈夫だと思います。

@ysakasin
Copy link
Member Author

ysakasin commented Apr 1, 2020

Normalize.comparison_op だと一部だけ省略することになり、違和感をかんじたため略さずにNormalize.comparison_operator としました。

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 8e70b47 into master Apr 2, 2020
@ysakasin ysakasin deleted the refactor_check_suc branch April 2, 2020 12:47
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.

3 participants