クラス間の重複部分を無くす
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; } }
