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

フィルトウィズ:ナンバーワンくじ、マジカルクッキングをRangeTableで表す #324

Merged
merged 3 commits into from
Dec 13, 2020

Conversation

ochaochaocha3
Copy link
Member

フィルトウィズの #311 と被らない範囲のリファクタリングです。ナンバーワンくじとマジカルクッキングの表をRangeTableで表しました。その際、本体のファイルが約1500行と非常に長かったため、別ファイルに分けました。取り出した項目によって他の表を再度引くという処理が同じだったため、roll_jump_table というメソッドを作って共通化しました。

@ochaochaocha3 ochaochaocha3 added the refactoring 内部構造の改良 label Dec 13, 2020
@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #324 (f1b73e9) into master (ee47c24) will increase coverage by 0.14%.
The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   93.38%   93.53%   +0.14%     
==========================================
  Files         255      257       +2     
  Lines       18358    18318      -40     
==========================================
- Hits        17143    17133      -10     
+ Misses       1215     1185      -30     
Impacted Files Coverage Δ
lib/bcdice/game_system/FilledWith.rb 64.51% <94.44%> (+1.55%) ⬆️
lib/bcdice/game_system/filled_with/lot_tables.rb 94.73% <94.73%> (ø)
lib/bcdice/game_system/filled_with/cook_tables.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 ee47c24...f1b73e9. Read the comment docs.

@ysakasin
Copy link
Member

RangeTableを流用するのではなく、この件用にクラスを作ってしまうのが王道かと思うのですが、それだとやりづらいでしょうか?

@ochaochaocha3
Copy link
Member Author

当初新しいクラスを作りかけ、最終的にRangeTableと関数1つになりクラスにするまでもないように感じたのでこれに落ち着きました。クラスにする場合、どのデータをメンバとして持たせるのを想定しているでしょうか?

@ysakasin
Copy link
Member

メンバ粒度は現在と同じ程度のものを考えていました。
ファイル分割というメインの目的は果たしているので大丈夫そうですね。

@ysakasin
Copy link
Member

ただ、lamdaでわざわざ遅延評価しているのは結構トリッキーかとも思うので、より良い方法が思いついたらあとでリファクタリングお願いします。

@ysakasin ysakasin merged commit b0feed4 into master Dec 13, 2020
@ysakasin ysakasin deleted the filled_with-jump_table branch December 13, 2020 10:59
@ochaochaocha3
Copy link
Member Author

lambda での遅延評価はもともと入っていまして、当初はできれば避けようとしていたのですが、簡潔さの面でこれに落ち着いてしまいました。lambda を避けるとなると、次に引く表とレベルを保持する配列/構造体あたりが必要なのですが、書く場所の都合上、表の名前をそのまま書くのが難しかった(未定義で NameError になりやすかった)です。一旦文字列やSymbolに変えると、どこかで分岐を別に書かなければならず長くなりそうでした。

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