概要

今回、ユニットテストしづらいコードの書き方を紹介したいと思います。
コードの言語はJavaです（他の言語は読み替えて下さい）。

この記事に書いた内容をリファクタリングする記事も書きました。
https://qiita.com/oda-kazuki/items/b66fe3d4efec822497e6

結論

可読性を悪くする
結合度を高くする
循環的複雑度を10以上にする
凝集度を低くする

凝集度・結合度・循環的複雑度については以下でもまとめてます
https://qiita.com/oda-kazuki/items/a16b43dc624429de7db3

テスト困難なコードサンプル

今回、以下のような処理を想定します。

ログイン処理
- 入力：アクセストークン(指定しない場合、キャッシュがあればそれを利用する)
- 出力：ログインしたユーザー情報 (問題があった場合はnullを返却する)
- 処理：以下の処理を最大3回行う (スマホでのログインを想定し、通信でタイムアウトした場合時間を置いてリトライする)
  - ログアウトしてない場合、自動でログアウト処理をしてからログイン処理を続行
    - ログアウト処理では、キャッシュを全部削除する
    - アカウントIDがキャッシュにない場合、アクセストークンをサーバーに渡して
      アカウントIDを取得する(アカウントIDがキャッシュされている場合この処理はスルー)
  - 取得したアカウントIDからアカウント情報を取得
  - アカウント情報を用いてユーザー情報を取得して返す
  - その際のログイン情報とアクセストークン、はキャッシュに保持する(ログイン中かどうかをキャッシュによって判別)

また、記載を簡素化するため、以下のインタフェースの外部ライブラリを利用してるものとします。

public class HttpClientFactory {
    /** HTTP通信を行うためのクライアントを返す */
    static Client createClient(String domain);
}

public interface Client {
    /** WebAPIでのGet */
    HttpResult get(String path) throws HttpException;
    /** WebAPIでのPost */
    HttpResult post(String path, Json json) throws HttpException;
}

public class HttpResult {
    /** JSONのResponseBodyを取得する */
    public Json body();
}

public class JsonMapper<T> {
    /** オブジェクトをJSONにシリアライズする */
    public Json serialize(T obj);
    /** JSONをオブジェクトにデシリアライズする */
    public T deserialize(Json json);
}

そんなわけでテストしづらいコード

まずはキャッシュを作成しましょう。ここはデザインパターンを使ったSingletonパターンで華麗にやっちゃいましょうか。デザインパターン使えるぼくかっこいい。

Cache.java

public class Cache {
   private static Cache instance = new Cahce();
   // こだわりポイント1: Mapの有効活用
   private Map<String,Object> user;
   private String token;
   private String accountId;

   private Cache() {
   }   

   public static Cache getInstance() {
       return instance;
   }

   public void setUser(Map<String,Object> u) {
       this.user = u;
   }

   public Map<String, Object> getUser() {
       return this.user;
   }

   public void setToken(String a) {
       this.token = a;
   }

   public String getToken() {
       return this.token;
   }

   public String setAccountId(String accountId) {
       this.accountId = accountId;
   }

   public String getAccountId() {
       return this.accountId;
   }
}

次に処理を書きます。

LoginService.java

public class LoginService {
    // こだわりポイント2: クラス内部で初期化する
    private JsonMapper<Map<String,Object>> mapper = new JsonMapper<>();
    // こだわりポイント1: 無駄にメンバ変数を使う
    private HttpException ex = null;
    private Map<String,Object> user = null;
    private Map<String,Object> body = null;
    private String accessToken = null;
    private Json json = null;

    public Map<String,Object> login(String accessToken) {
        // こだわりポイント1: メンバ変数を使いまわす
        this.accessToken = accessToken;
        this.account = null;
        // こだわりポイント1: ローカル変数を使いまわす
        Map<String,Object> user = null;
        this.ex = null;
        boolean result = true;
        this.json = null;
        this.body = null;

        if(this.accessToken != null) {
            getCache().setToken(accessToken);
        } else {
            this.accessToken = getCache().getToken();
        }

        // こだわりポイント3: ネストいっぱい
        for (int i = 0; i < 3; i++) {
            if (getCache().getUser() != null) {
                result = this.logout();
                if (result) {
                    try {
                        // こだわりポイント1: ここだけみてもいつ bodyに値が入ったかわからなくしてる
                        this.getAccountId();
                        if (this.body != null) {
                            this.getAccount();
                            this.account = this.body;
                            if (this.body != null) {
                                getCache().setAccountId(this.body.get("accountId"));
                                this.getUser();
                            }
                        }
                    } catch (HttpException e) {
                        // こだわりポイント1: 可読性悪いからこんなコメントいれるハメになってる
                        // exは、getAccount, getAccountId, getUserで格納してる
                        if (this.ex != null) {
                            if(this.ex.getReason() == HttpException.Reason.TIMEOUT) {
                                Thread.sleep(1);
                                continue;
                            } else {
                                // その他のエラーがおきたので中断
                                break;
                            }
                        }
                    }
                    if(this.body != null) {
                        // ユーザーがとれてるから処理をやめる
                        user = this.body;
                        break;
                    }
                } else {
                    // ログアウト失敗したから中断する
                    this.body = null;
                    break;
                }
            }
            try {
                // こだわりポイント1: DRY？なにそれ？
                this.getAccountId();
                if (this.body != null) {
                    this.getAccount();
                    if (this.body != null) {
                        getCache().setAccountId(this.body.get("accountId"));
                        this.getUser();
                    }
                }
            } catch (HttpException e) {
                // exは、getAccount, getAccountId, getUserで格納してる
                if (this.ex != null) {
                    if(this.ex.getReason() == HttpException.Reason.TIMEOUT) {
                        Thread.sleep(1);
                        continue;
                    } else {
                        // その他のエラーがおきたので中断
                        break;
                    }
                }
            }
            if(this.body != null) {
                // ユーザーがとれてるから処理をやめる
                user = this.body;
            }
            break;
        }
        if(user != null) {
            this.user = user;
            // こだわりポイント4: ログインだけじゃなくキャッシュの管理もするよ
            this.getCache().setUser(this.user);
        }
        return this.user;
    }

    // こだわりポイント4: 本来の役割とは違うメソッド
    private Cache getCache() {
        // こだわりポイント2: シングルトンを直接参照する
        return Cache.getInstance();
    } 

    // こだわりポイント1: getterっぽい名前のgetterじゃないメソッド
    // こだわりポイント4: 本来の役割とは違うメソッド
    private void getAccountId() throws HttpException {
        try {
            this.ex = null;
            if(getCache().getAccountId() != null) {
                // こだわりポイント1: メンバ変数をいろんな方法で使い回す
                this.body = new HashMap<>();
                this.body.put("accountId", getCache().getAccountId());
            } else {
                if(this.accessToken != null) {
                    // こだわりポイント1: メンバ変数をいろんな方法で使い回す
                    this.body = new HashMap<>();
                    this.body.set("token" , this.accessToken);
                    // こだわりポイント2: staticメソッドの利用 & WebAPIを直接呼び出す
                    this.json = Http.createClient("https://account.example.com").post("/auth", this.mapper.serialize(body)).body();
                    this.body = this.mapper.deserialize(this.json);
                }
            }
        } catch (HttpException e) {
            this.body = null;
            this.ex = e;
            throw e;
        }
    }

    // こだわりポイント1: getterっぽい名前のgetterじゃないメソッド
    // こだわりポイント4: 本来の役割とは違うメソッド
    private void getAccount() throws HttpException {
        try {
            
            this.ex = null;
            // こだわりポイント2: staticメソッドの利用 & WebAPIを直接呼び出す
            this.json = Http.createClient("https://account.example.com").get("/accounts/" + this.body.get("accountId")).body();
            this.body = this.mapper.deserialize(this.json);
        } catch (HttpException e) {
            this.body = null;
            this.ex = e;
            throw e;
        }
    }

    // こだわりポイント1: getterっぽい名前のgetterじゃないメソッド
    // こだわりポイント4: 本来の役割とは違うメソッド
    private void getUser() throws HttpException {
        try {
            this.ex = null;
            // こだわりポイント2: staticメソッドの利用 & WebAPIを直接呼び出す
            this.json = Http.createClient("https://example.com").post("/users", this.mapper.serialize(this.body)).body();
            this.body = this.mapper.deserialize(this.json);
        } catch (HttpException e) {
            this.body = null;
            this.ex = e;
            throw e;
        }
    }

    // こだわりポイント4: LoginServiceだけどログアウトもできちゃうよ
    public boolean logout() {
        this.ex = null;
        if (this.getCache().getUser() != null) {
            this.user = this.getCache().getUser();
            try {
                // こだわりポイント2: staticメソッドの利用 & WebAPIを直接呼び出す
                Json json = Http.createClient("https://example.com").post("/logout", this.mapper.serialize(this.user)).body();
            } catch (HttpException e) {
                this.ex = e;
            }
        }
        if (this.ex != null) {
            this.user = null;
            return false;
        } else {
            this.user = null;
            // こだわりポイント4: ログアウトだけじゃなくキャッシュの削除もするよ
            Cache.getInstance().setUser(null);
            Cache.getInstance().setAccountId(null);
            Cache.getInstance().setToken(null);
            return true;
        }
    }
}

我ながらなかなか頭がクラクラするものをかけました。これをテストしろって言われたらちょっと泣いちゃう。
今から書くこだわりポイントをもっと徹底すればさらなる高みへと登ることができそうですが、高みに登りすぎてだれもついてこれなくなるのは嫌なので今回はこれくらいにしておきます。

こだわりポイント

それでは、ぼくがこだわった「テストしづらいポイント」を説明していきます。

こだわり1.可読性にこだわりました

ユニットテストを作る目的は、大前提として「仕様通りであること」を確認するというのにあります。なので、あくまでも「コード内でどんな処理が記述されているか知らない」体で期待する結果を確認していく必要があります。そうしないと、単に「書かれたコードを確認してるだけで本当に必要なテストをしていない」というテストのアンチパターンに陥ります。

とはいえ、ユニットテストを通すためにはある程度そのメソッド内でどういう処理がされているのかも知っている必要があります。そのテストの対象ではない、クラスの外側にある処理はモック化しながら作成をしていくためです。

なので、可読性を悪くするだけでもだいぶテストは作りづらくなります。今回は以下のやり方で可読性を下げました。

ローカル変数を最初に初期化して使い回す
それが何をあらわしているのかよくわからない変数がある
getXXとか書いてるのに値をgetterではないメソッドがある
不要なメンバ変数を作成し、かつ複数の用途で使い回す
DRY原則は無視する
ひとつの処理のまとまりを、プライベートメソッド内だけで完結させない
何でも入れられるMapを活用する

最後のだけちょっと補足すると、たとえば getAccount() は getAccountId() を呼び出していることが前提となっていて、これだけを実行しても失敗します。なので、処理をちゃんと理解しようとする場合はあちこちコードを飛び飛びで確認していかなければならないため、可読性の悪さに貢献しています。

こだわり2. 依存性にこだわりました

この LoginService に書かれてあるメソッドは、全てlogin()メソッドとえぐく結合させました。
これはテストのしづらさにそのまま直結してくれます。

密な結合にするため、以下のことを行いました。

シングルトンをそのまま使う(外部結合および共有結合にできる)
WebAPIのリクエストを行う(同上)
Staticメソッドを使う(インタフェースじゃないためMock化が困難)
メンバ変数を自分自身で初期化する(同じくMock化が困難)

これらを活用することで、テストをする際に「PowerMock」などを使って無理やりMock化させたりしないとテスト困難にすることができます。
もし、無理やりMock化することができない言語の場合、諦めてWebAPIの接続など許容するしかないですね。ただその場合、Webサーバー側のデータに依存するので状態によってはテストに失敗するようになります。しびれますね。
~~本当は制御結合なども盛り込みたかったのですが、面倒だったのでやめました~~。

こだわり3. ネストにこだわりました(循環的複雑度が高い)

無駄にif文などを使いまくった結果、循環的複雑度は20くらいになってます(ちゃんとはかってないけど)。

これは、少なくとも20パターンのテストをする必要があることを表していますし、そのルートをちゃんと通すために、Mockにするのも針に糸を通すような感じでやらないといけないでしょう。プライベートメソッドがあるのも困難さに拍車をかけています。
作成時に発狂する姿が目に浮かびますね。

こだわり4. 神クラスを目指しました(凝集度が低い)

このLoginServiceは、ログインに必要な全ての処理を自分自身が持っており、凝集度がとても低い仕上がりとなっております。今はまだ行数が少ないので神クラスと名乗るにはおこがましいですが、その見込はあるでしょう。

このLoginServiceは、少なくとも以下の役割を担ってます。

4つのWebAPIのリクエストを発行する
取得した情報をキャッシュに保存する
ログインのフロー(ビジネスロジック)の記述
ログアウトのフロー(ビジネスロジック)の記述

これらがいっしょくたになっていることで、ただログインのフローをテストしたいだけなのにWebAPIのリクエストやら、ログアウトのロジックやらキャッシュのことやらいろんな雑音を考慮しなければならなくなります。
更に結合度の高さも相まってテストを記述する際のSAN値を削ることができるでしょう。

というわけで

次回はこのコードをもうちょっとテストしやすい形に作り変えようかと思います。