コードレビューの時、以下のようなC++のコードがあった。

{
    Myobj *a = create_obj1();
    func1(a);

    Myobj *b = create_obj2();
    func2(b);

    if (a)
       func3(a);
    else if(b)
       func3(b);
}

新しい変数bを作らないで、aを使い回せば、ifelse文の必要がなくなって、もっとシンプルになるじゃないかと言われた。

つまり

{
    Myobj *a = create_obj1();
    func1(a);

    a = create_obj2();
    func2(a);

    func3(a);
}

のように書けば、もっとシンプルになるじゃないか？

確かに、コードの行数が少なくなる。その時、「変数の使い回しはよくない」や「immutable的な使い方をしたかった」などの話をしたが、なぜ２番目のコードがよくないのかがうまく説明できなかった。そのため、ここでよくない理由をまとめる。

  1. 読みにくい
    状態の変化によって影響を受ける場所を全部把握しないといけないので、神的な疲れが大きい。
  2. バッグの温床になる
    変数を変更するたびに、関連するコードの機能を破壊する可能性がある。
  3. Debugしにくい
    ２番目のコードでは、aのデータがおかしくなると、create_obj1で問題が起こるかcreate_obj2で問題が起こるかを調べる必要がある。
  4. 単一責任原則（Single Responsibility Principle :SRP）に違反する
    単一責任原則とはクラス（関数、モジュールなど）を変更する理由は1つ以上存在してはならない。ここではクラスでも関数でもないが、変数にも適用すると思う。極端な例をいうと、テレビのリモコンを操作するとき、音量ボタンを押した瞬間、チャンネルも変わると、ユーザに迷惑をかける。
  5. リファクタリングしにくい
    上の二つのコードを二つの関数にリファクタリングすると、１番目のコードではaとbの部分をそれぞれ別の関数にcopyすればいいが、２番目のコードは少しやりづらい。
  6. 変数の生存期間が長くなる
    使い回しているので、変数の生存期間が長くなることがよくある。生存期間の一番長い変数はグローバル変数なので、できるだけ短くした方がいい。
  7. 並列化しにくい
    複数のスレッドから同時にどのようにアクセスしても絶対に状態が変化しないので安全使える。