Code Review Checklist: What I Look for After 10 Years of PRs \u2014 TXT1.ai

3年前、私は私の会社に1回の週末で47,000ドルの損失をもたらすプルリクエストを承認しました。コードは問題ないように見えました。テストは合格しました。論理的には正当でした。しかし、私は欧州の顧客向けのタイムゾーン変換の取り扱いにおいて微妙な何かを見逃し、夏時間が開始されたとき、私たちの決済処理システムはユーザーベースの18%に対して静かに失敗し始めました。

この事件は、私がコードをレビューする方法を永遠に変えました。私はサラ・チェンで、過去10年間に3つの異なるSaaS企業で上級エンジニアリングマネージャーを務めており、毎週平均15〜20のプルリクエストをレビューしています。これは私のキャリアの中で約8,000のPRに相当し、単一行のホットフィックスから50以上のファイルにわたる大規模なアーキテクチャ改修までの範囲です。私はバグを含む素晴らしいコードや、数年間問題なく運用された雑なコードを見てきました。

私が学んだことは、コードレビューは実際にはバグを見つけることではなく（それも重要ですが）、現実に接触しても生き残るシステムを構築することに関するものであるということです。それは次の開発者（多くの場合は自分自身で、6ヶ月後）に理解し、恐れずに修正できるコードを作成することに関するものです。そして、それはアプリケーションがスケールしたとき、エッジケースが出現したとき、またはオーストラリアの特定の顧客が予想外のことをしたときにのみ明らかになる見えない問題を捉えることでもあります。

これが私のチェックリストです。教科書からの理論的なものではなく、数え切れない本番環境の事故や深夜のデバッグセッションを通じて洗練した戦闘用のものです。承認したバグが実際のユーザーに影響を与えていることに気づいたときの沈み込むような感情も含まれます。

最初の30秒: PRの説明が私に伝えること

コードの1行を見始める前に、私はPRの説明を30秒間読みます。これは些細に思えるかもしれませんが、適切に書かれていない説明は、コード自体に問題がある可能性を示す最初のレッドフラグです。私の経験では、自分のコードが何をしているのかを明確に説明できない開発者は、実装を十分に考え抜いていないことが多いです。

私は具体的に3つのことを探しています: 解決される問題、採用されたアプローチ、そして行われたトレードオフ。良いPRの説明はミニ設計文書のように読み取れます。「ユーザー認証のバグを修正」という説明は何も教えてくれません。「高トラフィック期間中に0.3%のユーザーが予期せずログアウトする原因となったJWTトークンリフレッシュの競合状態を修正するために、Redisを使用して分散ロックを実装した」という説明は、開発者が問題を深く理解していることを示します。

PRのサイズが適切であるかも確認します。私の経験則: PRが400行以上のコードに及ぶ場合（テストと生成ファイルを除く）、効果的にレビューするにはおそらく大きすぎます。Googleのエンジニアリングプラクティスの研究によると、レビューの効果が200〜400行を超えると著しく低下することが示唆されています。私はこれが正確であると認識しています。約300行をレビューすると、私の注意が散漫になり、微妙な問題を見逃し始めます。

大規模なPRに出くわした際には、開発者にそれを小さく論理的な部分に分けるように頼みます。はい、これには最初に時間がかかりますが、私は圧倒されていて通過したいがために2,000行のPRを承認するというシナリオを防ぎます。私はキャリアの初期にこの方法で多くの問題のあるPRを承認しました。

説明には関連するコンテキストのリンクも含めるべきです: 取り組まれているチケットや問題、設計文書、関連するPRなどです。この変更がなぜ行われているのかを理解するためにJiraやSlackを探し回る必要がある場合、それはレビュー全体のプロセスを遅延させる摩擦です。私の現在の会社では、適切なコンテキストがないPRは、説明が更新されるまでレビューされないという方針を取っています。このシンプルなルールは、私たちのレビューの質を大幅に向上させました。

論理レイヤー: これは実際に問題を解決しますか？

PRが実際に何をする予定かを理解したら、それが本当にそうするか確認します。これは明白に思えますが、テストを通過したコードが根本的な問題に完全に対応していないことをどれだけ頻繁に驚かされるか分かりません。私は、開発者が症状を修正しつつ根本的な原因を放置しているのを見てきました。それは後に爆発する時限爆弾を作ります。

「コードレビューは本当にバグを見つけることではなく、現実に接触しても生き残るシステムを構築し、次の開発者が恐れずに理解できるコードを作成することに関するものです。」

私はまず、ハッピーパスを精神的に歩いてみます。これは返金処理のための新機能であるとすると、顧客が返金を要求し、APIエンドポイントからビジネスロジックを通じてデータベースの更新までのコードパスを辿ります。各ステップは意味がありますか？データは正しく処理されていますか？必要なすべてのレコードを更新していますか？

しかし、ハッピーパスは簡単です。良いコードと本番向けのコードを区別するのは、アンハッピーパスの処理方法です。決済ゲートウェイがダウンした場合はどうなりますか？返金額が負の場合はどうしますか？すでに返金されている注文の返金リクエストをユーザーが行った場合はどうなりますか？私はエッジケースに対して非常に神経質になっています。なぜなら、本番環境はあなたが考慮しなかったすべてのエッジケースを見つけ出す混沌のエンジンだからです。

私は防御的プログラミングパターンを探します: nullチェック、入力バリデーション、境界条件処理。しかし、私はまた、行き過ぎたエンジニアリングも探します。すべての関数がすべての可能なエッジケースを処理する必要はありません。プライベートメソッドが唯一の場所から検証済みの入力で呼び出されるだけの場合、冗長なバリデーションを追加することはノイズに過ぎません。重要なのは、各関数の契約を理解し、その境界でそれが守られていることを確認することです。

何度も問題を引き起こすパターンの1つは、「楽観的更新」です。開発者は操作が成功すると仮定し、成功を確認する前にUIまたはデータベースの状態を更新します。これは99%の確率でうまくいきますが、その1%はシステム状態が不整合になる非常に混乱するバグを生み出します。私はこれらのパターンを常にフラグ付けし、次のように尋ねます: この操作が途中で失敗したらどうなりますか？

データレイヤー: 情報の流れに従う

データバグは最悪の種類のバグです。なぜなら、それらはしばしば静かで累積的だからです。論理エラーはアプリケーションを即座にクラッシュさせるかもしれませんが、データエラーは数週間かけてデータベースを徐々に破損し、気づいたときには既に何千もの不正なレコードがあり、それらを修正するクリーンな方法がないのです。

データに触れるコードをレビューする際、私はそのデータの全生涯を追います。どこから来たのか？どのように検証されているのか？どのように変換されているのか？どこに保存されているのか？誰がアクセスできるのか？これは、エラーが現実的な結果を持つユーザー生成コンテンツや財務データにとって特に重要です。

データ型変換には特に注意を払います。私はかつて、計算のために価格を浮動小数点から整数に変換し、再度浮動小数点に戻すコードを承認しました。無害に見えました。しかし、浮動小数点の精度の問題により、$10.99のような価格が時々$10.98や$11.00に変わることがありました。私たちは顧客から不正確な請求についてクレームが来るまでそれに気づきませんでした。今では、特にお金や測定値に関わるすべての型変換を精査しています。

データベースの移行も私が特に慎重になる分野です。私はいくつかのことを探します: 移行は逆転可能ですか？途中で失敗した場合はどうなりますか？私たちの本番データベースでどのくらいの時間がかかりますか？1,000レコードの開発データベースで問題なく動作した移行が、5,000万レコードの本番データベースでテーブルを20分間ロックし、アプリケーション全体をダウンさせるのを見たことがあります。

既存のデータを変更するコードがある場合、私は次のように尋ねます: バックアッププランはありますか？何かがうまくいかない場合、これを元に戻せますか？私の前の会社では、10,000レコード以上に影響を与えるデータ移行は、本番スナップショットでのドライランと明示的なロールバック手順をPRに文書化することが必要という方針がありました。これは何度も私たちを救ってくれました。

データアクセスパターンも見ます。このコードはN+1クエリを行っていませんか？必要なフィールドだけを使用する際に、全コレクションをメモリにロードしていますか？これらのパフォーマンス問題は開発中には明白ではないかもしれませんが、スケールに達すると重要になります。私は、コードが問題なく見えたとしても、もし本番に出荷されていたらデータベースのCPUが100%に上昇していたであろうコードをレビューしたことがあります。