Code Review Checklist: What I Look for After 10 Years of PRs \u2014 COD-AI.com

3年前、私は、私の会社にとって47,000ドルの損失をもたらしたプルリクエストを承認しました。それはたったの週末の間に起こりました。コードは問題なさそうに見えました。テストはパスしました。論理も十分でした。しかし、私たちのサブスクリプション請求システムでのタイムゾーン変換の扱いについて微妙な部分を見逃してしまい、特定の地域の顧客が二重で請求されてしまいました。

この事件は、私のコードレビューの仕方を永遠に変えました。私の名前はサラ・チェンで、過去10年間に3つの異なるSaaS企業でシニアエンジニアリングマネージャーを務め、週に15～20件のプルリクエストをレビューしています。これはキャリアの中で約8,000件のPRに相当します。私はバグを持った素晴らしいコードや、何年も問題なく稼働した混沌としたコードを見てきました。私は、コードレビューは構文エラーを見つけることではなく、経験でしか発見できない見えない問題を捉えることだと学びました。

この記事は、私が最初に持っていたかったチェックリストです。包括的ではありません—どんなチェックリストでもそうなることはありませんが、私が認識するよう学んだパターン、私が自分に質問するよう訓練した質問、そして私のチームを無数の本番事故から救ってくれたメンタルモデルを示しています。

最初の30秒：PRの説明が教えてくれること

私は1行のコードを見る前に、PRの説明を30秒間読んでいます。これは逆に思えるかもしれませんが、問題のあるPRの60％が不十分なコミュニケーションを通じてすぐに明らかになることを私は発見しました。良いPRの説明は、3つの質問に答えます：何が変更されたのか、なぜ変更されたのか、そして何が悪化する可能性があるのか。

「バグを修正」や「コンポーネントを更新」と書かれている説明を見ると、私はすぐにこのレビューには時間がかかることを理解します。著者は自分の変更の影響を考えていないため、私がその作業を代わりにしなければならないということです。逆に、「ユーザー認証をJWTトークンを使用するようにリファクタリングしました。これにより、ピーク時にデータベースの負荷を40％削減しますが、クライアントはトークンのリフレッシュを処理する必要があります。リスク：既存のモバイルアプリ版本 (< 2.3) は再認証が必要です」という説明を見ると、著者が宿題をしっかりやったことが分かります。

私は説明の中に具体的なメトリクスを探します。漠然とした改善ではなく、実際の数字です。「パフォーマンスを改善しました」は何の意味もありません。「ユーザープロファイルエンドポイントで、1000の同時リクエスト下で340msから180msにAPIの応答時間を短縮しました」は、著者が自分の作業を測定し、その影響を理解していることを示しています。私の経験では、PRの説明にメトリクスを含む開発者は、全体的により思慮深いコードを書く傾向があります。

説明は、関連するコンテキストへのリンクも含むべきです—チケット、デザイン文書、アプローチが議論されたSlackスレッドなどです。もし私がより広いコンテキストを理解せずに孤立してPRをレビューしていると、私は何かを見逃すことになります。かつて私は、「単純なリファクタリング」を承認し、それが文書化されていない依存関係について知らなかったために、クリティカルな統合を知らずに壊してしまいました。それは3か月前のメールスレッドにしか記録されていませんでした。

PRの説明での赤旗には、まったく説明がないこと、コード変更そのものよりも長い説明（通常は過剰設計を示す）、PRがレビューの準備ができているとマークされているが「WIP」または「ドラフト」と書かれているもの、そして「これが適切なアプローチかどうかは分からない」というフレーズが含まれている説明があります（それなら、なぜ私にレビューを頼むのですか？）。

サイズが重要：400行ルール

私は厳格なルールを持っています：実際のコード変更が400行を超えるプルリクエストを徹底的にレビューしないこと（生成されたコード、パッケージロックファイル、テストフィクスチャーは除外）。これは恣意的ではありません。CiscoとSmartBearからの研究は、コードレビューの効果が400行を超えると劇的に低下することを示しており、私自身の経験もこれを裏付けています。約200行のコードをレビューした後、私の脳は詳細を見落とし始めます。400行に達すると、私は基本的にスキミングしているだけです。

"コードレビューは構文エラーを見つけることではありません—あなたのリンターがそれをします。経験でしか発見できない見えない問題を捉えることです。"

もし誰かが1,200行のPRを提出してきたら、私は彼らにそれを分割するように頼みます。「すべてが関連している」と言われても、私は気にしません。大きなPRはバグが隠れる場所です。私は、レビュアーが疲れて600行目あたりで注意を払わなくなり、大規模なリファクタリングPRの中で重大なセキュリティ脆弱性が見逃されるのを見てきました。その脆弱性は847行目にありました。

もちろん、例外もあります。時にはファイルを移動させたり、生成コードを更新したり、新しいリンティング基準を満たすために大規模な変更を加える必要があります。その場合、私は著者に機械的な変更を論理的な変更から分けるように頼みます。ファイル移動を1つのPR、実際の論理変更を別のPRで提出します。これにより、両方のPRはレビューしやすく、何か問題が発生した際にも元に戻しやすくなります。

私はまた、追加されたコードと削除されたコードの比率にも注意を払います。私の経験では、最良のPRは負のネット行数を持っており、コードベースを小さくしながら何かを達成します。500行追加して50行削除するPRを見ると、私は疑いを持ちます。複雑さが追加されていますか？既存の機能が重複していますか？著者はコードベースに何が既にあるのかを理解していますか？

サイズの問題は、個々のファイルにも拡張されます。もしPRが30の異なるファイルに触れている場合、合計行数が合理的であっても、それは赤旗です。それは、変更が不適切にスコープされているか、コードベースに結合の問題があることを示唆します。どちらにしても、追加の精査が必要です。

ビジネスロジック層：ほとんどのバグが潜む場所

10年間のコードレビューを経て、私はバグがどこに隠れているかをお伝えできます：ビジネスロジック層、特に境界条件や状態遷移にあります。UIコンポーネントやデータベースクエリにはありません。実際のビジネスルールを実装するコードにあります。

ビジネスロジックをレビューする際、私は仮定を探します。すべての仮定は潜在的なバグです。私は、一度、すべての割引がパーセンテージであると仮定されている割引計算システムのコードをレビューしました。マーケティングが「10ドルオフ」のプロモーションを提供したいと言ったとき、コードは完璧に機能しましたが、システムはクラッシュしました。なぜなら、誰かが割引額で割ったからで、0から1の間の数字を期待しているときに10で割ると非常に間違った結果になります。

私は自分に聞きます：境界で何が起こりますか？入力がゼロの場合は？負の場合は？ヌルの場合は？空の文字列とヌルの文字列の場合は？配列が空の場合は？ユーザーに権限がない場合は？すべての権限がある場合は？データベースが結果を返さない場合は？100万件の結果を返す場合は？

私はビジネスロジックにおける魔法の数字を探します。if (user.loginAttempts > 5)のような場合、私は「なぜ5なのか？」と尋ねます。それは文書化されていますか？設定可能ですか？特定のユーザータイプのために3に変更したい場合はどうしますか？ビジネスロジックにおける魔法の数字は、発生を待つ技術的負債です。

状態遷移を管理するコードも、バグの一般的な原因です—注文の状態、ユーザーライフサイクル、ワークフローステージなど。私は頭の中で状態図を描きます。すべての遷移が把握されていますか？無効な状態に入ることはできますか？一度、ユーザーが「アクティブ」と「一時停止」の両方の状態になるバグを見つけました。なぜなら、一方の状態を設定するコードが他方を確認していなかったからです。

私はまた、複数の層にまたがるビジネスロジックにも注意を払っています。もし私がUIにバリデーションを、APIにさらにバリデーションを、そしてデータベース層にもバリデーションを見たら、一貫性が欠けることになることがわかります。ビジネスルールは一つの場所に存在すべきであり、その場所は明確であるべきです。