AddDice::Node: 二項演算子をリファクタリングする

[ochaochaocha3]

除算ノードのクラスを新しく作り、BinaryOpを単純化しました。

除算の処理だけが特別なので、汎用的なBinaryOpにそれを書くのではなく、端数処理方法別に除算ノードのクラスを3つ作り、それらに処理を分担させました。

クラスの継承関係：

BinaryOp（二項演算子）
|
+-- DivideBase（除算の基底クラス）
    |
    |-- DivideWithRoundingUp  （除算 -> 切り上げ）
    |-- DivideWithRoundingOff （除算 -> 四捨五入）
    +-- DivideWithRoundingDown（除算 -> 切り捨て）

[ysakasin]

切り上げ/切り捨て/四捨五入でまでクラスを分けてしまうのは、細分化しすぎてコードの見通しが悪くなっていると思います。除算のクラスだけ作って内部で処理を分岐すれば良いと思ったのですがどうですか。

[ochaochaocha3]

除算のクラスだけ作って内部で処理を分岐すれば良い

今回の目的は、case-when を構文解析時の一箇所にまとめて、種類を表すタイプコード（数値やシンボル）が現れないようにすることです。
具体的には、:roundUp や :roundOff が一切現れないようにすることです。
これがある限り、構文解析時、計算時、表示のための式の生成時など、処理を作るたびに際限なく上記のシンボルかどうかを判定する if-else や case-when を書くことが必要になります。
それを避けるため、サブクラスやポリモーフィズムを活用することがリファクタリングの定石です（以前、テーブルのクラスでも同様に提案いたしました）。

参考：

• ポリモーフィズムとデザインパターン
• ポリモーフィズムによる条件記述の置き換え (リファクタリング-p255)

[ysakasin]

了解しました。ド正論ですね。

以下蛇足
個人的には、Tableクラスの時とは、今後処理が増える可能性がないと言う点で事情が違うので、除算クラスの中に押し込めても十分問題ないとは思っていました。#to_s等で使うPrefixの埋め込みもインスタンス変数で事前に代入しておけば、分岐は少なくできます。

[ysakasin]

SYMBOL だとどうしても Symbol が入っているのを連想してしまうので、何かしら名前を変更した方が良いと思います。

• ROUNDING_METHOD
• ROUNDING_METHOD_PREFIX
• ROUNDING_METHOD_SIGN
• ROUNDING_METHOD_STR
• ROUNDING_METHOD_TYPE
• ROUNDING_TYPE
  等

[ysakasin]

どうせ外部から使わないし、他に良い名前がないならこのままでもいいです。

[ochaochaocha3]

ありがとうございます。それでは ROUNDING_METHOD にしてみます。

[ochaochaocha3]

個人的には、Tableクラスの時とは、今後処理が増える可能性がないと言う点で事情が違うので、除算クラスの中に押し込めても十分問題ないとは思っていました。

確かにそのとおりですね。試しにインスタンス変数版も作ってみたのですが、どちらの方が良いでしょうか？

https://github.com/bcdice/BCDice/compare/add_dice_parser-refactor_binary_op...add_dice_parser-refactor_binary_op-divide_node?expand=1

[codecov-io]

Codecov Report

Merging #162 into add_dice_parser will increase coverage by 0.01%.
The diff coverage is 98.03%.

@@                 Coverage Diff                 @@
##           add_dice_parser     #162      +/-   ##
===================================================
+ Coverage            86.27%   86.28%   +0.01%     
===================================================
  Files                  202      202              
  Lines                21123    21150      +27     
===================================================
+ Hits                 18223    18249      +26     
- Misses                2900     2901       +1     

Impacted Files	Coverage Δ
src/dice/add_dice/node.rb	99.14% <97.61%> (-0.86%)	⬇️
src/dice/add_dice/parser.rb	93.93% <100.00%> (+0.18%)	⬆️

---

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 a24e968...dc8bf1d. Read the comment docs.

[ysakasin]

追加で書かれたインスタンス変数版よりも、当PRのクラス分けされた方が読みやすいと思いました。

ただ、追加のコードを見てちょっと思い浮かんだので、私の方でも書いてみました。こちらどうでしょう。
ee4faa7

[ochaochaocha3]

新しいコード、結局、例えば :roundUp が 'R' に変わって if 文が復活しているので当初の目的が…
Parser#mul で除算のノードを作るときに1回端数処理方法ごとに分岐させて、後はメソッドを呼ぶだけで処理（中置式を作るときの接尾辞も含む）が切り替わるようにしたいです。

[ysakasin]

具体的には、:roundUp や :roundOff が一切現れないようにすることです。

これか……

[ysakasin]

私の書きたいやり方と、ochaさんの書きたいやり方は平行線なんですね。

当PRの変更はど正論なので、現在の差分で行きましょう。
定数名の修正だけお願いします。

[ochaochaocha3]

ノード側でわざわざ定数を作っているのには、端数処理方法の記号とノード側での表現とを独立させておきたいという意図もあります。
例えば、10/3U という式ならば、まだ端数処理の記号が 'U'で「Up（切り上げ）のUかな」と見当がつきますが、10/3 で端数処理の記号が空文字列だと、「空？ デフォルト？ 具体的には何？」となり得ます。
サブクラス（Template Methodパターン）でも、端数処理だけ独立したオブジェクトにする（Strategyパターン）でも良いのですが、端数処理の手続きとそれを表す記号をひとまとめにして、まとめたクラス/オブジェクトに具体的で不変の名前をつけておきたいのです。

[ysakasin]

了解です。現行PRの案で行きましょう