適切な修正を意識する

TL;DR

• 過剰な修正は悪。
• 誘惑に打ち克ち、確実に問題となる個所を選択して修正する。

こんなコードを書いたのは誰だ！？

不具合修正などで他人のコードをみると思わずこう叫んでしまうことがあるでしょう。

• コーディング規約にそっていない
• 不必要なループ
• 非効率な処理
• 変数などの不適切なスコープ
• 命名 等

挙げたらキリがありません。
ただし、いわゆるリファクタリングと不具合修正は分けて考える必要があります。

過剰に修正をした例

次のイメージはあるバグ修正の修正前と修正後をWinMergeで比較したものから色のみ抽出したものです。

大きく条件が変わったため、跡形もありません。危険なサインです。
そしてこの変更の結果、正常に動いていたケースにデグレードを起こしてしまいました。
実際の修正内容を紐解いてみましょう。

修正前の処理

リストに格納されたデータに対してループ処理で以下を順次実施していました。

1. 条件（4つの区分）に一致したデータの除外
2. （外部システムオンラインの場合）除外されなかったデータを対象にAPIで外部システムから最新情報の取得
3. データを処理対象リストに追加
4. 処理対象リストを処理

コード例は簡略化しています。

List<Foo> dataList;

dataList = new ArrayList<Foo>();
for (Foo orgData : OriginalData) {
  // (1) 区分の判定
  if (...) {
  // 除外済みデータの追加
    dataList.add(orgData)
  }
}

if (isSystemOnline()) {
  List<Foo> newDataList = new ArrayList<Foo>();
  for (Foo offlienData : dataList) {
    // (2) API問い合わせ
    Foo newData = getNewDataViaAPI(offlienData.getNo());
    newDataList.add(newData);
  }
  dataList = newDataList;
}

process(dataList);

不具合の内容

「処理対象リストのデータに誤り」

（1）の判定は過剰で一部区分だけを判定する必要がありました。
また外部システムから最新情報を取得した後に修正前の（1）と同様の4つの区分判定が必要でした。

修正後の処理

（1）で実施しているデータの除外処理はデータをひとつずつ取り出して区分を判定するため、効率が悪いと考え、（1）の処理を（2）の外部システム問い合わせの後に持ってくることにしました。
そもそものループ処理の気持ち悪さも大幅に変更してしまった一因かもしれません。
また、外部システムに問い合わせをする前に一部条件の判定が必要だったため、（2）に区分の判定処理を追加しました。

1. 条件（4つの区分）に一致したデータの除外
2. （外部システムオンラインの場合）すべてのデータ （一部除外） を対象にAPIで最新情報の取得
3. 条件（4つの区分）に一致したデータの除外
4. データを処理対象リストに追加
5. 処理対象リストを処理

上記を実現するためにループする一時リストやループの処理を大きく変更しました。

List<Foo> dataList;

// dataList = new ArrayList<Foo>();
// for (Foo orgData : OriginalData) {
//   // (1) 区分の判定
//   if (...) {
//   // 除外済みデータの追加
//     dataList.add(orgData)
//   }
// }

if (isSystemOnline()) {
  dataList = new ArrayList<Foo>();
  for (Foo orgData : OriginalData) {
    // 一部区分の除外判定
    if (...) {
      continue;
    }
    // (2) API問い合わせ
    Foo newData = getNewDataViaAPI(orgData.getNo());
    // (3) 区分の判定
    if (...) {
      // 除外済みデータの追加
      dataList.add(newData)
    }
    //newDataList.add(newData);
  }
  //dataList = newDataList;
}

process(dataList);

デグレード発生

大幅に処理を入れ替えた結果、後続のプロセスでアベンドしました。
処理を追ってみるとわかりますが、isSystemOnline()がfalseのときに、process関数に渡すdataListがnullになってしまいました。
適切にテストを実施していれば、適切にレビューを実施していれば、発見できそうです。
また、設計書と向き合うことも行われていませんでした。
しかしそれらの問題があっても、既存のコードを生かして修正を最小限にしていたら、デグレードの確率を下げることができたと思います。

誘惑に打ち克つ

リファクタリングに関する記述ですが、プログラマが知るべき97のこと - リファクタリングの際に注意すべきことから一節を引用します。

すべてをゼロから書き直したい衝動に駆られることもありますが、その誘惑に打ち克たなければなりません。
既存のコードをできるかぎり活かすべきです。
いかに醜悪なコードであっても、そのコードはテストやレビューを通っているものなのです。
既存のコード（とくに、すでにリリースされてたシステムのコード）をすべて破棄するというのは、それまでの何ヶ月（何年）という時間を捨ててしまうということを意味します。
大変な作業を経て、曲がりなりにも形にしてきたコードです。
その過程では無数に発生した問題の回避策を講じ、数えきれないほどのバグ修正もしてきたでしょう。
仮にコードを新たにゼロから書き直したとすると、同じようなことをまた繰り返すことになりますし、既存のコードでは発見／修正できたバグを、今度は見逃してしまうかもしれません。
これでは時間と労力のムダだし、過去の作業で得た知識もムダになってしまいます。
（Rajith Attapattu）

まとめ

• 大きな修正は悪いサイン
  既存の処理を修正するときは既存の処理を残したまま、どこまで実現できるかを深く検討しましょう。
  とくに繁忙期で疲れが溜まっていると万能感から大きく変更をしたくなります。
  一歩立ち止まって同僚と会話するなど協力して誘惑に立ち向かいましょう。