DRY原則と一緒に教えてほしいこと

なにが言いたい記事か

既存関数の引数にフラグを追加するのは止めさせてほしい。

想定読者

新人プログラマさんにも読んでもらいたいが、それ以上に新人に教育する方とか、レビュアーさんに読んでもらいたい。

DRY原則とは

こちらをどうぞ
DRY原則（プログラマが知るべき97のこと）

要はコーディングにおいては抽象化で重複を避けましょうね、という主張。
本当は「抽象化」というところが肝なのだけれどそこを削ぎ落として単純化し、「コピペコード禁止」としている現場もある。

間違った解釈

ところが新人さんにこのDRY原則を教えると曲解してしまう人が必ず一定数いるのだ。
具体的には以下のような具合だ。

例えば以下のコードがある。

void do_something() {
    // 処理A
    // 処理B
}

新たに処理Aと処理Cを処理する関数が欲しくなったとする。
DRY原則を曲解した人は以下のようなコードを書いてしまう。

void do_something(bool do_C = false) {
    // 処理A
    if (do_C) {
        // 処理C
    } else {
        // 処理B
    }
}

「まさか？！そんなコード書く人なんていないだろ」というあなたはいままで幸せなプログラミング人生を送ってきました。うらやましい。

何がいけないのか

責務分割とか凝集度・結合度といった話は置いておいて、引数へのフラグ追加を放置してるとこうなっちゃうよという話。
実際に以下のようなコードに遭遇したことがある。

void do_something(bool do_A=false, bool do_B=false,
                  bool do_C=false, bool do_D=false,
                  bool do_E=false, bool do_F=false) {
  // なにか処理

  if (do_A) {
    // 処理A
  }

  if (do_B) {
    // 処理B
  }

  if (do_C) {
    // 処理C
  }

  if (do_D) {
    // 処理D
  }

  if (do_E) {
    // 処理E
  }

  if (do_F) {
    // 処理F
  }
}

まずテストが大変だ。c1網羅しようと思うと $2^6$ 通りのテストが必要になる。もしかしたら実際には処理Aと処理Cが同時に実行されるケースなんて無いのかもしれないのにだ。

更に可読性も悪い。

do_something(true, false, true, false, true, false);

上記コードを見て何をしてるか即座にわかる人はまずいないだろう。

どうするべきだったのか

冒頭の例であれば、処理Aを関数化するだけでよかったのだ。

void do_A() {
    // 処理A
}

void do_something() {
    do_A();

    // 処理B
}

void new_function() {
    do_A();
    
    // 処理C
}

どうしてこうなった

新人さん（だけじゃないけど）を観察してると一定数いるのが、「関数を作るときは必ず既存関数をコピペして修正する」という人だ。話を聞いてみると新規で関数を作ることに何かしら抵抗があるようなのだ。そういう状況の人に一律「コピペ禁止」というルールを押し付けると何とかして既存関数のみで解決しようとし、既存関数へのフラグ追加という結果になってしまっているようだ。

解決策？

画期的な解決策というものはなくて、やはり地道なレビューが重要だと思う。
経験上メンテナンスコストの増大という意味ではコピペコードよりもフラグまみれの関数の方がはるかに被害が甚大なのだが¹、単体テスト等で見つけられる類のものでもない（デフォルト引数の追加では既存の単体テストは通ってしまう！）ため、レビュアーさんの頑張りにかかっている。

1. とは言うものの現実はもうちょっと厳しくてフラグまみれの関数がさらにコピペされたりするのだ。 ↩