一口にコードレビューと言っても、明確な方法論がなく行き当たりばったりだと、コードを理解するのに手間取ったり、不十分なレビューになったりしてしまいます。
そのようなことがなるべく起こらないように、私がコードレビューをする際に辿っているステップを紹介します。
きちんと順番を踏むことで、コードレビューを効率的かつ網羅的に行なうことができますので、ぜひ参考にしてみてください。
1. タスクチケットの概要とコメントを読む
コードをいきなりレビューするのではなく、タスクチケットの概要によって受け入れ基準を把握したり、これまでどのようなコメントがエンジニアとプロダクトマネージャーの間でなされたのかを確認します。
もしワイアフレームやモックなど添付ファイルがある場合はそれも確認します。
また、コメント内でエンジニアから実装のスクリーンショットがあればあわせて確認しておくことで、コードが読みやすくなります。
2. プルリクエストの説明欄を読む
プルリクエスト概要、どのような対応をしたか（コード処理の流れやロジック）、関連チケットリンクなどがプルリクエスト自体に記載してあれば、それを読んで概要を把握します。そうすることで、実際のコードを読む時に頭に入りやすくなります。
3. 動作を確認する
・多くの場合、コードを見るよりも先に受け入れ基準などのタスク仕様を読みながら動作確認をした方が良いです。その方が、コードレビューの際にそのコードがどんな処理をしているかを理解しやすいからです。また、もし受け入れ基準に沿った動作をしていなければ、バグ発生箇所を既に特定出来た状態でコードレビューに臨むことができます。具体的には、受け入れ基準をコピペしてチェックリスト化し、各項目ごとにパスしているか、フェイルしている場合はどのようにフェイルしているかをメモします。
・バグ修正であれば、修正前のコードをチェックアウトしてバグの再現をしてから修正コードの動作確認をします。そうすることで、修正コードが本当にそのバグを修正するのか、修正方法として適切なのかを確認しやすくなります。
・APIの場合は、curlなどで実際にエンドポイントを叩いてみます。
・動作確認で出力されるログも見る。ファイル名までは見ませんが、どのようなroute、コントローラーが叩かれたのか、どのようなSQLが実行されたのか。新しいテーブルやカラムが追加された場合は、どのようなレコードが追加されて変更されたのか。必要であればデータベースクライアントを使ってデータの中身まで確認。データ構造を把握しておくとコードは読みやすい。また、N+1クエリもコードレビュー前に把握できます。
・タスクによっては、画面遷移のスクリーンショットや動画を撮っておきます。レビューの途中であの画面どうなってたっけ？と見返すときに、再度同じ動作を繰り返さなくても良いからです。また、もしバグや受け入れ基準と異なる挙動を見つけた場合、スクリーンショットや動画を使って最後のフィードバックのステップで実装者に共有します。
・計算式の修正や確認などで軽微なものなら、インタラクティブ・プロンプト（いわゆるREPL。rubyならirb）で計算結果を確認しながら進めると良い場合があります。例えば小数点の計算で四捨五入や切り上げ、切り捨て関数などは実際に数値を計算するのがおすすめです。
・動作確認時に気になったことを全て記憶するのは大変なので、メモアプリで自分用のメモを取っていきます。使うものはなんでも良く、コードエディターでも良いですし、Google Keepなどのオンラインツールもおすすめです。これはあくまで自分用のメモなので、レビューコメントとは別物です。
4. 変更されたファイル一覧を確認する
プルリクエストでは変更されたファイルの一覧を見ることが出来ますので、どのファイルが新規作成されたか、変更されたか、削除されたかをざっと確認します。
これを行なうことで、コードを見る前に全体感が掴めるようになります。いわゆる、木を見る前に森を見る、というやつです。
既に動作確認を終えているので、ファイル一覧を見るだけで各ファイルがどの部分の動作を担当しているのかは大体想像出来るはずです。ここで予期せぬファイル名があったら、動作確認をし忘れた箇所がある可能性がありますので、後ほどの実際のコードを見るステップで確認します。
ローカルで確認する場合は、以下のコマンドで変更ファイル名の一覧とステータスを見ることができます。以下はorigin/masterとの差分ですが、エピックブランチやdevelopブランチを使うこともあります。
# origin/masterに別の開発ブランチが取り込まれていた場合に除外出来るように、3点ドット(...)を使って共通のマージベースからの差分のみを表示。
git diff --name-status origin/master...HEAD表示されるステータスの意味は以下の通りです
- A：追加されたファイル
- C：コピーされたファイル
- D：削除されたファイル
- M：変更されたファイル
- R：ファイル名が変更されたファイル
5. コードを確認する
・全体の処理の流れを確認します。この時点ではあまり細かいことは気にせず、流れを掴むだけです。
・気になる箇所があったらコメントの形でメモを取っていきます。この時点では実装者に知らせる必要はなく、自分用のメモです（自分用メモ、TODO、仕様確認などをメモの最初に記載します）。もし仕様確認後にメモの内容が明確になったら、レビューコメントとしては不要なのでコメント削除します。
・githubなどのオンラインレポジトリーでPRを確認すると同時に、ローカル環境でもPRブランチにスイッチして確認します。オンラインレポジトリーだと隠されているコード箇所が多く（expandすることは出来ますが）、ローカルの方がファイル全体を確認しやすいからです。別の理由としては、ローカルだとLSPを使って定義や参照箇所などを確認しやすいという点もあります。
・githubのファイルパスのコピー機能を使うと、ローカル環境でファイルパスをペーストするだけで目的のファイルを一発で開けて便利です。
・ローカルのエディター設定で行ナンバー表示を有効にしておくと、githubの該当箇所の行と簡単に比較が出来るのでおすすめです。
・もしデータベースのマイグレーションが含まれる場合は、マイグレーション・ファイルを一番先に見るようにしています。データ構造が頭に入っていたほうがコードが読みやすいからです。
・ファイル並び順の上から順番に見ていくのではなく、実際のユーザー遷移の順番やデータフローに沿ってファイルを見ていきます。例えばMVCフレームワークだったら、ルーティング > コントローラー > モデル > ビューの順番で確認します。
・SQLのviewファイルのバージョン更新などで、コードがまったく別のファイルにコピペされてさらに加筆されているような場合は、git diffでなく普通のdiffを使って２つのファイルを比較すると差分を確認しやすいです。例：diff -up file1 file2
・もしコードの処理が複雑で追えなかったり、気になる箇所があればデバッガーをローカル環境で実行して処理の流れや変数の値を確認します。特に、どのページ遷移でどこのコードが実行されるのかがわからない場合、不明なコード箇所にデバッガを設置して一通りの動作を試してみると判明することが多いです。
6. 実装者に結果をフィードバックする
レビューが終わったら、個別コメントに加え、全体コメントを総括として加えると、よりコミュニケーションが捗ります。全体コメントとしては、問題ないならLGTMのような簡潔なものから、個別のコード行にコメントするのには適さない全体設計についての指摘など多岐に渡ります。
そして、もし実装に問題がある場合は変更を要請、問題ない場合なら承認します。
なお、何も問題ない場合でも、レビューを送信することは重要です。別のタスク管理ツールや口頭でコミュニケーションを取って終わりにすることももちろん可能ですが、他の人がプルリクエストがどのような状態にあるかを把握出来なくなりますのであまりおすすめはしません。また、もし後で追加コミットが発生した場合、githubにある最終レビュー完了時からの差分確認機能も使えなくなってしまいます。
また、プルリクエストとは別に、タスク管理ツールでもレビューが終わったことをコメントで残して置くと、githubからの通知に気づかないといった事態を防ぐことが出来ます。
まとめ
この一連のプロセスを経て、コードレビューは完了となります。もし参考になるようでしたら、ぜひご自身のコードレビューに取り入れてみてください。