コードレビューを半年以上行っているので、自分なりのコードレビューの観点をまとめておく。
大枠はIPAセキュア・プログラミング講座の第2章 脆弱性回避策とソフトウェア開発工程のソースコードレビューの章を参考にしている。
レビューは4回に分けて行う
ソースコードレビューは次の4つの段階に分けて行う。
4回というのはレビュワーとレビュイー間のやりとりを4回行うという意味ではなく、単に4つの観点からレビューを行うというもの。
時間はかかるが、最初のうちは何をレビューすればいいか分からず指摘しやすいコーディング規約に関する指摘しかされないケースが多いので以下の4つ観点に沿ってレビューを行う。
| 1回目 | 修正内容の確認 | 修正内容の確認を行い修正箇所を把握 |
|---|---|---|
| 2回目 | コードの品質 | コーディング規約に沿っているか |
| 3回目 | 機能性 | 機能としての要件を満たしているか |
| 4回目 | パフォーマンス・セキュリティ | パフォーマンスやセキュリティに問題はないか |
1回目： 修正内容の確認
最初に方針の確認と修正範囲の把握を行う。
方針の確認
- まずはチケットを確認して何の機能/不具合に対する実装をおこなったのか確認する
- 本来であればチケットやプルリクに実装方針が書かれているのでそれを見るが、ルール化されておらず記載がなければ実装者に確認する
修正範囲の把握
- 修正されたコードがあるファイルやメソッドの構成を見てそれらがどの機能のどの部分なのか把握する
- 各モジュールの参照関係、影響範囲を確認する
- どのようなデータ構造を扱っているか確認する
2回目：コードの品質
リーダブルコードに載っているようなコーディングルールに沿っているかを確認する。
先に不要なコードはないか確認
- console.logは全て削除されているか
- debuggerが残っていないか
- デッドコードがないか
見るべき観点（抜粋）
- 変数・メソッド・クラス名の命名規則が適したものになっているか
- モジュールの分割がなされているか、1つのメソッドの行数が多すぎないか
- 例外処理がなされていること
- 再起処理が無限ループしないか
- 外部から入力されるデータには全てバリデーションをかけているか
- コメントは適切に書かれているか
参考にしているサイト
自分の場合はフロントエンドでjsをメインで書いているので、下記をよく参考にしている
3回目：機能性
書かれたコードが要件に沿って動作するかを確認する。
レビュワーが動作確認まで行うのは手間なので、基本はレビュイーに動作確認をしてもらう。
影響範囲によってはどこの機能を重点的に確認してもらうかをレビュイーに伝えて動確させる。
フロントだとUIの修正の場合、画面上で確認する必要があるので
4回目： パフォーマンス・セキュリティなど
自分の場合は最終的なパフォーマンスチェックはQAで計測を行ってくれるので、実装面で問題がないかを確認する。
- GETのリクエストを送る際はキャッシュを利用したものになっているか、もしくは利用を検討したか
- ライブラリのインポート時にライブラリ全体をインポートしていないか
- 並列処理を行なっているか