概要
コードレビューとは、変更をマージする前に作成者以外の視点で確認し、その確認結果をもとに、その変更をコードベースの一部として取り込んでよいかを判断する実践です。 このページでは、まずレビューが組織にもたらす価値と、レビュアーが実際に何を確認するのかを扱います。次に、そのチェック作業をツールがどう肩代わりしてきたかを見て、最後に、自分の変更を先にレビューする習慣、完璧さではなく改善を基準に承認すること、対応しやすいコメントを書く習慣という3つの実践で締めくくります。プルリクエスト自体の粒度・テンプレート・タイトルやコミットの規約、そして依頼からマージまでの基本的な流れはすでに整っている前提です。それらはプルリクエストを、変更を検証可能にする土台はテストコードを参照してください。レビューが組織にもたらす価値
出荷前に欠陥を見つけることはレビューを行うわかりやすい理由であり、それ自体は事実です。ただし、そこで止まってしまうと、レビューが持つ価値の大部分を取りこぼします。知識の共有
変更を確認する過程で、作成者以外の誰かがその変更を理解せざるを得なくなります。なぜその変更が存在し、どう動くのかという知識が、一人の頭の中だけにとどまることはありません。
規約の育成
規約は、レビューでの「うちではこうしない」という一度きりの指摘から始まります。十分な数のレビューで繰り返されることで、次の作成者がすでに知っているルールになります。チームの共通の基準は、レビュー1回ごとに育っていきます。
非機能要求の確認
レスポンスタイム・セキュリティ上の露出・コード外のルールへの準拠は、失敗するテストとして表面化することがほとんどありません。それらを尋ねるチェックポイントは、レビューだけであることが多くあります。
品質向上
すでに動いている変更も、さらに良いものへとレビューできます。レビュアーがより良いやり方を提案すれば、「動くかどうか」の基準を守るだけでなく、その基準自体を引き上げます。
レビューで確認する観点
機能要件は、Issueの受入基準やチケットの説明として明示されていることがほとんどなので、確認作業は主に、コンパイルが通るかではなく実際に依頼された内容と差分を照らし合わせる作業になります。
非機能要件は見落としやすい観点です。遅いクエリはビルドを失敗させませんし、コンプライアンス上の欠落も例外を投げないため、問題があっても自分からは表に出てこないからです。
設計の適合・テストスイート・可読性は、確認作業の残りを支えます。設計の適合はその要件が追加される先のコードベースを守り、テストスイートは次にこのコードに触れる人を守り、可読性は、変更を保守し続けるという本当のコストが、マージ後しばらく経つまで見えないままになるのを防ぎます。
ツールがレビューの機械的なチェックを肩代わりしてきた歴史
かつては、上表のすべての観点が人間によって確認されており、レビューとはそれらすべてを目視で見つけることを意味していました。 レビューが長引く要因はいくつかありますが、そのうち最も本質的でないのが、機械がすでに作成者へ伝えられたはずのことを人間が改めて洗い出しているケースです。 lint違反・テストの不足・スタイルガイドですでに決着している命名の不一致などがその典型です。さらに悪いことに、簡単な項目と難しい項目を一緒に洗い出すとレビュアーの注意が分散し、本来判断が必要な項目こそ見落とされやすくなります。 linterやフォーマッタは、まさにこの問題への標準的な解決策として登場しました。命名・フォーマット・そして年々増えていく既知のバグパターンを、人間のレビュアーが差分を開くより前に自動でチェックします。現代の多くのチームは、これを前提にしています。 セミコロンの付け忘れや命名の不一致を捕まえるためにレビュアーの時間が使われることは、もうありません。すでにツールがそれをやっているからです。 AIはこの発想をさらに一歩進めます。linterが固定的で一般的なルールとしか照合できないのに対し、AIによるチェックはチーム独自のコーディング規約や文脈とも照合できます。これは、以前はそのコードベースをすでに知っている人にしかできなかった種類の判断です。機械的にチェックする
正解がルール、あるいはチーム独自の規約で決まるもの。
- 命名・コーディング規約への準拠
- 変更行のテストカバレッジ
- 固定のルールセットと照合できる既知のバグ・脆弱性パターン
人間のレビュアーが担う
正解が文脈に依存する判断。
- 設計・アーキテクチャが周辺システムになじんでいるか
- 変更が実際に要件を満たしているか
- セキュリティリスクの受容とマージの最終判断
ファインディでは、この機械的なチェックをAIによるセルフレビューに任せ、プルリクエストを作成する前に実行しています。大事なのはルールで判定できる部分と人間の判断が必要な部分を分けておくことです。この考え方がAI生成コード全般にどう当てはまるかは、Agentic Workflowを参照してください。
自分の変更を先にレビューする
レビュアーに依頼する前に、初めて見るつもりで差分を読みます。距離を置くと見え方が変わります。書いているときには見えなかった間違いも、他人として変更を見直すと気づけるようになります。ここで見つけたものはすべて、他人の注意力を借りて往復することなく、その場での修正で済みます。 差分を、自分が書いた順ではなくレビュアーが見る順、つまり上から下へファイルごとに、途中で直さずに読み通すと、最初にぶつかった1件だけでなく、レビュアーが気づくのと同じ粗い箇所に気づけます。セルフレビューチェックリストの例
承認の基準 — 完璧ではなく、より良く
レビュアーは、変更が完璧になるまで承認を待つ必要はありません。承認するかどうかの基準は「この変更をマージすると、コードベースは今より良くなるか」です。完璧なコードというものは存在せず、あるのはより良いコードだけです。 完璧を求めて承認を引き延ばすと、指摘への対応で変更は膨らみ、レビューの往復は増え、ユーザーに価値が届くのも遅れます。小さな変更を素早くマージし続けるほうが、コードベースは着実に良くなっていきます。 ただし、これは基準を下げるという意味ではありません。ブロックするものとしないものを分けるということです。マージをブロックする
マージするとコードベースやユーザーが悪い状態になるもの。
- 要件を満たしていない、振る舞いが誤っている
- 変更した振る舞いがテストで裏付けられていない
- パフォーマンスやセキュリティに悪影響がある
- そもそもシステムに入れるべきではない機能である
マージをブロックしない
マージ後に別のプルリクエストで扱えるもの。
- あとから思いついた、より良い書き方
- 既存の構造になじんだうえでの、好みの範囲にとどまる設計や命名の違い
- 次の変更に向けた整理やリファクタリング
mustのラベルを付けます。ブロックしない指摘はimoやnitsのラベルを付けて残し、適用するかどうかは作成者に任せます。今回は見送る改善は、リファクタリングだけのプルリクエストやIssueに切り出し、レビュー中の変更を長引かせないようにします。構造の変更と振る舞いの変更を分けて出す考え方はリファクタリングを参照してください。
改善を後回しにできるのは、テストがあるからです。振る舞いがテストで守られていれば、構造の改善はあとからいつでも、振る舞いを変えずに安全に行えます。逆に、テストのない変更を「とりあえずマージ」すると、後から直すことはかえって難しくなります。
ファインディでは「綺麗なコードは後。アプリケーションの振る舞いが先。」という考え方をとっています。テストが網羅されていてCIが通れば、パフォーマンスやセキュリティに悪影響がない限りマージします。より良い書き方をあとから思いついた場合は、リファクタリングだけのプルリクエストを別に出します。
レビューコメントの書き方
コメントの役目は、往復を最小限にして正しい修正にたどり着かせることです。そのためには、作成者がコメントの意味も緊急度も推測せずに行動へ移せる必要があります。 曖昧な反応は、その推測作業を作成者へ押し付けます。「これは違和感があります」だけでは、何が問題で、どこにあり、マージをブロックするのかどうかがわかりません。反応だけでなく具体的な場所と理由を書くと、コメントは、作成者がまず意味を読み解かなければならないものではなく、そのまま行動に移せるものになります。 コメントの先頭に、その重みを示す短いラベルを付けると、もう1つの推測作業をなくせます。よく使われるラベルの組み合わせは次のとおりです。関連ページ
プルリクエスト
レビューを依頼し、その結果を適用する場所。粒度・テンプレート・マージの仕組みを扱います。
テストコード
そもそも変更を検証可能にするもの。レビューがテストコードに対して確認するパターン。
Agentic Workflow
上記の機械的なチェックと判断の分担のもとになっている、AI生成コード全般でのAIと人間のより広い責任分担。
依存関係の更新
機械的なチェックと判断の分担を、AIエージェントが一次確認する更新プルリクエストに当てはめた例。