Code Review Best Practices: How to Review (and Be Reviewed) — txt1.ai

私は、ソフトウェアエンジニアリングを辞めたくなるほどのコードレビューを未だに覚えています。それは2012年で、私はフィンテックのスタートアップでの最初の仕事に就いてから6ヶ月が経過し、私たちの決済処理システムを素晴らしくリファクタリングしたと思ってコードを提出したばかりでした。シニアエンジニアのレビューが返ってきて、そのコメントは47件—ほとんどが「これは間違っている」のバリエーションで、説明もありませんでした。私は全てを書き直すのに3日かかり、その同じレビュアーが「良い」と一言だけで承認しました。その経験は、より良いコードを書くことについて何も教えてくれませんでしたが、コードレビューを行う際にどうしないべきかについてすべてを教えてくれました。

12年後の今、私はCシリーズのSaaS企業でプリンシパルエンジニアとして働いており、8,000以上のプルリクエストをレビューし、40人以上の開発者をコードレビューのプロセスを通して指導してきました。私は、コードレビューが会社を壊滅的なバグから救うのを見てきましたし、それがチームの士気を根底から崩壊させるのを目の当たりにしました。完全なエンジニアリング部門が6ヶ月以内にひっくり返りました。その違いは、レビューされるコードの質ではなく、レビューのプロセスそのものの質です。

コードレビューは、ソフトウェア開発において最も強力なツールの一つであり、最も頻繁に誤用されるものでもあります。SmartBearの2023年のコードレビューに関するレポートによると、構造化されたコードレビューのプロセスを実施しているチームは、本番環境前に60〜90%の欠陥を発見していますが、同じ調査によると73%の開発者が信頼やチームメンバーとの関係に悪影響を及ぼしたコードレビューの否定的な経験を報告しています。この逆説は、ほとんどのチームが何をレビューするかに集中し、どのようにレビューするかには集中しないために存在します。

コードレビューの心理学: なぜほとんどのレビューは開始前に失敗するのか

ここに誰も言わないことがあります。コードレビューは主にコードに関するものではなく、人に関するものです。すべてのプルリクエストは、誰かの知的努力、創造的な問題解決、そして職業的アイデンティティの数時間を表しています。コードをレビューする際、あなたは論理や構文を評価しているだけでなく、他の人の作品と関わっているのです。それは、彼らを高めるか、ダメにするかのどちらかです。

私は、私たちのチームを去った才能あるジュニア開発者との退職インタビューを行った後、このことを痛感しました。彼女は、たった一つの軽視的なコードレビューのコメント「なぜこんな風にやるのですか？」が、彼女に工学に属しているのか疑問を抱かせたと教えてくれました。レビュー担当者は、彼女の理由を知りたいという真剣な疑問だったのですが、彼女はそれを判断として解釈しました。その時、非同期のテキストの媒体がトーン、表情、ボディーランゲージを取り去り、最悪の解釈ができる言葉だけが残ることを私は理解しました。

心理学的研究もこれを裏付けています。2021年にIEEE Transactions on Software Engineeringに掲載された研究は、厳しいまたは軽視的と見なされるコードレビューコメントが、マージまでの平均時間を3.2日増加させ、著者が将来的な改善に貢献する可能性を34%減少させることを発見しました。逆に、具体的な賞賛を含むレビューは、28%早い反復サイクルと41%高い品質のコードが同じ著者からの後の提出において結果をもたらしました。

これは、すべてを甘く見るべきだとか、問題を指摘することを避けるべきだという意味ではありません。それは、反対側の人間に対して意図をもってコードレビューを行う必要があるということを意味します。レビューコメントを書く前に、私は自分に3つの質問をします。これは本当ですか？これは必要ですか？これは親切ですか？この3つすべてに「はい」と答えられない場合、コメントを書き直します。このシンプルなフィルターが、私のチームのコミュニケーションを変革し、過去2年間で私たちの平均PRサイクルタイムを4.3日から1.8日に削減しました。

レビュアーのチェックリスト: 実際に何を探すべきか

新しいレビュアーを訓練するとき、彼らはいつも同じ質問をします。「私は何を探していればいいですか？」その答えは、文法ルールやスタイルガイドのシンプルなリストではありません—それらは自動化すべきです。人間のレビュアーとしてのあなたの仕事は、機械には評価できないもの、設計上の決定、保守性、そしてビジネスロジックの正確性を評価することです。

私は、4段階の優先順位システムを使用して、レビューエネルギーを最も重要なところに集中させています。Tier 1の問題はブロッカーです: セキュリティの脆弱性、データ損失のリスク、または本番環境でのインシデントを引き起こす可能性のある破壊的変更です。これらは、リスクの明確な説明と共に、直ちにフラグが立てられます。私の経験では、真のTier 1の問題は、プルリクエストの5%未満に現れますが、現れた時には、それを見つけることが私たちがコードレビューを行う全ての理由です。

Tier 2の問題は、アーキテクチャ上の懸念事項です: 動作するが、技術的負債を引き起こし、確立されたパターンを逸脱し、将来の変更を困難にするコードです。これは、現在のコードベースとチームの将来の方向性の両方を理解する必要があるため、レビューが最も難しい問題です。これを指示ではなく質問としてフレーミングする効果的な方法を見つけています: 「このアプローチは、次の四半期に機能Xを実装するのを困難にするかもしれません—代わりにパターンYを使用することを考えてみませんか？」これは防衛的態度ではなく、議論を促します。

Tier 3の問題は、可読性や保守性の向上に関するものです: 不明瞭な変数名、複雑なロジックへのコメントの欠如、または明確にするために分解できる機能です。これらは重要ですが、緊急ではありません。私は通常、レビューあたり3つのTier 3のコメントに制限し、将来の保守性に最も大きな影響を与える分野に焦点を当てます。それ以上になると、著者が圧倒されてフィードバックとの関わりを辞めてしまいます。

Tier 4はその他すべてです: スタイルの好み、必ずしもより良いわけではない代替のアプローチ、フォーマットに関する小さな指摘です。私の論争のある意見: Tier 4のコメントはほとんど残すべきではありません。標準化する価値があるなら、それをリンターやスタイルガイドに追加してください。自動化する価値がないなら、プルリクエストを遅らせる価値はありません。私は、実際のロジックエラーを含むコードを出荷する一方で、単一引用符か二重引用符の使用について数時間議論するチームを見てきました。あなたもそのチームにならないでください。

建設的なコメントの技法

役立つコードレビューのコメントと士気を低下させるコメントの違いは、しばしば数語の違いに帰着します。同じコードの部分に対する次の二つのコメントを比較してください:

"この関数は長すぎて、あまりにも多くのことを行っています。" "この関数はバリデーションとデータ変換の両方を処理しているため、各懸念点を独立してテストするのが難しくなります。validateUserInput() と transformToApiFormat() に分割することを検討してください—そうすれば、変換をモックすることなくバリデーションロジックをテストでき、その逆も可能です。"

最初のコメントは技術的には正しいですが、役に立ちません。著者に何が間違っているのかを伝えるだけで、なぜそれが重要なのか、どう修正するのかは伝えません。2番目のコメントは問題を説明し、影響を述べ、具体的な解決策を提案しています。それには私がかつてのように時間をかけました。