効果的なコードレビューについて書いた我々のブログ記事の中で、チェックリストを使う事を推奨した。チェックリストを使う事で、レビューが継続的にチーム全体でうまくはたらくようにできる。また、ありがちな問題を見つけたり解決したりするのを確実にする、簡単な方法でもある。
ソフトウェア工学研究所の研究によると、プログラマは15から20の良くある間違いをしてしまいがちだという。そういった間違いをチェックリストに追加しておく事で、問題が起こった時にそれを特定し、確実に駆逐するようにできる。
チェックリスト作りに取り掛かるに当たって、一般的な項目は以下のリストのようになるだろう。
コードレビューチェックリスト
一般
- コードは動作するか？想定した役割を果たすか、ロジックは正しいか、など。
- 全てのコードは理解しやすいか？
- 同意されているコード規約に従っているか？カッコの位置、変数名や関数名、行の長さ、インデントの数、フォーマット、コメントと言ったものが含まれる。
- 冗長あるいは重複したコードはないか？
- 可能な限りモジュール化されているか？
- 置き換え可能なグローバル変数はないか？
- コメントアウトされたままのコードはないか？
- ループは回数が決まっているか？あるいは終了条件は正しいか？
- ライブラリ関数で置き換え可能なコードはないか？
- ロギングあるいはデバッグ用のコードは削除されているか？
セキュリティ
- 全てのデータ入力はチェックされ(型、長さ、フォーマット、範囲が正しいかどうか)、エンコードされているか？
- サードパーティ製のユーティリティを使っている部分では、返されるエラーを拾っているか？
- 出力値はチェックされ、エンコードされているか？
- 不正なパラメータ値の扱いが書かれているか？
ドキュメント
- コメントは存在していて、かつコードの意図が記述されているか？
- 全ての関数にコメントが付いているか？
- 通常と違う振る舞いや、特殊な場合の扱いについて説明されているか？
- サードパーティ製ライブラリを使っている事やその関数についてドキュメントに書かれているか？
- データ構造や数値の単位は説明されているか？
- 中途半端になっているコードはないか？その場合、その部分は削除されるべきか？あるいは'TODO'のような適切な目印でフラグを立てておくべきか？
テスト
- コードはテスト可能か？すなわち、依存性が強すぎたり隠されていたりしないか、オブジェクトが初期化できなくないか、テストフレームワークがメソッドを使えるか、など。
- テストが存在しており、かつ理解できるものか？少なくとも、同意されているコードカバレッジを守る程度はあるか？
- ユニットテストは、コードが意図した機能を実現するよう動作しているかどうかをテストしているか？
- 配列の「out-of-bound(範囲外のアクセス)」エラーはチェックされているか？
- テストコードは既にあるAPIで置き換え可能ではないか？
この他に、言語特有の問題の種になり得るものもチェックリストに追加したいところだろう。
このチェックリストは、ありうる問題を全部網羅しないように意図的に作っている。誰も使わないような長いリストなんて必要ないだろう。一般的な問題をカバーするだけでいいのだ。
チェックリストを最適化しよう
このチェックリストを始めの一歩にするとしても、それぞれのユースケースに合わせてリストを最適化した方がいいだろう。ある一定期間のコードレビューで上がってきた問題を記録しておくのが一番いい方法だ。その記録を元に、チーム内で起きがちな間違いを特定して、そこから自分たちのチェックリストを作る事ができる。再発しそうもない問題(セキュリティ関連の問題など、あまり起きそうもないがクリティカルなものは残しておきたいかもしれない)を除く事を忘れないように。
同意を得よう、そして常に最新に保とう
一般的なルールとして、チェックリストの全ての項目は明確で、さらに可能であれば、二択の判断ができるものになっていた方がよい。これが、判断のブレをなくす手助けになる。また、チェックリストをチーム内で共有し、内容についてメンバーの同意をとっておくのもよいだろう。各項目が意味のあるものであるかチェックするため、定期的にチェックリストを見返すのも忘れないように。
素晴らしいチェックリストを武器にすることで、コードレビューの際に見つけていた数々の不具合を取り上げる事ができるようになる。これによって、コーディングの基礎を引き上げ、コードレビューの品質のばらつきをなくすことができる。
Fog Creekでのコードレビューについてさらに知りたいなら、このビデオを見てみよう。
