ifの多いプログラムの解消法

以下のコードをDRY原則(重複コードを作らない)に基づいてリファクタリングするとすればどのようにするだろう。

public function a() {
  $valueA = 1;
  $this->c();
  $this->d();
  $this->e();
  $this->f();
  $this->g();
  $this->h();
  $this->i();
  $this->j();
  $this->k();
}

public function b() {
  $valueA = null;
  $this->c();
  $this->d();
  $this->e();
  $this->l();
  $this->m();
  $this->n();
  $this->i();
  $this->j();
  $this->k();
}

関数a()と関数b()の違いは途中に呼び出す関数がf(),g(),h()かl(),m(),n()かだ。
弊社のアルバイトは以下のようにコーディングしていた。

public function a() {
  $valueA = 1;
  $this->o($valueA);
}

public function b() {
  $valueA = null;
  $this->o($valueA);
}

private function o($valueA) {
  $this->c();
  $this->d();
  $this->e();
  if ($valueA != null) {
    $this->f();
    $this->g();
    $this->h();
  } else {
    $this->l();
    $this->m();
    $this->n();
  }
  $this->i();
  $this->j();
  $this->k();
}

これは処理をまとめているようで、まとめたことにならない。
関数を作成すると同時にif文を作成してしまっている。
そのため、この関数をテストする場合は最低でも
ifを通る場合とelseを通る場合の二通りをテストしなければならず、
結局2つの処理は2つのままだ。
上記の程度であればまだ処理は単純に見えるが、
上記のような関数を作成するプログラマーが開発し続けたコードは
ifやforが何重にもネストした可読性の低いコードになっていく。

関数にまとめるのであれば、まずは以下のようにまとめる。

public function a() {
  $valueA = 1;
  $this->o();
  $this->f();
  $this->g();
  $this->h();
  $this->p();
}

public function b() {
  $valueA = null;
  $this->o();
  $this->l();
  $this->m();
  $this->n();
  $this->p();
}

private function o() {
  $this->c();
  $this->d();
  $this->e();
}
private function p() {
  $this->i();
  $this->j();
  $this->k();
}

このように、処理の異なる部分はもとのままにしておく。
これで関数o()とp()は分岐のない処理のまま可読性を確保できる。

更に以下のようなまとめ方もできる。

public function a() {
  $valueA = 1;
  $function = function() {
    $this->f();
    $this->g();
    $this->h();
  }
  $this->q($function);
}

public function b() {
  $valueA = null;
  $function = function() {
    $this->l();
    $this->m();
    $this->n();
  }
  $this->q($function);
}

private function q($function) {
  $this->c();
  $this->d();
  $this->e();
  $function();
  $this->i();
  $this->j();
  $this->k();
}

これはStrategyパターンとかDependency Injection(D.I)と呼ばれ、
for文の中のifを取り除いたりするのに有効な方法だ。
この場合はもちろん関数a()や関数b()をテストするときに
関数q()の仕様も含めたテストを行っておきたい。
D.Iに関するブログなどを見るとモックに置き換えてテストするような記述が多く見つかるが、
それは上記で言うところの
引数$functionのコーディングが完了するまでの間の関数q()のテストについてであり、
関数a()や関数b()をテストするときに関数q()をモックに置き換えてはいけない。
また、無名関数も増えすぎると可読性に影響が出てくるので
いつかの段階でクラスとそのメンバー関数として定義し直しておいた方がよいだろう。

話をifに戻すと、それでもどうしてもifが必要な場合があるかもしれない。
たとえば以下のようになっていたとする。

public function a() {
  $valueA = unkown();
  if ($valueA != null) {
    $this->b();
    $this->c();
    $this->d();
  } else {
    throw new Exception();
  }
  $this->e();
  $this->f();
  $this->g();
}

関数unknown()の結果が事前に何になるかわからないとする。
このような場合はifの条件部分に必要な変数が取得できた時点でif文を作成し、
できればelse節を記載しないで済む方法を取る。

public function a() {
  $valueA = unkown();
  if ($valueA == null) {
    throw new Exception();
  }
  $this->b();
  $this->c();
  $this->d();
  $this->e();
  $this->f();
  $this->g();
}

こうすることでネストを深くしないで済み、可読性を確保できる。
これらの考え方はtwigやjspなどのHTMLの動的生成時のコーディングにも応用できる。
決してヘッダーとフッターの間にif文を書くようなtwigを書いてはいけない、
本文からヘッダーとフッターのテンプレートを参照するのだ。

まとめ