2000のプルリクエスト後に作成したコードレビューチェックリスト

私は18ヶ月間にわたり、2147のPRコメントを分類しました。34%はエラーハンドリングに関するもので、22%は命名に関するものでした。パフォーマンスに関するのはわずか8%でした。レビューを追跡し始めたとき、私が予想していたのはアーキテクチャの問題や複雑なバグを捉えることでした。しかし、私は繰り返し同じ基本的な問題を指摘していました：ヌルチェックの欠如、あいまいな変数名、およびユーザーに何の役にも立たないエラーメッセージ。これらの問題が本番のインシデントで浮上するのを見て、私はこれが小さな細部の問題ではなく、優雅に失敗するシステムと夜中の3時にあなたを起こすシステムとの違いであることに気付きました。

ほとんどのコードレビューのチェックリストが見落とすポイント

みんながコードスタイル、テストカバレッジ、およびドキュメントをチェックするように言います。それは良いですが、本番が壊れる場所ではありません。エンジニアリングブログで共有されるチェックリストは、実際に重要なことではなく、測定が簡単なことに焦点を当てています。彼らは「関数が50行未満であることを確認する」と言いますが、ログが唯一の友人である深夜2時にエラーハンドリングが実際に問題のデバッグを助けるかどうかを確認するようには言いません。

最高のコードレビューはバグを防ぐだけでなく、インシデントに繋がる種類のバグを防ぎます。ヌルチェックの欠如は単なる潜在的なクラッシュではなく、そのクラッシュがトランザクションの途中で発生した場合の潜在的なデータ損失イベントです。

私がPRコメントを追跡し始めたのは、イライラしていたからです。私はコードをレビューし、承認し、その後同じ開発者が次のPRで同じ間違いをするのを見ました。フィードバックが定着していなかったのです。そこで私はスプレッドシートを作成しました。私が残した全てのコメントは、エラーハンドリング、命名、テスト、セキュリティ、パフォーマンス、アーキテクチャ、またはその他に分類されました。6ヶ月後、パターンを見るための十分なデータが集まりました。18ヶ月後、それらのパターンは実際に機能するチェックリストに固まっていました。

驚いたことは、リストのトップに何があるかだけでなく、何がないかでした。パフォーマンス最適化に関するコメントは、私のレビューのわずか8%を占めていました。セキュリティの問題は6%でした。これらは私たちがアーキテクチャ討論でこだわることですが、日常のPRではめったに見られません。一般的なのは、開発者がハッピーパスを仮定すること、名付けが不適切であること、および深夜にデバッグをする人のためではなく、自分のためにエラーメッセージを書くことです。

本番を壊し続けるエラーハンドリングのパターン

ここに、実際のインシデントを引き起こす頻度に基づいてランク付けされたエラーハンドリングの問題の番号付きリストがあります：

1. バックグラウンドジョブのサイレント失敗。 コードは例外をキャッチし、ログを取り、続行します。これは受け入れられそうですが、そのジョブは支払い確認メールを送信する予定だったことに気付くと、そうではありません。今、あなたの顧客は料金が請求されていないと思っていますが、請求されています。このパターンは40%のバックグラウンドジョブのPRに見られます。修正は簡単です：操作が重大な場合は、例外をキャッチせず、バブルアップしてアラートをトリガーさせます。重要でない場合は、サイレントに失敗する理由を文書化してください。
2. コンテキストを隠す一般的なエラーメッセージ。 「エラーが発生しました」は何も教えてくれません。「支払いの処理に失敗しました」はより良いですが、まだ役に立ちません。「4242で終わるカードの請求に失敗しました：残高不足（エラーコード：card_declined）」は実際に役立ちます。これを約30%のPRでフラグします。私が使用するテスト：午前3時に本番のログでこのエラーを見た場合、追加のログを追加せずに診断できるでしょうか？
3. 特定の例外の代わりにExceptionをキャッチする。 これは物議を醸す問題ですが、スタイルガイドがそれを推奨することもありますが、私はそれがバグを隠すのをあまりにも多く見てきました。Exceptionをキャッチすると、NullPointerException、IllegalStateException、およびプログラマーエラーを示すすべての他の例外もキャッチしています。あなたが処理することを予想しているものをキャッチしてください。プログラマーエラーはクラッシュさせるべきです。それが見つける方法です。
4. バウンダリで外部データを検証しない。 APIレスポンス、ユーザー入力、ファイル内容—それがプロセスの外部から来る場合は、即座に検証してください。私は開発者がビジネスロジック層で検証しているのを見かけます。これは無効なデータがすでにいくつかの関数を通過することを意味します。その時点で、なぜヌル値がデータベースに入ったのかをデバッグしています。バウンダリで検証し、早期に失敗し、明確なエラーを返してください。
5. 指数バックオフなしのリトライロジック。 サービスがダウンしているので、即座にリトライします。まだダウンしているので、再び即座にリトライします。おめでとうございます、リトライによってサービスの劣化を完全な停止に変えてしましました。私はリトライロジックを追加するPRの15%でこれを見かけます。常にじっくりリトライと指数バックオフを使用してください。常に最大リトライカウントを設定してください。この操作のためにリトライすることが妥当かどうかも考慮してください。
6. バッチ操作で部分的な失敗を処理しない。 あなたは1000レコードを処理しています。レコード500が失敗します。レコード501-1000はどうなりますか？ 私がレビューするほとんどのコードでは、これらは決して処理されません。バッチが失敗し、リトライされ、再びレコード500で失敗し、あなたは詰まります。部分的な失敗を明示的に処理してください：成功したもの、失敗したもの、そしてその理由を追跡します。バッチ操作を再開可能にしてください。
7. データベーストランザクションが常に成功することを仮定する。 トランザクションを開始し、作業を行い、コミットします。しかし、コミットが失敗した場合はどうなりますか？ トランザクション途中でデータベース接続を失ったら？ 私がデータベースコードに触れる25%のPRの中でこれを処理していないコードを見かけます。その結果、アプリケーションの状態とデータベースの状態が食い違い、ログが示していることとデータが一致しない理由をデバッグするのに数時間を費やします。

変数名が$50,000のインシデントを引き起こした日

火曜日の朝、私はSlackのメッセージを受け取りました：「私たちは間違った顧客に$50,000を返金しました。」私はインシデントチャンネルを開き、読み始めました。ある開発者が私たちの返金処理システムのバグの修正を前の晩にプッシュしました。修正は1行でした。PRは2人のシニアエンジニアによって承認されていました。テストは通過しました。すべてが正常に見えました。

バグは変数名にありました。元のコードには返金額をセントで表す`refundAmount`という変数がありました。開発者はドルで表す`refundAmount`という新しい変数を追加しました。彼らは元の変数の名前を変更するのを忘れていました。コードは正常にコンパイルされました—両方とも整数でした。テストデータはたまたまセントとドルの間が近いため、アサーションがそれをキャッチしませんでした。

本番環境では、その朝に200件の返金を処理しました。その半数は誤った金額でした。$10.00の返金が$1,000の返金になり、$5.00の返金が$500の返金になりました。誰かが気付く頃には、$50,000もオーバーペイされていました。私たちはすべての返金を手動で確認し、顧客に連絡し、場合によってはお金を返してもらうように頼まなければなりませんでした。清算に3日かかりました。

根本的な原因は開発者の間違いではありませんでした—誰でも間違いを犯します。それは、同じスコープ内で異なる単位を表す同じ名前の2つの変数があったことでした。コードレビューはそれをキャッチしませんでした。レビューアはロジックに集中していて、命名を見ていなかったからです。そのインシデントの後、私はチェックリストにルールを追加しました：もし変数が単位を持つ量を表すのであれば、その単位は名前に含まれなければなりません。`amount`ではなく、`amountCents`または`amountDollars`。`duration`ではなく、`durationSeconds`または`durationMilliseconds`。

これは細かすぎるように思えるかもしれませんが、でも重要です。