秘伝のタレと化したAndroidプロジェクトを改善した(い)話

この記事はKosen13s' Advent Calendar 2018の19日目の記事です。
本当は、もっと面白い記事を書きたい人生だった。

前置き

いつか貴方が「クソコード秘伝のタレ」に出会ったときの助けになりますように、という思いでこの文章は書かかれました。
もちろん秘伝のタレに出会わないに越したことはないと思います。ですが、万が一にも出会ってしまったときにちょっと思い出して貰えれば幸いです。

TL;DR

本質的に良いコード = テストが書けるコード
単体テストを書くのが難しいときはシステムテストを足掛かりにしよう

「秘伝のタレ」プロジェクトの問題：テストが書けない

秘伝のタレを生み出す原因は何でしょうか？

• 変数名が意味不明
• 1つのメソッドが数百行くらいある
• 括弧の位置がバラバラ
• 1行で色々なことをやりすぎ
• コメントがない (or 意味がないコメント)

これらの要因は確かにコードの品質を下げます。しかし、これらは秘伝のタレの要因にはならないと思っています。
秘伝のタレの本質的な要因は以下だと思います。

• モジュール間の結合が密
• スコープが広すぎる

なぜなら、これらの要因は単体テストを困難にするからです。
変数名が意味不明だったりメソッドが長かったりするだけなら、単体テストを書いてリファクタリングして終了です。
一方で、密結合なモジュールはテストを行えないので、リファクタリングにかなりの勇気を必要とします。

単体テストが困難な具体例

例えば、こんな感じのクラスがあるとします。

Data.java

/**
 * ユーザデータと商品データを扱う
 * UserとItemの実装は割愛
 */
public class Data {
  private List<User> users = new ArrayList<>();
  public User getUser(int index) {return user.get(i);}

  private List<Item> items = new ArrayList<>();
  public Item getItem(int index) {return items.get(i);}

  public void getData() {
    // apiを叩いてユーザ情報を取得している
    users = ...
    // apiを叩いて商品情報を取得している
    items = ...
  }
}

このクラスをActivityがこんな感じで使っています。

Sample.java

class Sample extends Activity {
  User user;
  Item item;
  int index;

  @Override
  onCreate(Bundle savedInstanceState) {
    Data data = new Data();
    user = data.getUser(index);
    item = data.getItem(index);
  }
	
  // 例外処理は省略
  getNextData() {
    user = data.getUser(index++);
    item = data.getItem(index++);
  }
}

このような実装になると、単体テストを行うことは困難です。
なぜなら、

1. Dataクラスとapiが結びついている = 実際にapiを叩かないとプログラムの実行ができない
2. Sample#getNextがglobal変数を参照しているため、結果が一定でない

からです。
前者は、apiへのアクセスを別のクラスにすると良いでしょう。DIコンテナを使えると非常にgoodですね☺。
後者を改善するのは非常に難しいと思います。異なるクラスの複数のメソッドがglobal変数にアクセスしまくっていると読み解くことも困難です。僕はgit grepしながらゴリゴリ読んでいきました😇。何か良い方法があれば教えて欲しいです。

改善する：システムテストを書く

「単体テストが書けないのが秘伝のタレを生み出す」という主張をここまでしてきました。
では、どのようにして秘伝のタレを改善すれば良いでしょうか？
単体テストが書けないのはどうしようもないので、システムテストを書くことにしました。「色々変更した後にシステムテストが動けばOK！」という方針でリファクタリングに取り組んでいます。
システムテストにはEspressoを使いました。
アプリの操作をコードに変換できるので、学習コストは低めと思います。

まとめ：俺たちの戦いはまだまだこれからだ

...1つ前のセクションにはもっと色々と書きたかったです。Advent Calendarに日程突っ込んだときには「あと3週間もあるし、いろいろ書けるやろｗ」とか思っていましたが、そんなことはありませんでした。
やや尻すぼみにはなりましたが、改めて結論を出すと「単体テストが書けないコードは秘伝のタレを生み出す。単体テストが書けないコードはシステムテストを書く。」ということになります。
今後、リファクタリングを続けていって得た知見を追記したいと思います。👍

余談：asciidocを使おう

ところで、仕様書がないというパターンがあると思います。存在していても、良くないフォーマットで作られているとか。
そんなときには是非asciidocを試してほしいと思います。markdownの強化版みたいなやつです。テーブル関連の表現力が良さげでした。