クラス間の重複部分を無くす
CheckDependentsRule
とCheckStateRule
という2つのルールには、類似点がいくつかあるように見えます。eval
メソッドのブロックが同じですし、いくつかのサポートメソッドも同じであるように見えます。この2つのクラスの間での重複をなくしたいと思います。そうすれば、リスクと将来の保守コストが低減されるはずです。
委譲と継承のどちらを使用すべきでしょうか。委譲ならそれほど強い依存関係を生じさせないので、通常であれば委譲の検討から始めたいところです。共通の構造とロジックを再使用するときには委譲が理想的です。
しかし、このケースでは2つのクラスの動作が関連しています。2つとも目標と動作が同じです。そこで、BaseRule
という共通のクラスを作成し、共通の機能をこのクラスに徐々に移していくことにします。
最初のステップとして、空のスーパークラスを作成し、両方のRule
クラスがこのクラスを継承するようにします。当然ながら、この段階で一度テストしておきます。
public class BaseRule { } public class CheckStateRule extends BaseRule implements Rule { ... public class CheckDependentsRule extends BaseRule implements Rule { ...
CheckDependentsRule
クラスを見ると、try
ブロック内の最後の部分が、CheckStateRule
クラスのswitchReturnValues
メソッド内の機能と重複しているように見えます。
boolean returnVal1 = returnVal; if (!returnTrue) returnVal1 = !returnVal1; String retErrStr = null; if (returnVal1 == false) { if (returnTrue) retErrStr = "Dependent info incomplete"; else retErrStr = "All dependent info provided"; } List<Object> res = new ArrayList<Object>(); res.add(new Boolean(returnVal1)); res.add(retErrStr);
このコードを抜き出して、CheckStateRule
クラスのものと同じ名前を持つ独立したメソッドにします。
// return value List<Object> returns = switchReturnValues(returnTrue, returnVal, "Dependent info incomplete", "All dependent info provided"); ... } private List<Object> switchReturnValues(boolean returnTrue, boolean returnVal, String trueMsg, String falseMsg) { boolean returnVal1 = returnVal; if (!returnTrue) returnVal1 = !returnVal1; String retErrStr = null; if (returnVal1 == false) { if (returnTrue) retErrStr = trueMsg; else retErrStr = falseMsg; } List<Object> res = new ArrayList<Object>(); res.add(new Boolean(returnVal1)); res.add(retErrStr); return res; }
この時点で、2つのswitchReturnValues
メソッドを見て、両者のロジックが本当に同じかどうか確かめることができます。よい単体テストが揃っているので、この比較にはわずかな時間しかかかりません。この重複を取り除けば、求めている答えになります。そこで、このうちの一方をBaseRule
スーパークラスに移し(その後でテストを実行し)、もう一方を除去します(そして再びテストを実行します)。
import java.util.*; public class BaseRule { protected List<Object> switchReturnValues(boolean returnTrue, boolean returnVal, String trueMsg, String falseMsg) { boolean returnVal1 = returnVal; if (!returnTrue) returnVal1 = !returnVal1; String retErrStr = null; if (returnVal1 == false) { if (returnTrue) retErrStr = trueMsg; else retErrStr = falseMsg; } List<Object> res = new ArrayList<Object>(); res.add(new Boolean(returnVal1)); res.add(retErrStr); return res; } }
多くの場合、重複を取り除くためには、まず双方の見た目を同じにすることが必要になります。どちらのルールクラスにもhasMissingData
フラグがあります。エラーメッセージを格納するらしいString
フィールドもありますが、名前は異なっています。
そこで、CheckStateRule
内のフィールド名をerrStr
からerrorMessage
に変更します(そしてテストを実行します)。また、CheckStateRule
クラスで両方のフィールドをprivate
にします。CheckDependentsRule
クラスでは両方ともprivate
になっているからです。そしてテストを実行します。hasMissingData
/missingData
フィールドについても同じことをします。
見た目を同じにするという準備作業が終わったら、フィールドとアクセサ(ゲッター)メソッドをBaseRule
クラスに移します。そして、これらのフィールドを、private
ではなくprotected
にします。この決定は疑問の余地があるかもしれませんが、現在のリファクタリングの残りの部分を完了させてから訂正することもできます。まだテストが通るか検証します。
もう1つ、単純にできるリファクタリングがあります。両方のクラスでEXCEPT
定数を定義し、この定義をスーパークラスに移し、両方のサブクラスから削除します。
さて、これからが面白いところです。eval
メソッドはまだ少し長めです。しかし、CheckStateRule
クラス内のeval
メソッドの構造を見ると、次のポリシーがあることに気づきます。
- 引数値を抽出する
- フォームデータを抽出する
- ルールを実行する
- 戻り値を準備し、もし必要なら否定する
- 上の3つのステップからトラップした例外を、ルール障害として処理する
CheckDependentsRule
クラスにもよく似た構造がありますが、引数値を抽出しない点が異なります。まずCheckStateRule
クラスに手を入れて、この機能を上記のポリシーに沿ったメソッド呼び出しに分割します。
import java.util.*; /** * * Check if the state the case is located in * is equal to argument passed in. */ public class CheckStateRule extends BaseRule implements Rule { private String argVal; private String formState; public boolean eval(Case c, List<String> val, boolean returnTrue) { try { return evalWithThrow(c, val, returnTrue); } catch (Exception e) { errorMessage = BaseRule.EXCEPT + e.getMessage(); hasMissingData = true; return false; } } private boolean evalWithThrow(Case c, List<String> val, boolean returnTrue) { if (!extractArgs(val)) return false; if (!extractFormData(c)) return false; boolean returnVal = executeRule(c); return prepareReturnValues(returnTrue, returnVal); } private boolean extractFormData(Case c) { // get the location object off the form and get the state the // case resides in Location loc = (Location)c.getFormComposite("location"); if (loc == null) { errorMessage = "Cannot determine the state this case is located in."; hasMissingData = true; return false; } String st = loc.getAttribute("state"); formState = (st == null ? null : st.toUpperCase()); if (formState == null) { errorMessage = "Cannot determine the state this case is located in."; hasMissingData = true; return false; } return true; } private boolean prepareReturnValues(boolean returnTrue, boolean returnVal) { List<Object> returns = switchReturnValues(returnTrue, returnVal, "Case does not reside in '" + argVal + "'", "Case resides in '" + argVal + "'"); returnVal = ((Boolean)returns.get(0)).booleanValue(); errorMessage = (String)returns.get(1); return returnVal; } private boolean executeRule(Case c) { return formState.equals(argVal); } private boolean extractArgs(List<String> val) { if (val == null || val.size() != 1) { errorMessage = "Can't convert value list to a state"; hasMissingData = true; return false; } argVal = (String)val.get(0); return true; } }