プルリクレビューのノウハウ＆観点集
ここ数年、ときどきPRレビューに追われているのだが、なかなかレビュー観点の体系化は難しい。
かといって蓄積していかないとチームメンバーへ渡すことも難しいため、自身のノウハウを少しずつ書き溜めていく。
随時更新予定。
レビュー水準
最も高いレビュー水準は「自分だったらどのように設計・実装するか」との差分を取ることだが、すべてに対してこれを行うのは非現実的。
原則はリスク＝不具合や障害が顕在化したときの影響度 × 顕在化する確率として捉え、レビュー水準を変更すること。
不具合や障害が顕在化したときの影響度
以下を満たす機能・システムであるほど、影響度は大きいと判断する。
- 取り返しのつかない要素がある
- ミッションクリティカル性が高い
- 代替手段が存在しない
上記の「取り返しのつかない要素」とは、主に以下を指す。
- 大きな経済的損失が発生するリスク
- 情報流出などセキュリティのリスク
- 人命に関わるリスク
顕在化する確率
顕在化する確率の構成要素として発見可能性と発生頻度がある。
発見可能性
テストやCIなどで発見できるかどうか。
もし確実に発見して対策できるのであれば、必ずしもレビューで見つける必要はない。 1
発生頻度
そのリスク事象（不具合や障害）が発生する確率。以下を満たすほど、発生頻度が高い。
- 利用頻度が高い機能に関する事象
- 再現性の高い事象
レビュー観点
MECEに書こうと思ったが、なかなか難しい…。
ビジネス的な妥当性
要求・要件に合致したものとなっているか。
明確に要求された内容はもちろん、ユーザにとって勘違い等を招きそうな要素がなさそうかを確認する。
技術的な妥当性
機能的に正しく動作するか、テストケースに過不足がないかを確認する。
テスト自体は手動でもテストコードでもよいが、資産性・再現性・厳密性などを考えるとテストコードのほうが望ましい。
ただしフロントエンドではテストコードでの実装が難しい場合も多くあるため、PRの本文にテストケースやスクリーンショットを載せてもらうとレビューしやすくなる。
特に、コードレビューでしか発見できないような不具合がないかどうかを意識してレビューする。
フロントエンドの場合はコードの変更行数が多くなりやすいが、手動での動作確認が容易であれば流し読みで済ますのも一案。
テストでの発見が容易であるなら必ずしもレビューで防ぐ必要はない。
また非機能要件として、処理件数によるメモリ使用量やパフォーマンスへの影響は許容できそうかを確認する。
例えばレコード数が単調増加していくようなテーブル対してSQLで全件取得すると、いつか破綻する可能性がある。
そうならないためにも、使用するメモリサイズが許容できるか、監視でパフォーマンスの悪化は検知できるか、少なくとも何ヶ月持つか、といった観点で確認する。
非機能要件にはセキュリティもあるが、後述。
取り返しのつかない要素
前述したような「取り返しのつかない要素」についてレビューする。その中でも、特に重要なのはセキュリティ。
認証・認可制御が適切に行われていることを確認する他、一般的な脆弱性が入り込まないようにする。
セキュリティ関係は通常の動作確認では不具合を発見できないことも多いため、特にコードレビューでチェックしたい内容。
またシステムの開発・運用面から、将来の選択肢が縛られるような内容が含まれていないことを確認しておく。
例えば入力文字数を40文字から100文字に広げることは容易であるが、100文字から40文字に制限することは難しい。2
こういった項目については取り返しの付かない要素として認識し、その機能の初回リリースまでにレビューで発見・対策しておきたい。
保守性
コードの可読性が低い箇所や、今後修正できなくなりそうな箇所がないかを確認する。
また、未来の人に対していわゆる「地雷」を埋めることにならないかを確認する。
特にコードだけでは目的や処理内容が分かりにくい箇所は、要求や要件、参考URLなどをコード上にコメントとして記載してもらうとよい。
PR上でやりとりされる質問・回答・補足についても、重要な内容はコード上のコメントとして残してもらうようにする。
リリース可能性
ダウンタイムが許容し難いような場合は、リリース時にダウンタイムが発生しないような構成となっているかを確認する。
リリース時に不具合や想定外の事態となった場合に備え、切り戻しが可能な構成となっていることも確認する。
もしリリースするタイミングをコントロールする必要がある場合は、マージ・リリースしてよい前提条件を明確にしておくこと。3
GitHubの使い方
Notification画面を活用する
NotificationのInboxではGmailをフィーチャーしたショートカットが設定されており、未読メールを捌くのと類似した操作で通知を管理できるようになっている。
具体的には以下の通り。
- Inboxで1つ以上チェックを入れた状態で
eを押すとDoneにできる- Gmail：チェックを入れた状態で
eを押すとメールをアーカイブすることができる
- Gmail：チェックを入れた状態で
- Inboxで通知内容を開いた後、
eを押すとDoneにしてInboxへ戻ることができる- Gmail：メールを開いた状態で
eを押すと、そのメールをアーカイブすることができる
- Gmail：メールを開いた状態で
通知を受け取ることができれば、対応が必要なPRレビューはすべてこの画面で把握することができるようになる。
プルリクテンプレートを作る
ひとまず以下の項目が書かれていればレビューしやすいはず。
- 概要
- 対応方針
- 具体的な変更内容
- テストした内容
- レビューしてほしいポイント
Suggestion機能を使う
以下に当てはまる場合は、Suggestion機能を使ったほうが日本語よりも早くて正確に伝えられる。
- 1ファイルの数行程度で完結する修正
- typoなど、修正すべきことが自明なもの
- 日本語で表現することが難しい・煩わしい内容
Commit毎に差分を見る
Commit毎に差分を見たほうが変更の妥当性を確認しやすいことも多い。
ただし、これはRevieweeが適切な規模とコメントでCommitできることが前提となる。
Approveの判断根拠を書き残しておく
Approveした内容が本番環境で不具合として顕在化した場合、「これレビューのとき確認しなかったっけ…」となることがある。
そうならないためにも、将来の自分のためにApproveとした根拠を書き残しておく。
またApproveの判断根拠を書き残していくことで、周囲のメンバーに対する教育効果もある。