コードの凝集性が重要だということを布教しているのですが、先日開発中のレビューで残念なコードを見て驚いたのでメモ。
別にこれが正解!というものではなく、個人的にはこういうコードが好き。という程度のものです。
レビューで見た残念なコード
該当のコードは受け取ったJSessionIDのCookieを有効期限れにして返すというものだったのですが、以下のようになっていました。抜粋して書きます。
public class CookieLogic {
public Cookie deleteJsessionCookie(){
Cookie cookie = new Cookie("JSESSIONID","delete");
cookie.setDomain("/");
cookie.setPath("/");
cookie.setMaxage(0);
cookie.setSecure(true);
cookie.setHttponly(true);
}
}
で、これを使う側は
DeleteCookieLogic logic = new DeleteCookieLogic();
Cookie cookie = logic.deleteJsessionCookie();
response.addCookie(cookie);
こんな感じになっていました。
そしてこのシステムでは複数のCookieを生成/削除する機能があります。
例えば、上記のJSESSIONIDはアプリの内部で生成して返すこともありますし、削除することもあります。
操作対象となるCookieは複数あり、イメージを掴むために若干変えて以下のように例示します。
- JSESSIONID:生成・削除
- Cookie1:生成・削除
- Cookie2:生成
- Cookie3:削除
- Cookie4:生成・削除
つまりこれらのCookieを操作するためのメソッドが、上記1クラス内にcreateXXXCookie()およびdeleteXXXCookie()というメソッドで定義されているということになります。
さらに、CookieによってMaxAgeやSecure、Domainなどの値も異なりますから、全てのロジックは共通化できないという状況にあります。
こんな感じ。
何が残念か
個人的には以下のポイントを大変残念に思います。
- Cookieの生成はいちいちロジック化するほどのコードではない。さらに言えば、動的に変更するものでもない。
- 上記例で生成する全てのCookieがCookie型になっており、型安全を確保できない。どのCookieを操作したいのかをメソッド名だけで判断しなければならない。
上記の例では登場しなかったですが、Cookie2だけを受け取って処理したいようなメソッドを作った場合、Cookie型を作ってしまうと5つのCookie全てを引数に受けつけられてしまう。
クラス図の例では、SampleLogic#method1では、CookieLogicが作った全てのCookieを受け付けられる。 - どのCookieをどのように生成して、どのように削除するかという情報が全てロジッククラスに流出しており、全てフラットに扱われている。
Cookie1とJSESSIONIDは一切の関連がないのに、同じよう階層で処理が記述されている。 - あるCookieのルールを変えようとした場合、同じロジッククラスに処理が書かれているため影響を受ける可能性がある。(=修正後にコンパイルしたクラスと修正前にコンパイルしたクラスでは、クラスごと差分として現れてしまうため、影響の有無が確認できない)
処方箋
上記の例で登場した各種Cookieはお互いに干渉し合うものではありません。つまり、JSESSIONIDクラスとCookie1は一切無関係ですから、同じロジッククラス内で扱うのは適切ではありません。
また、上記程度の処理であればいちいちロジック化しなくても良さそうです。
まとめると主に以下を達成したい。
- お互いが干渉しないものの処理は別クラスに分けておいた方が凝集性や拡張性が向上する。
- お互いが干渉しないものはクラスを分けておいた方が型安全を確保しやすい。
- 値を設定するだけのものはロジックというよりもユーティリティ的な使い方で十分。
修正例
以下のようにしてみるのはいかがでしょうか。
JSESSIONIDという固有のクラスを用意しました。このクラスはCookieクラスを継承しており、振る舞いとしてはCookieクラスと同じです。
値の設定などをロジック化せず、JSESSIONIDという固有のクラス内に全て閉じ込めました。
public class JSESSIONID extends Cookie {
//外部からのnewを禁止し、staticメソッド経由のみでインスタンスを生成する
private JSESSIONID(String name, String sid) {
super(name, sid);
}
//staticメソッドとして宣言
//別に不要であれば、引数sidもなくていい。任意のsidをつけたかったので追加した。
public static JSESSIONID newCookie(String sid) {
//Cookie名の"JSESSIONID"は外部から指定不要。クラス名がどのCookieかを語ってくれる。
JSESSIONID cookie = new JSESSIONID("JSESSIONID", sid);
//JSESSIONID生成時のルールはstaticメソッド内飲みに記述し、外部には見せない。
cookie.setDomain("/");
cookie.setPath("/");
cookie.setSecure(false);
cookie.setHttpOnly(true);
cookie.setMaxAge(10000);
return cookie;
}
//deleteの場合も同様
public static JSESSIONID deleteCookie(String sid) {
JSESSIONID cookie = new JSESSIONID("JSESSIONID", sid);
cookie.setDomain("/");
cookie.setPath("/");
cookie.setSecure(false);
cookie.setHttpOnly(true);
cookie.setMaxAge(0);
return cookie;
}
}
こんな感じで描き直すと、クラス図は以下のようになります。
注目して欲しいところを赤丸で囲みました。
ポイントは、
- 全てのCookieクラスは継承によって個別のクラスとし、生成や削除は各クラス内部でメソッド化した。
- SampleLogicでは、本当に使用したい型だけを指定できるようになり、意図しないCookieを渡されることがなくなった。
- (コード例では出てきましたが)メソッドをstatic化し、外部からのインスタンス生成を禁じたことにより、各Cookieのルールを1箇所に集めた。
これにより、あるCookieの生成や削除のルール変更が別のCookieに影響を及ぼすことがなくなりました。SOLID原則の中のオープン・クローズの原則も達成することができましたね。
あるCookieのルールを追加したい場合は、該当のクラスのみをいじればOKです。