リファクタリングに失敗してとんでもないバグを入れてしまった orz

2010/07/22

リファクタリング前のクラス

class myClass {
   ...
   function doSomething($user_id)
   {
           $sql1 = "SELECT  * FROM tb1 WHERE user_id = " . $user_id;
           ....
           $sql2 = "DELETE FROM  tbl1 WHERE user_id = " . $user_id;
   }
   ...
}

リファクタリング後のクラス（バグ有り）

class myClass { 
　...

   function doSomething()
   {
           $sql1 = "SELECT  * FROM tb1 WHERE user_id = " . $this->gerUserId();
           ....
           $sql2 = "DELETE FROM  tbl1 WHERE user_id = " . $user_id;
   }
   ...
}

いわゆる「問い合わせによる一時変数の置き換え」をやろうとして、置換が漏れてました。（$user_id を $this->getUserId()に置き換えたかった）しかもこのバグを本番にアップして２ヶ月間も気づかず放置。上記のDELETE文はSQL文法エラーで実行されず、消さなきゃいけないデータが２ヶ月も消えないまま残ってしまっていた。。。

敗因

最初に頭によぎったことは、「Perlで書いてれば！！Perlなら絶対use strictって書くからこんなミスしないのに！！」こともあろうに私は言語のせいにしようとしました。もちろんPHPに非があろうはずがありません。悪いのは私であって、言語ではありません。

E_STRICTを有効にしてなかった

Perlのuse strict;を羨むくらいなら、E_STRICTを有効にしておけばよかった。

自動単体テストを書いていなかった

リファクタリングには自動単体テストが必須なわけですが、私はこれを怠けたのです。
「それってリファクタリングちゃうやん」と言われたら、まったくその通りです。

手動テストをしていなかった

上記delete文がちゃんと機能しているかを確認する手動テストすらしていませんでした。典型的なEdit & Pray（編集して祈る）開発スタイル !!

本番サーバでのエラーログを見てなかった

上記スクリプトはSQL的に文法エラーなわけで、当然サーバのエラーログにもエラーが残っていました。しかしその文法エラーのエラー文を見ても、上記スクリプトが原因だと気づきませんでした。。。

ユーザからの声に気づけず

「動作がおかしい」という話をここ数週間聞いてたのですが、どうしてもバグが再現しないので「バグじゃないかもしれない」などと甘いことを考えていました。本気でバグを探そうという気概が足りなかったのかもしれません。

心がまえ

「自分はリファクタリングに慣れてるからテストなんか書かなくても大丈夫」という慢心がありました。この慢心がすべての原因のような気がします。（←これが言いたかった）

カテゴリ: PHP