コードレビューの主な目的は、保守しにくいコードを見つけること
(mathstodon.xyz)- コードレビューは、バグを見つけたり完全性を保証したりする手順というより、後で保守しにくくなるコードを事前にあぶり出すプロセスに近い
- レビュアーがコードを読んでも理解しにくいなら、将来の保守担当者も同じ問題に直面する可能性が高い
- 修正は、元の作成者が文脈を覚えている今のうちに行うほうがよく、コード検査だけでバグを安定して見つけられると期待するのは現実的ではない
- 「バグを探せ」よりも、「理解してみて、理解できない部分を示せ」のほうが、レビュアーにとって実行しやすい課題である
- 良いレビューは、すべてを完璧に証明することではなく、理解できない箇所にメモを残して改善を求めることから始まる
コードレビューの焦点を変える
- コードレビューの中核的な目的は、レビュアーがバグを見つけることではなく、コードにバグがないと保証することでもない
- コードをざっと見るだけで一般的にバグを見つけられると期待するのは、実務上あまり現実的ではない
- だからこそ、レビューの中心は「正しいコードか」よりも「他の人が後で読んで修正できるか」に置かれる
理解しにくいコードは保守リスクのサイン
- レビュアーは、コードが何をしていて、どう動くのかを理解しようとして読む
- 理解できない部分は、将来の保守担当者が行き詰まる可能性のあるリスクのサインになる
- こうしたコードは、元の作成者がまだ文脈を覚えているうちにすぐ直すほうがよい
バグ探しより実行可能なレビュー課題
- 「このコードの中からバグを探してほしい」という依頼は、成功したかどうかを判断しにくい作業である
- バグをいくつか見つけても、さらに潜んでいるバグを見落としているかもしれず、レビュアーの立場では失敗だけが明確になりやすい
- 一方で、「このコードを理解してみて、理解できなければ指摘してほしい」という依頼は、基準がより明確である
- すべてを完璧に理解する必要はない
- 理解できなかった部分を記録すればよい
- 全体を理解しようと試み、詰まった箇所にメモを残せば、レビューの役割を果たしたことになる
実務におけるレビュー基準
- レビュアーが理解できないコードは、それ自体が修正対象になりうる
- レビューコメントは、バグ報告だけでなく、説明不足、構造上の問題、読みにくい流れを明らかにする役割を持つ
- この基準では、コードの正当性を証明することよりも、その後チームメンバーが読んで扱える状態にすることが重要である
バグ発見は副次的な効果
- コードレビューがバグをまったく見つけられないという意味ではない
- バグはレビュー中に発見されることもあるが、すべてのバグや大半のバグを見つける方法として期待するのは難しい
- より現実的な成功条件は、理解可能性を点検し、保守しにくい部分を元の作成者と一緒にその場で改善することである
1件のコメント
Hacker News のコメント
コードレビューには単一の目的だけがあるわけではない。保守しにくいコードを見つけることも重要だが、それが唯一の目的でも、もしかすると最重要の目的でもないかもしれない。
開発者や AI が妙な方向へ進んだとしても、悪意あるコードのマージを難しくする安全装置になるし、問題に近づきすぎていない人が、より良い方法や見落とした問題に気づけることもある。
システムの別の部分をよりよく知っている人が相互作用上の問題を見つけられることもあり、少なくとも1人はそのコードに慣れることになる。また、作者とレビュアーの双方にとって学習の機会にもなる。
特に経験年数に差がある場合に重要で、新入社員をメンタリングするときは、自分のすべての PR にレビュアーとして追加して自分の働き方を見せ、彼らの PR もレビューして方向づけをする。たまには自分も彼らから学ぶ。
バグを見つけることも確かにあるが、それが主たるメカニズムであるべきではなく、自動テストで見つけにくい セキュリティ・性能バグ では特に重要になる。
特に モジュール化と分解 ではそうで、巨大な PR 全体を理解すると頭の中にモデルができ、保守可能か、いつか完全な悪夢になるのか、それともその中間なのかが見え始める。
コードレビューでおそらく最も重要な部分は 知識移転 だと思う。
私たちの小さなチームでは、急ぎの場合を除き、マージ前にチーム全員が PR に承認を付ける。そのおかげで、メンバー全員が現在のコードベースの状態を大まかに把握している。以前、よりサイロ化された会社で経験した「自分が依存していたシステム全体が消えていた」といった不意打ちを避けられる。
また、動作の仕組みについて質問し、理解を深める場にもなる。うまく機能しているチームなら、すべての開発者が、自分で直接触らない部分も含めて、システム全体をある程度理解しているべきだ。
もう1つ重要な機能は 組織知識のチェック だ。最近テーブルを少し変更したところ、同僚が、私の考慮していなかったマイクロサービスがそのテーブルを使っているので壊れると教えてくれた。そのマイクロサービスが存在することも、そのテーブルにアクセスしていることも知らなかったが、そのチェックのおかげで、より大きな問題やデータ整理の事態を防げた。
最終的には皆が自分の仕事で忙しく、重要な PR を止める人になりたくないので、ただ承認だけ押すようになり、実際には誰もコードをレビューしていない状態になった。
「自分が依存していたシステム全体が消えていた」のような問題は、そのシステムに依存する人がその場にいなくても検出できる 自動テスト が非常に重要だと思う。
すべてに対する共有オーナーシップにも強く賛成する。人々がコードベースの特定部分、特に自分が作ったコンポーネントを事実上所有するようになるのは自然だが、それはサイロ化と低いバスファクターにつながる。1人が1つのシステムを所有し、そのシステムがさらに別の1つのコンポーネントに依存するような構造になってはいけない。
読むものが多すぎると誰も追いつけないので、委任し、文書を作り、概要セッション を開くのだ。
コードレビューは、作者が所有していたコードが チームやプロジェクトの所有物 に移る関門だと見るのが一番よいと、私はずっと思ってきた。
私がレビューしているコードはあなたのコードではなく、まもなく私たちのコードになるコードだ。当然、保守性はその中で大きな要素になる。
私たちのチームは AI を使い始めたので、私は単純なやり方に変えた。コメントは残さず、「これは常軌を逸しているか、それとも通してよいか」だけを二値で承認判断している。
自分の時間とメンタルヘルスを守っているところだ。
こうすると、レビュアーと作者がさらに怠惰になるだけ
コードレビューの目的は多面的です。保守しにくいか、バグがあり得るか、もっとシンプルでクリーンにできるか、プロジェクトのコードスタイルに合っているか、他の人にもコードを理解させるか、ジュニアメンバーをオンボーディングするか、設計上の判断を sanity check するか、といったことはすべて含まれます。
こういう軽い文章は、たいてい 怠惰なコードレビュアー が自分を正当化するものに近いです。
issue や PR の説明どおりに機能面の目標を達成しているか、残ったデバッグ出力や非公開 API キーのような不要なコードがないか、メモリリーク・処理されていないエッジケース・セキュリティ欠陥・古い API 呼び出しのような明らかな欠陥がないかを見るべきです。
もっと理解しやすくできるか、抽象化を追加または削除すべきか、変数名・メソッド名はよりよいか、関数型スタイルをもっと使う／減らすほうがよいかも見るべきです。
コードベースやスタイルガイドと一貫しているか、リストの代わりにハッシュセットを使う、遅延評価を適用するなど、明らかな性能改善があるか、テストが十分かも確認すべきです。
理解できないコードなら通してはいけない、という主張にも完全には同意しません。コードの中には単に本当に理解が難しいものもあり、目標は機能的に正しく、かつ可能な限り理解しやすくすることです。
ところがこの記事はほとんど釣りに近く、「人々は夕食を食べ物を食べることだと思っているが、実際には食べることではなく、家族や友人とつながることだ」と言っているのに似ています。HN で受けやすい、特定の種類の雑な 還元主義的論理 です。
レビューとデバッグは、コードを書くこと・生産することよりはるかに時間がかかると感じましたし、単に「動くことを祈る」のは決してよい結果になりません。
「コードを眺めても一般にはバグを見つけられない」という言い方はまったく正しくありません。抽象化の各レベルで十分に見つけられますし、そういうものを コードスメル と呼びます。
閉じられていないファイルディスクリプタ、await されていないコルーチン、エラーを記録せず何らかの値に戻す巨大な try/catch ブロック、誤った型キャストなどがすべて該当します。
一般原則として、型検査器・コンパイラ・ランタイムを、ただ通過しなければならない段階と見なすべきではありません。これらの段階と協調し、それらが価値あるツールであることを認めるべきで、逆らって作業すべきではありません。
「一般には」をどうにか技術的に真になるよう定義することはできるでしょうが、そうするとほとんど意味がなくなります。
コードレビューについて広い主張をするなら、どの種類のコードレビューを指しているのか定義することが重要です。
今では GitHub 式の非同期 PR レビューとインラインコメントが標準ですが、レビューはそれだけではありません。昔は対面レビューが学位論文審査や学会発表に近いプロセスだったこともありました。
コードレビューが有用な品質プラクティスであり、実際には数少ない有用な品質プラクティスの一つだとする文献は、概して現在よりはるかに構造化されたレビュープロセスから出てきたものです。
個人的には、LLM 以前の GitHub 式 PR レビューは、プロセスがまともだと感じさせるため、あるいはガバナンスのチェックボックスを埋めるためのもので、LLM 時代 には費用対効果がはるかに悪くなり、消えていきそうに思います。
無理に技術的な比喩を使うなら、非同期のテキストコミュニケーションは、うまくエンコードできる情報という点で会話より損失が大きく、スループットも低いです。だから多くの情報を交換する必要があるときは、同期コスト を払う価値があります。
伝統的な工学に近く、全員がソフトウェアをより永続的なものとして考える必要がありました。
保守性を保証することがコードレビューの利点の一つであるのは確かですが、それが唯一の目的だと言うのはかなり大胆な主張です。
コードレビューは、チームがコード変更を把握し、コードベース全体に対する 共同責任 を分かち合うためのツールでもあります。
コードレビューは単一のものではありません。知識共有、責任逃れ、コード品質、規制遵守などさまざまな理由があり、いつものことですが、どんな目的を持つかは 利用する文脈 によって変わります。
作者が数学者なら、「コードを眺めても一般にはバグを見つけられない」という言葉は、バグを見つけることが完全に不可能だという意味ではないでしょう。
すべてのバグを見つける、あるいは特定のバグを見つけることはできない、という意味に近いはずです。
保守性の論点とつなげて付け加えるなら、コードを可能な限りシンプルにして、レビューでデバッグ可能 になる確率を高めることもレビューの目標です。絶対的な意味でバグを防ぐことはできませんが、確率は引き上げます。
最初の解釈は関連はありますが誤りで、二つ目の解釈は真ですが関連がありません。
おそらく作者は「与えられたバグをコードを眺めて一般には見つけられない」、つまり「すべてのバグ B について、B を見つけられるわけではない」と言いたかったのでしょうが、これも真ではあるものの、核心からは離れています。
PR の提案をよく拒否する人と、提案を受け入れる人の両方と一緒に仕事をしたことがある。
提案を受け入れる人は、ある程度 オーナーシップを私と共有しようとしているのだと考えさせられる。双方がコードを保守し、理解し、同じ方向を見ているという感覚がある。
逆に、PR の提案を拒否する人の PR には、参加しようという気持ちが薄れる。どうせ拒否されるなら、なぜ時間をかけて丁寧にレビューするのか、という気になる。
thought、change、nit、fix、chatのような接頭辞を付ける。例えば
thoughtは「後で foo がより一般的になるかもしれないので、そのときにリファクタリングしよう」、changeは「これは漏れのある抽象化なので、bar のようにモデル化してほしい」、nitは「名前が少し直感的ではないので、Baz や Boo を検討しよう」、fixは「このユニットテストは間違ったフィールドを検証している」、chatは「今後このカテゴリの解決策の形を決める大きな判断なので、まずチームと議論しよう」といった具合。ある接頭辞は修正されるまで PR を止めるものを意味し、別のものは受け入れても受け入れなくてもよいコメントであることを意味する。作者にとって、何が「同じ理解に到達すべきこと」で、何が「好みの表現」または「観察」なのかが明確になる。
ただし、
nitを残したのに相手が同意せず無視しても、気を悪くしてはいけない。強く感じていたなら、それはnitであるべきではなかった。