プルリクレビュー
プルリクのHow-Toに関する記事をあつめてみました。
プルリクレビューの10の過ち
1. 心のこもらない+1
信頼できる人だからといって、ちゃんと中身を見ずに「+1」や「LGTM」しない。
2. 怠慢
つい後回しにしてしまう。その理由とちゃんと向き合おう。
変更の意図がよく分からなければ、ちゃんと聞こう。「元の要件はなんだったの?」など。
変更が大きすぎて一度にレビューできなさそうな場合は、分割をお願いしよう。
何か理解できないことがあれば、プライドを捨ててちゃんと聞こう。
あまりにも問題や懸念事項がたくさん出たら、プルリク作成者と膝を突き合わせて話しよう
3. Unified diffs
Unified形式のdiffではBefore/Afterが分かりにくいので、Split形式を使おう。
4. 本質的なことよりもスタイルに気を奪われる
スタイルやフォーマットはLinterに仕事させて、本質的な問題に集中できるようにしよう。
5. diffだけでは不完全
diffは何が変更されたかは非常に分かりやすいが、変更されてないものは表示されない。なので、変更すべきなのに、されてないものはコードベース全体を見ないと分からない。
diffに含まれていないテストも、それだけ見てては欠けていることに気づきにくい。
6. テストコードがあるだけで気を抜くな
テストはチームが共有コンテキストを構築するのに最適な場所だ。
テストのコストとリスク軽減を適切にバランスさせよう
テストのタイトルは適切に記述されているか?
主要なシナリオが網羅されているか?
エッジケースがカバーされているか?
テストが失敗したら、エラー追跡が簡単にできるようになっているか?
7. フロントエンドの複雑さを考慮しない
フロントエンドは特に、コードだけみても、動作まで保証できることは困難である。実際にチェックアウトして動かし、自分の目で確かめてみよう。
8. 狭い考え
diffだけ見てそれが良く出来てそうだからといって、油断してはいけない。
その新しいコードがプロジェクトに追加されるようになって、何が変わるのか? 何が起こるのか? を注意深く見よう。
性能に影響するか?
ユーザへの影響はどれほどか? 変更内容をユーザに伝えるほどのものか?
ログ、監視、デプロイプロセスへの影響は?
ステージングと本番で違うものはないか?
9. 短期的な思考
プルリクでハイコンテキストなやり取りがされることは、短期的には良いことだが、後から新しい開発者がそのコードを触るときに問題となる。
将来の開発者がアクセスしやすいようにしておこう。
プルリクの主要な議論をコードのコメントに残しておく。
ADRのような形で議論内容をドキュメント化しておく。
コード内のTODOは、プルリク作成者だけでなく他の人が理解して対応できるように書かれていなければならない。
10. 修正の上っ面のレビュー
プルリクの修正は、みんな前に進みたい訳なので、実は最もリスクが高いところだ。開発者は注意力が欠けがちだし、レビューワもレビューがおろそかになりがちだ。
How to code review in a Pull Request
レビューするとき見るもの/やること
変更の意図
なぜその変更が必要かが分かるようになっているか。
変更の影響
その変更で壊れる箇所がないかどうか?
書いた人と一緒にレビューをしない
プルリクを書いた人に説明してもらいながらコードを読むと、後でプルリクだけを読んだ人が理解できないものになることが多い。
ブランチをチェックアウトする
何かおかしい? と少しでも違和感を感じたら、チェックアウトしてエディタ/IDEで詳しく調べよう。
悪い習慣を見つける
悪い習慣だと思うコードを見つけたら指摘をしよう。プルリクは議論の場だ。
可読性が重要である
コードは書くよりも、読む機会の方が多い。
大きなプルリクを分割する
リファクタリングを別のプルリクにする。
1プルリクは1つの問題になるようにする。
テストのレビュー
コーナーケースを考える
コードがおかしければテストは失敗するようになっているか
出力に基づいて作られたテストを拒否する
実際のデータを使ったテストを書くこともあるが、コーナーケースを見つけられなくなるため。それだけにしないようにする。
Code Review Best Practices
コードレビューの意義
コミッタのモチベーションを高めるため (信頼できる仲間からお墨付きをもらえる)
知識の共有
コードベースに一貫性をもたらす
可読性を維持するため
構造的なエラーだけでなく、typoのような偶発的なエラーを見つける
セキュリティ的な問題を検出するため。
何をレビューするか
正解は無いのでチームでよく話し合って決める。以下のような戦術がある。
メインブランチにマージされるすべての変更をレビューする。
レビュープロセスを省略してもよいもののしきい値を決める。
ただし、人で決めないこと。どんな上級者でもレビューは必要だ。
いつレビューするか
自動チェック(テストやlint)をパスした後、メインブランチにマージされる前に行うべきだ。
レビューのためのコード準備
スコープとサイズ
適切なサイズ
5ファイル以内
1〜2日程度以内
20分以内でレビューできる
セルフチェック済みでテストをパスしたものをレビューに出す
リファクタリングは動作を変更してはいけない
逆に動作を変更する場合は、リファクタリングやコードフォーマットの変更を避ける
目的
このコードは書いた人の目的を達成しているか?
関数やクラスは何のために存在しているのか?
実装
自分だったらどう問題を解決するか?
エッジケースが他にないか?
まだ短く/簡潔に/きれいに/速く/安全に書けるところがないか?
抽象化できるところがないか?
批判的な目でみよう
ライブラリや既存のコードとの適合性を考えよう
変更は標準のパターンにしたがっているだろうか?
依存ライブラリが追加されている場合は、その妥当性をみよう。
可読性とスタイル
読みやすかったかどうか考えてみよう。
コンセプトを適度な時間で把握できたか
流れがまともで、変数名やメソッド名は分かりやすかったか?
矛盾したネーミングに気を取られなかったか?
TODOが残っていないか?
どうしてもTODO残してマージする場合は、Issueを起票してもらおう。
保守性
テストが書かれているか? 十分なケースになっているか?
テストコードを壊すリスクがないか?
後方互換性を壊していないか?
その変更はインテグレーションテストが必要か?
ドキュメント、コメント、コミットメッセージについてフィードバックを残そう
(必要な場合は)READMEやCHANGELOGなども更新されているか?
セキュリティ
APIエンドポイントは適切な認証・認可がされているか?
よくある脆弱性の対策がされているか?
姿勢
レビューは、コードを書いた人ではなくコードを批判すべきだ
分かりにくいところがあれば、無知を指摘するのではなく説明を求めよう。
「あなたの変更の前には動いていたのに」とか「あなたのメソッドにはバグがある」とかいった、所有格代名詞を使ってはいけない。
「提案」「マストな修正」「議論が必要な点」がハッキリ区別できるように
Googleコードレビュー