Nguyen Le Phong

やさしく効果的なエンジニアリング全 5 回中第 1 回

コードレビューという技術:人を傷つけずにコードを批評する方法

コードレビューは、チームカルチャーが育まれるか壊れるかの分岐点です。コードをより良く仕上げながら、書いた人がより成長して戻ってこられるレビューの実践ガイド——具体的なフレーズ、レビュアーのチェックリスト、そしてレビューを静かに有害にしてしまう習慣を紹介します。

こんな経験はありませんか。月曜の朝にプルリクエストを開く——ここ数ヶ月で一番きれいに書けたコードだと自信満々で。でも午後になると、たった一つのコメントで、次のデプロイが恐ろしくなってしまう。フィードバックが間違っていたわけではありません。でも、届け方の問題だったのです。「これは不必要に複雑です。なぜこうしたのですか?」七文字。具体的なアドバイスはゼロ。それだけで、週全体の気分が台無しになってしまう。

コードレビューは、あらゆるエンジニアリングチームで最も高い影響力を持つ習慣です。うまくやれば、より良いソフトウェアを届け、エンジニアを速く成長させ、信頼の文化をひっそりと育てます。うまくやれなければ、燃え尽き症候群を招き、デリバリーを遅らせ、優秀なエンジニアをプルリクエストのたびに自分を守らなくて済むチームへと追い出してしまいます。このガイドは「うまくやる方法」についてのものです——具体的なフレーズ、レビュアーのチェックリスト、レビューを静かに有害にしてしまう習慣、そして自分がコメントを受け取る側になったときの対処法を紹介します。

コードレビューは文化であり、バグ探しだけではない

コードレビューが何のためにあるのか、よく誤解されています。技術的な目的——バグが本番に届く前に見つけること——は確かに大切です。でも、それはストーリーの一部に過ぎません。

あなたが残すコメントのひとつひとつが、何かを教えます。コードベースについて、トレードオフについて、チームが大切にしている基準について、著者に伝えます。何百ものレビューを経るうちに、あなたはチームの集合記憶を言語化した存在になります。新しいエンジニアはあなたのコメントを読んで「良いコード」とはここではどういうものかを学びます。ジュニアエンジニアはレビュアーが一貫して問いかける問いを内面化してシニアになっていきます。シニアは自分の直感の「なぜ」を言語化することで切れ味を保ちます。

レビューはまた、目に見えないところで規範を作ります。優しく、具体的で、好奇心あふれるコメントを一貫して残すチームは、不完全なものを出して学ぶことが心理的に安全な場所になります。ぶっきらぼうで、見下すような、あるいは暗号めいたコメントを残すチームは、批判を避けるために沈黙の中で過剰設計し、プルリクエストを開くのを先延ばしにし、質問をやめてしまう場所になります。

悪いレビュー文化の本当のコスト

エンジニアがプルリクエストを開くことを恐れると、レビューの回数を最小化するためにますます大きな変更をまとめてしまいます。大きなプルリクエストはきちんとレビューしにくく、レビューの質が下がり、フィードバックがより厳しくなり、悪循環が深まります。解決策はより少ない厳しさではなく——より多くの優しさを厳しさに添えることです。

これはコードレビューを甘くしたり、まったく指摘しないということではありません。最も効果的なレビュアーは深く正直です。問題をはっきりと指摘します。本当に大切なことについては一線を守ります。でも、著者が裁判官に裁かれた感覚ではなく、協力者を得た感覚を持てるような方法でそれをします。

本当にレビューすべきこと——そして見逃して良いこと

最もよくあるレビューの間違いのひとつは、すべてに同じ比重をかけてレビューすることです。末尾のカンマがないことでプルリクエストをブロックするレビュアーは、SQLインジェクションを指摘するレビュアーと同じ拒否権を持っています。この非対称性はすぐに信頼を損ないます。まず自分の頭の中で、何が本当に重要なのかを明確にすることから始めましょう:

  • 正しさ:このコードはやるべきことをやっていますか?エッジケースは処理されていますか?テストがカバーしていない方法で本番で失敗しうる箇所はありますか?
  • 設計:この変更は既存のアーキテクチャに自然に収まりますか?不必要な結合を生み出していませんか?同じ問題をよりシンプルに解くアプローチがありますか?
  • 読みやすさ:これを書いていないチームメンバーが六ヶ月後に理解できますか?名前は明確ですか?流れは追いやすいですか?
  • テスト:テストは意味がありますか?ハッピーパス、エッジケース、失敗パターンをカバーしていますか?テストが失敗したとき、実際に何か有用なことを教えてくれますか?
  • セキュリティ:ユーザーの入力はバリデーションされていますか?データの漏洩、権限昇格、インジェクションが生じうる箇所はありますか?(もしそうなら、ブロッカーとして扱ってください。)

レビューのエネルギーを使わなくていいこと:

  • フォーマットとスタイルはリンターやフォーマッターが自動的に処理するもの。CIが検出するなら、あなたが指摘する必要はありません。人間によるレビューの時間は貴重です——機械が処理することに費やさないでください。
  • チームの標準でない個人の好み。「私なら別の名前にした」はレビューコメントではありません。「この名前はドメイン全体のユビキタス言語と一致していない」は、場合によっては、コメントになりえます。
  • プルリクエストのコメントスレッドでのアーキテクチャ論争。方向性について根本的な意見の相違があるなら、それはコードレビューではなく設計についての会話です。次のプルリクエストが始まる前に、別途かつ協働的に行ってください。
役立つフィルター

コメントを書く前に問いかけましょう:「著者がこの通りに変更したら、コードは意味のある形で良くなるか?」答えが「少し」か「たぶん」なら、著者の時間とあなたの時間をかける価値があるか考えてみてください。答えが「はい、そしてその理由はこうです」なら、書きましょう——でも、優しく書いてください。

コメントを優しく、それでも正直に表現する方法

コードレビューで最も大きなレバーは、言葉の選び方です。同じ懸念でも、二通りの届け方をすると、読んだ相手がまったく異なる反応をします。実践的なツールキットをお伝えします:

  • 判決を下すのではなく、質問をする。「ここでXではなくYを選んだのはなぜですか?」は会話への招待です。「Yを使うべきでした」はそれを閉じます。質問によって、あなたが持っていなかったコンテキストが明らかになるかもしれません——たとえそうならなくても、著者は責められた感覚を持たずに同じ結論に至れます。
  • 「何を」ではなく「なぜ」を説明する。「このロジックをサービスに移せますか?」は、「このロジックをサービスに移せますか?コントローラーに置いたままだと分離してテストしにくいですし、cronジョブが同じ動作を必要とする場合に重複してしまいます」よりも弱いです。「なぜ」は教え、「何を」は指示するだけです。
  • 批判だけでなく提案を提供する。問題が見えたら、代わりに何をするかを示しましょう——少なくともその形だけでも。「確信はないですが、こんな感じで…」は「これは正しくないように思います」よりはるかに有用です。
  • 良いコードを明示的に褒める。「このエラーハンドリングは本当にきれいです——このパターンを拝借します」と打つのに10秒かかり、コストはゼロです。著者の自信を支え、そのパターンをチームの標準として強化し、批判的なコメントが来たときにそれを受け取りやすくします。
  • 正直な範囲で「私たち」「私たちの」を使う。「私たちの慣例ではこれをリポジトリの近くに置きます」は「あなたはこれを間違った場所に置いています」よりも柔らかく、しかし同じくらい正確です。

厳しい表現と優しい表現:同じ指摘、二つの言い方

厳しい表現(しかし技術的には正しい) 優しい——そして正直な——表現
「これは間違っています。nullケースを処理する必要があります。」 userがnullだとここでスローします——ガード節を追加するか、呼び出し元がnon-nullを保証することをドキュメントに書くかできますか?」
「なぜこうしたのですか?不必要に複雑です。」 「これを追うのが難しいと感じています——もっとシンプルな表現方法はないでしょうか?私が何かコンテキストを見落としているかもしれません。話し合いたければ喜んで。」
「この関数はやりすぎです。分割してください。」 「この関数はバリデーションと永続化の両方を担っています——それぞれの責務を分けると、各部分を独立してテストしやすくなるかもしれません。どう思いますか?」
「明らかにテストしていませんね。」 「空入力ケースのテストが見当たらなかったのですが——意図的なものですか、それともフォローアップチケットを作りましょうか?」
「ここはletでなくconstを使ってください。」 「nit: 再代入されないのでconstの方が意図が明確になります。」
「これはセキュリティホールです。」 「blocking: このインプットはユーザー提供でそのままクエリに入っています——インジェクションのリスクがあります。マージ前にパラメータ化できますか?」
「抽象化が間違っています。」 「この抽象化が正しい方向に引っ張っているか少し心配です——次の機能を難しくしてしまう可能性があるように感じます。簡単にすり合わせる価値があるでしょうか?」

「優しい」列がどんなものかに注目してください:柔らかくも、曖昧でも、不誠実でもありません。セキュリティの問題は依然としてブロッカーとして指摘されています。nullケースはまだ修正が必要です。関数はまだ分割が必要です。ここでの優しさとは、具体的で、説明があり、裁判官が判決を下すのではなく、人間が人間に語りかけるように届けるということです。

細かい指摘、提案、ブロッカー——ラベルをつける

レビュー文化のために実践できる最も実用的なことの一つは、すべてのコメントに深刻度のラベルをつけることです。ラベルがないと、著者は各コメントが「マージがブロックされる」のか「ただの思いつき」なのかを推測しなければなりません。その推測はエネルギーを消費し、しばしば間違った答えをもたらします。

多くのチームで使われているシンプルな慣例(Conventional Commentsとして広まったもの):

  • nit: 軽微なスタイルや好みの点——修正するかしないかは著者が決めます。「nit: ここの末尾カンマがファイルの他の部分と合っています。」
  • suggestion: コードを改善するが、ブロッカーではないもの。「suggestion: これをヘルパーに切り出せますか——ただし今のプルリクエストには必須ではありません。」
  • blocking: これが対処されるまでプルリクエストはマージすべきでない。正しさのバグ、セキュリティの問題、チーム標準の明確な違反のためだけに使います。
  • question: 純粋に質問していて、変更が必要と示唆しているのではない。「question: これはバックグラウンドワーカーでも動きますか、それともWebプロセスだけですか?」
  • praise: 明示的で本物の肯定的フィードバック。大切です。スキップしないでください。

ブロッキングの問題がすべて解決されたら、コメント付きで承認しましょう。これは大切な習慣です。「残りの細かい指摘はあなたに任せます。もう一度私のレビューは不要です」という意味になります。同僚の作業をアンブロックし、信頼を示し、レビューのループがボトルネックになるのを防ぎます。逆に——すべての細かい点が対処されるまで承認を保留すること——はレビュー文化の遅効性の毒の一つです。

「コメント付きで承認」の習慣

ブロッキングの項目がすべて解決されたら、承認して残りは著者に任せましょう。細かい指摘のためにプルリクエストを人質にすることは、レビューが協働ではなく障害物だと人々に教えます。どうしても気になる非ブロッキングな点があれば、明示的に伝えてください——「これは気になっていますが、あなたの判断に任せます」——著者が完全な情報を持てるようにするために。

著者の立場:フィードバックを個人的に受け取らないために

レビューされることは、それ自体がスキルであり、エンジニアリング文化に関するほとんどの文章はレビュアーにのみ焦点を当てています。でも、著者の行動もレビュー文化を同じくらい形作ります。

デフォルトで善意を前提にする。ぶっきらぼうなコメントは、ほぼ常に急いでいる人が素早く書いたもので、あなたを傷つけようとしている人ではありません。コメントのトーンを読む前に、中立的かポジティブな解釈の可能性を考えてみてください。大抵はそういう解釈があります。

すべてのコメントに返信する。たとえ同意しない場合でも。実装済みフィードバックには✅か「done」を、反論するものには説明を——ループを閉じて、レビュアーの時間が大切にされたことを示します。沈黙は、意図しなくても受動的攻撃に見えます。

本当に同意しない場合は、明確かつ落ち着いて反論する。「懸念はわかります。でもこの選択をした理由はこうです:[理由]。まだ強く感じるなら、話し合いましょう。」これはすべての決定を反射的に守ることとは違います。目標は本物の会話をすることであり、勝つことではありません。時にはレビュアーが正しい。時にはあなたが正しい。時には「チームに委ねましょう」が正しい答えです。

プルリクエストの説明で自分のコードを過剰に説明しない。コードがすべての決定を正当化するための長文を必要とするなら、それはコード、名前、またはインラインコメントを改善すべきというシグナルであり、より長いプルリクエストの説明を書くシグナルではありません。最良のプルリクエストは、コンテキストと意図の良い説明があれば自明です。

レビューフィードバックの質をマッピングした2×2クアドラント。理想のクアドラントは「優しい×具体的」です。 KIND UNKIND VAGUE SPECIFIC Kind + Vague 心地よいが、成長には つながらない。 「全体的に良さそうです!」 Kind + Specific ✦ 目標 ✦ 明確で、実行可能で、敬意がある。 教え、出荷を速める。 「nit: このヘルパーを切り出す—— 分離してテストしやすくなる。」 Unkind + Vague 最も有害。 導くことなく士気を下げる。 「これはひどいな。」 Unkind + Specific 技術的には有用だが、 時間とともに信頼を損なう。 「Xを使うべきでした、 明らかに間違っています。」
レビューフィードバックの2×2。Kind + Specific(右上、強調表示)が目標:正直かつ敬意を持ったフィードバック。Kind + Vagueは安心感があるが何も教えない。Unkind + Specificは短期的には機能するが信頼を損なう。Unkind + Vague——最悪のクアドラント——は導くことなく士気を下げる。

レビューのスピードとプルリクエストのサイズ——大きな影響を持つ二つのレバー

コードレビューの質は、レビュアーのスキルとはまったく関係のない二つのことで強く予測されます:どれだけ素早くレビューするか、そしてプルリクエストがどれだけ大きいかです。

迅速にレビューする。レビュー待ちのプルリクエストは、次に進むのを待っている人です。レビューに数日かかると、作業が溜まっていきます——エンジニアは生産性を保つために次のタスクを開き、コンテキストの切り替えが増え、レビューが戻ってくる頃には著者が何を考えていたか忘れてしまっています。良い目安:タグ付けされてから数時間以内にプルリクエストを確認し(「本日中に対応します」だけでも良い)、可能な限り一営業日以内にレビューを完了する。

小さなプルリクエストは劇的に良いレビューを受ける。これは十分に記録されており、実際にも明らかです。100行のプルリクエストはすべての行に注目が集まります。1,000行のプルリクエストはゴム印を押される、ざっと読まれる、あるいは非常にゆっくりレビューされて対立の源になります。プルリクエストを小さく保つ訓練——「プルリクエストごとに一つの論理的な変更」は良いヒューリスティックです——は、他のほぼどんなレビュー衛生の改善よりも大きな見返りをもたらします。また、一つの変更が何を意味するかを明確に考えなければならないため、各プルリクエストを書くことも簡単になります。

プルリクエストサイズ(変更行数) 典型的なレビュー品質 レビュー時間
< 200行 高い——レビュアーはワーキングメモリに保持できる 15〜45分
200〜500行 中程度——微妙な問題で疲労が生じる 45〜90分
500〜1,000行 下がっている——レビュアーがざっと読むかセッションを分割する 数時間から一日
> 1,000行 ほぼ形式的——重大な問題が定期的に見落とされる 数日間(または完全には終わらない)

機能が本当に何千行も必要な場合は、スタック型プルリクエストを検討してください:それぞれが独立してレビュー可能な、互いに積み重なる小さな自己完結した変更のシリーズです。より多くのプルリクエストを開くオーバーヘッドは、ほぼ常に価値があります。

チームの規模によってレビュー文化がどう変わるか

コードレビューへの適切なアプローチは、チームが成長するにつれて大きく変わります。3人のエンジニアで機能することが30人ではうまくいかないことが多く、エンタープライズ規模のチームにはまったく異なるツールが必要です。

ソロまたは初期スタートアップ(1〜5人のエンジニア)。この規模ではレビューがまったくない場合もあり、リスクに応じてそれが問題になることもあれば問題にならないこともあります。レビューをする場合、関係が通常十分に強く、ぶっきらぼうなフィードバックも伝わりやすいですが、悪い習慣が誰も名前をつける前に固定されてしまう段階でもあります。コメントの深刻度ラベリングの習慣を早めに確立してください。コストはゼロで、後から非常に価値があります。

小さな成長中のチーム(5〜20人のエンジニア)。レビュー文化が作られるのはここです。「お互いを知っている」という略記が薄れ始めるほど大きいですが、影響力のある数人の声で規範がまだ設定できるほど小さいです。優しく、具体的で、ラベルの付いたレビューをモデルにするシニアエンジニア一人で、数ヶ月以内にチーム全体の文化を変えられます。また、チームのレビューガイド——一ページでも——が見返りをもたらす時期でもあります:何がブロッカーか、何が細かい指摘か、レビューにどのくらい時間をかけるべきか。

中規模チーム(20〜100人のエンジニア)。レビューは今やほとんど顔を知らないスクワッド間で行われています。小さなチームで自然に起こる信頼の構築を意図的に行わなければなりません。チーム横断のレビューは規範が共有されていない場合、大きな摩擦源になります。コードオーナー、レビューのローテーション、軽量なレビューチェックリストが助けになります。レビュー品質をエンジニアリングの振り返りのトピックにすることも——「速く出荷したか」だけでなく「うまくレビューしたか」。

大規模またはエンタープライズのチーム(100人超のエンジニア)。この規模では、レビューは品質機能と同様にガバナンスおよびコンプライアンス機能でもあります。ツールが非常に重要:自動チェック、必須レビュアー、レビューSLA、レビューサイクルタイムのメトリクス。人間によるレビューのスロットは貴重であり、人間にしかできないことのために確保すべきです——設計の判断、ドメイン知識、チーム横断の影響。リンターや静的解析ツールが検出できることは、プルリクエストが開かれる前に検出されるべきであり、コメントスレッドではありません。

まとめ

  • コードレビューはまず文化であり、バグ探しは二の次です。すべてのコメントが何かを教え、コメントのパターンが何ヶ月もかけて心理的安全性を築いたり壊したりします。
  • 重要なことにレビューのエネルギーを集中させましょう:正しさ、設計、読みやすさ、テスト、セキュリティ。フォーマットはリンターに任せましょう。
  • 「優しい×具体的」が目標です。「なぜ」を説明し、判決を下すのではなく質問をし、良いコードを明示的に褒めましょう。
  • コメントにラベルをつけましょう:nit / suggestion / blocking / question / praise。曖昧さをなくし、著者の時間を尊重します。
  • ブロッカーが解決されたらコメント付きで承認しましょう。細かい指摘のためにプルリクエストを人質にしないでください。
  • 著者として:善意を前提にし、すべてのコメントに返信し、同意しない場合は落ち着いて反論し、プルリクエストの説明で過剰に説明しないようにしましょう。
  • 小さなプルリクエスト、速いレビュー。200行以下が最も良い注目を得ます。大きな機能にはスタック型プルリクエストを使いましょう。
  • レビューの規範は意図的に作る必要があります——特にチームが10〜20人を超えて成長する場合は。書き留めて、振り返りで見直しましょう。

これが響いたなら、このシリーズの次の記事ではさらに広く——コードのレビュー方法から毎日エンジニアとしてどう在るかへ——テーマを広げています:Kind Engineering

よくある質問

コードレビューにはどのくらい時間がかかりますか?
200行以下のスコープが絞られたプルリクエストであれば、15〜45分の集中した時間を見込んでください。レビューリクエストに数時間以内に応答し、可能な限り一営業日以内に完了することが健全な目標です。48時間を超えるレビューはチーム全体を遅らせるボトルネックになります——迅速さはレビュー品質の一部です。
コードレビューで何を確認すべきですか?
正しさ(エッジケースを含め、本番環境で動作するか?)、設計(アーキテクチャにきれいに収まっているか?)、読みやすさ(チームメンバーが六ヶ月後に理解できるか?)、テスト(意味があり、重要なケースをカバーしているか?)、セキュリティ(ユーザー入力はバリデーションされているか、インジェクションのリスクはあるか?)に集中しましょう。リンターがすでに検出するフォーマットの問題はスキップしてください——その注目は人間にしか判断できないことに使いましょう。
コードレビューのフィードバックを厳しく聞こえないように伝えるにはどうすればいいですか?
最も効果的なテクニックは、判決を下すのではなく質問をすること、そしてなぜそれが重要かを常に説明することです。「なぜここでXを選んだのですか?」は対話への招待ですが、「Yを使うべきです」はそれを閉じます。コメントの深刻度をラベル(nit / suggestion / blocking)で示すことも助けになります——「これはブロッカーです」と「これは好みの問題です」を分け、著者が推測しなくて済むようにします。
スタイルの細かい指摘のためにプルリクエストをブロックすべきですか?
いいえ。プルリクエストをブロックすることが正当化されるのは、正しさのバグ、セキュリティの問題、または合意されたチーム標準の明確な違反——マージすれば本当の問題を引き起こすもの——についてです。スタイルの好みや軽微なフォーマットの問題はnitとしてラベルを付け、著者の裁量に委ねてください。すべてのブロッキングコメントが解決されたら、コメント付きで承認し、残りは著者に任せましょう。
プルリクエストはどのくらいの大きさにすべきですか?
200行以下の変更が高品質なレビューのスイートスポットです。このサイズであれば、レビュアーは変更をワーキングメモリに保持でき、微妙な問題も見つけられます。500行を超えるプルリクエストはレビュー品質が急激に下がります——レビュアーはざっと読み、疲労が生じ、重要な問題が見落とされます。大きな機能には、スタック型プルリクエスト——それぞれが一つの明確なストーリーを語る、小さな自己完結した変更のシリーズ——を使いましょう。