Fat Controller改善ガイド
はじめに
「テストコードがなくて品質が良くないんですよ…」というシステムのソースコードを覗くと、いろんなロジックがコントローラの中に書かれたファットコントローラだったり、ビジネスロジック層と定義はされているけれども中身はただのDAOだったりということが多くあります。
そういうファットコントローラだと書けるテストは、以下のように入力値のバリデーションエラー1つテストするだけでも、コントローラの依存関係を解決し呼び出す必要があり、テストコード書くにもモックライブラリの知識が要求されるようになるし、書いたテストも実行速度が遅くなりがちです。さらにアサーション対象はコントローラの戻り値、すなわちテンプレートに渡す変数であったり、JSON化するためのオブジェクトで、この例のようにバリデーションエラーの場合は、ユーザに表示するためのメッセージ文しかないこともあり、アサーション書くのも一苦労だし、メッセージを修正するたびにテストも修正することになってしまいます。
code:java
@Inject
ArticlePostController sat;
void badName() {
ArticleRequest articleRequest = new ArticleRequest();
articleRequest.setName("\\");
// ... 他の項目がエラーにならないように、name以外のプロパティもセットする ...
HttpResponse response = sat.post(articleRequest);
assertThat(response.getErrors()).contains(new Messages("E0001", "名前が不正です"));
}
そうなると、ファットコントローラに対してテストを書くコストメリットが少なくなってしまいます。
こういう状態ではテストを書く前にやるべきことは、テスト可能なコードにしていくリストラクチャリングです。
Validationを切り出す
ファットコントローラの中にはフレームワークの機能を使わずにバリデーションが書かれたものがあります。
これをフレームワークの機能を使ってバリデーションするように変更し、テストを書きます。
code:Before.java
if (param.getName() == null || param.getName().matches("^a-zA-Z$" || params.getName().length() > 100) { errors.add(new Message("E0001", "名前が不正です");
}
code:After.java
class ArticleRequest {
@Length(100)
@NotBlank
private String name;
}
テスト観点の変更
宣言的にバリデーション定義を書けるフレームワークでは、あまり正常/異常の入力値を適当に作ってケースを書くExapmle Based Testingの効果は薄れ、その定義をしっかりレビューする方が費用対効果が高くなります。
ただこのバリデーションを通過して、その入力値を正常値として期待して動く後続の処理と仕様認識のズレがあった場合にそれを検知できないので、Property Based Testingを併用するとよいです。
ファサードを導入する
コントローラが以下の状態であれば、ファサードの導入ポイントです。
依存するクラスが多い
依存クラスの呼び出しを決まった順番で呼び出す一連のコードが存在する。
上記のケースが、いろんな箇所に散見される
一連の決まった呼び出しは、いわゆるTemporal Cohesionで、凝集度があまり高くありません。Pragmatic Programmerで言われている、Tell Don't Askの原則にも反しており、呼び出し方の変更が様々なコントローラに波及することになります。
code:Before.java
for (OrderLine line : order.getOrderLines()) {
wishlistRepository.delete(orderLine.getProduct());
}
orderRepository.save(order);
cartRepository.delete(session.getAttribute("user"));
この例では、カートから注文を生成し、注文した商品はウィッシュリストから削除し、カートをクリアする、という一連の呼び出しがいろんなRepositoryクラスに依存して、処理を実現しています。これをCheckOutServiceのファサードを作り、そちらを呼ぶようにします。
code:After.java
checkoutService.checkout(cart);
code:CheckOutService.java
class CheckOutService {
void checkout(Cart cart) {
Order order = Order.createFromCart(cart);
for (OrderLine line : order.getOrderLines()) {
wishlistRepository.delete(orderLine.getProduct());
}
orderRepository.save(order);
cartRepository.delete(cart.getUser());
}
}
このとき、同じ一連の呼び出しをしている箇所は、すべてファサードを経由するようにしておくことが重要です。チェックアウトの処理が変わった場合に、変更箇所をこのファサードに閉じ込めるためです。
マジックナンバーを使ったOR文をSet#containsに変える
下記のように、マジックナンバーとの比較ORで接続されたコードがあり、この組み合わせの条件式が色んなところで登場することがある。この組み合わせの意図がわからないと、その分だけデータを用意しテストケースを書かなくてはならなくなります。
(なお、このマジックナンバーを定数やEnumで書き換えたところで本質は変わらないので注意)
code:Before.java
// 部署が02: 総務, 03: 経理, 05: 企画の場合は、人件費に0をセット
if (deptCd.equals("02") || deptCd.equals("03") || deptCd.equals("05")) {
laborCost = 0L;
}
このようなマジックナンバーの組み合わせの条件式は、それ自体に名前をつけそこに判定のロジックも持たせます。
code:After.java
class BackOfficeDepartment {
private EnumSet<Department> backOfficeDepartments = EnumSet.of(GENERAL_AFFAIRS, ACCOUNTING, PLANNING);
public boolean contains(String deptCd) {
return backOfficeDepartments.contains(Department.valueOf(deptCd);
}
public long laborCost() {
retunr 0L
}
}
// これを使う場所
if (backOfficeDepartment.contains(deptCd)) {
laborCost = backOfficeDepartment.laborCost();
}
テスト観点の変更
今までdeptCdをこの分岐に入る入力データを用意してテストをしていた(書いていた)ところを、この意味での分岐ですべてBackOfficeDepartmentを使っているかソースレビューでチェックします。
ログを出すためだけの例外ハンドリングをAOPまたはフィルタでやる
世の中には2種類の例外がある。予期するものと予期しないものである。
予期する例外はアプリケーション層/ビジネスロジック層で設計し、ハンドリングし、上位レイヤーには伝播させないようにします。したがって、そう設計できていれば、コントローラ層で例外ハンドリングをする必要はありません。
もし、予期する例外をハンドリングする必要があれば、前述のファサードを導入しそこでハンドリングするようにしましょう。
コントローラ層は予期しない例外だけを扱う必要がありますが、これは横断的関心事なのでコントローラ自体のコードではやらず、AOPを使ったインタセプタやフィルターでやるようにします。
code:Before.java
try {
} catch(Exception e) {
LOG.error("", e);
throw new ApplicationSystemException(e);
}
クエリと型変換、計算処理を切り離す
データベースなどに対するクエリはとかく複雑になりがちで、そこに業務上の計算処理ももたせると、テストが非常にしづらくなります。
if文の条件式をfunctionとして切り出す
たくさんの論理演算子(特にOR)で繋がれた条件式は、不具合の温床です。括弧を付け忘れたり、NOTを付け忘れたり、不等号を逆にかいたり、様々な不具合を作り込む可能性があるので、きっちりテスト書いておきたいポイントです。
code:Before.java
if ((HolidayUtils.isHoliday(drive.getEnteredAt().toLocalDate())
|| HolidayUtils.isHoliday(drive.getExitedAt().toLocalDate()))
&& EnumSet.of(STANDARD, MINI, MOTORCYCLE).contains(drive.getVehicleFamily())
&& drive.getRouteType() == RouteType.RURAL) {
このif文の条件式の中身をまるっと抽出してfunctionに切り出します。一見しただけでは何を判定しているのか分からない条件式に名前を付けることができる副次的効果があります。
code:After.java
if (isHolidayDiscount(drive)) {
// ....
}
code:AfterDiscount.java
boolean isHolidayDiscount(HighwayDrive drive) {
return (HolidayUtils.isHoliday(drive.getEnteredAt().toLocalDate())
|| HolidayUtils.isHoliday(drive.getExitedAt().toLocalDate()))
&& targetVehicleFamilies.contains(drive.getVehicleFamily())
&& drive.getRouteType() == RURAL;
}
if文をファットコントローラから追い出すのは最後に
メソッドが長くなったからといって、ifやforの制御文を含む処理ブロックをまるっと別のメソッドに切り出すのは、以下の点で危険が伴います。
処理フローが追いにくくなる。
これまでファットコントローラで全体の処理の流れが一応見えていたものが、まるっと切り出したメソッドに隠されることになります。特に分岐やループは不具合の温床なので、良い構造が見出される前の段階でこれが隠されてしまうと、別の不具合を作り込んでしまう危険性が高まります。
凝集度を高められない
ファットコントローラは複数の関心事が一箇所に集まった低凝集の状態で、保守性が損なわれることが問題の本質ですが、「まるっと切り出し」はコントローラの中の順番にやる処理を切り出しただけの時間的凝集を解消できないことが多くあります。
if文の中のロジックをストラテジーパターンとして切り出す