本規約は、世の中のシステム開発プロジェクトのために無償で提供致します。
ただし、掲載内容および利用に際して発生した問題、それに伴う損害については、フューチャー株式会社は一切の責務を負わないものとします。
また、掲載している情報は予告なく変更することがございますので、あらかじめご了承下さい。
プログラマが知るべき97のこと の14個目に「コードレビュー」について記述がある。コードレビューの目的は「コードの質を上げ、欠陥を減らすため」だけではなく、「チーム全員に同じ知識を共有させること、またコーディングにおいて全員が守るべきガイドラインを確立すること」が大切だとある。そして「 レビューを楽しいものにすること」がおそらく最も有効だとある。
コードレビューを楽しい場にするためにも、レビュアー／レビュイー が同じ方向性を向くことが重要である。同時に、人によってコメントなどの表現が揺れ、それにより認識齟齬があると楽しむ以前の問題であるため、守るべきルールも存在するはずである。このガイドラインは推奨する行動と、守るべきルールの両方を定義し、コードレビューをより有意義で学びが多く、生産性と品質を高める場とすることを目指す。
GitHubやGitLabなどのサービスを利用した、コーディングについてのレビューのみを対象とする。
プロジェクト計画によっては、開発プロセスを複数の「フェーズ」に分割し、各フェーズの終了時点で「フェーズレビュー」を挟むようなケースも考えられるが、それらは対象外とする。
::: warning 有志で作成したドキュメントである
:::
Gitブランチフロー規約 > ブランチ戦略の選定 にある、以下の2パターンのいずれかを利用しているとする。
Lite GitLab FlowGitLab Flowそのため当然であるが、全ての機能開発や不具合修正に、feature
ブランチを使用したプルリクエストを経由してマージする方針を取る。その他の
feature ブランチから develop
ブランチへのマージ方法などもGitブランチフローに記載した推奨事項に則る。
Gitブランチフロー規約 に則った設定を実施する。例えば、ブランチ保護やレビューの設定には以下の内容がある。できるかぎり自動化やテンプレートを用意することで、個々人が考慮すべきことを減らすことで運用負荷を下げ、規則を形骸化にせず実効性を上げることを基本姿勢とする。
| 設定項目 | 設定例 | 説明 |
|---|---|---|
| Require a pull request before merging | ✔ | プルリクエストを必須とする |
| Require approvals | 1 | 1名以上の承認を必須とする |
| Require status checks to pass before merging | ✔ | CIの成功をマージ条件とする |
| CODEOWNERS | (省略) | 重要ファイル（例えば、ddl.sqlやopenapi.yamlなど）は承認が必須なレビュアーを指定する |
| linguist-generated=true | (省略) | ツールなどによる生成ファイルの差分を非表示にする .gitattributesの設定 |
コードレビューのやり取りで、「コードフォーマットされていない」 「リンターを通していない」といった指摘は不毛である。そのため、CIでそれらを実行し、パスしているという前提条件を作ることが重要である。それにより、レビュアーは人間が本来見るべき点に集中できるためである。
例えば、下表のような内容はフォーマッタやリンターに寄せることができる可能性が高い。
| 自動化可能だと考えられる指摘例 | ツール例 |
|---|---|
| 使われていないメソッドや関数 | Pythonの場合、Flake8 など |
| デッドコード | Pythonの場合、Vultureなど |
| デバック用のログの残骸 | JSの場合は、ESLint（console.logの検知など）。 Pythonの場合は、Pylintなど |
| 不適切なレイヤーに対するimport | Pythonの場合、import-linterなど |
| //TODO コメントが残っている | JSでは、eslint-plugin-no-inline-commentsなど |
| フォーマッタが適用されているかどうか | JSでは
prettier --check "yourfile.js"、Pythonでは
black --check yourfile.py など |
Linterなどの検知結果をreviewdogなどと連携することで、形式的な指摘はツール側に寄せる。それにより開発者個人個人のやり取りを減らし、本当に集中すべき事項に時間を割くこととする。
プルリクエストにチェックリストを追加し、レビュー依頼前にセルフチェックする運用を採用するチームも多い。チェックリストが多すぎると形骸化するため、項目は具体的にする（例: 設計ドキュメントを同時に直したか？など）とともに、必要最低限の項目に留め、チームの成熟度に応じて適時見直す運用とする。
参考: リポジトリ用のプルリクエストテンプレートの作成 - GitHub Docs
「Javaコーディング規約」 「SQLコーディング規約」 「AWSインフラ命名規約」 「OpenAPI Specification 3.0.3規約」など、チームで守るべきと合意形成されたコーディング規約や命名ルールが存在することを前提とする。また、これらに記載された内容をチームで改善し続ける運用とする。
「フォーマッタやリンター」などのツールをCIと組み合わせることで、レビュアーが見るべき領域を減らすことができる。また、「チームで合意された開発規約」をレビュイーが遵守できれば、ツールがカバーできない領域に対してもレビュアーの負担減に繋がる。
| 大分類 | 中分類 | 小分類 | レビュイーが気にすべきか | レビュアーが見るべきか |
|---|---|---|---|---|
| 開発規約に記載済 | ツールでカバー可能 | ⚠️ツール補助あり | 不要 | |
| ツールでカバー不可 | ✅️規約を準拠しているか確認する必要がある | ✅️規約を準拠しているか確認する必要がある | ||
| 開発規約に未記載 | 規約に記載可能 | ⚠️Yesだが、難しい場合が多い | ✅️言語化して開発規約に追加すべき | |
| 規約に記載不可 | 機能要件の充足 | ✅️ | ✅️ | |
| 文脈依存が強く汎化不可 | ⚠️Yesだが、難しい場合が多い | ✅️個別の命名や、他に類似の実装パターンがない場合は個別に見るしかない |
レビュー観点は、開発しているプロダクトの性質、開発体制、採用技術で大きく変動するためここで詳細は述べない。
基本的には、チームで合意形成された開発規約（参考: 開発アーキテクチャドキュメント）やツール（リンターやフォーマッタ）を充実させる方向に寄せるべきである。以下に、開発規約にはどのような内容を記載すべきかを以下に参考情報としてまとめる。
# 開発ガイドライン（例）
## 仕様確認
- 開発の入力情報となる、設計ドキュメント、チケットなどの前提が正しいかどうかの確認は開発者が行う
- もし、入力情報が疑わしい場合は開発者自身が有識者に確認、必要に応じてチケットを起票して、ドキュメント修正を行うか、修正依頼を行う
## 機能要件
フロントエンド:
- 設計書通りの見た目となっているか確認する
- 必須/任意、Readonlyなど表示仕様は適切か確認する
- 表示項目の編集仕様（時刻など）は他機能と整合性が取れているか確認する
バックエンド:
- 環境によって変わる値がハードコードになっていないか
## 性能観点
SQL:
- N+1クエリはなるべく避けるようにする
- アプリケーション側でジョインしてはならない
- SQLの検索条件で、パーティションキーの指定が抜けていないように注意する
## 可用性
- 外部API呼び出し時などでタイムアウトは設定されているか
- 外部API呼び出し時に、リトライ設定は行っているか
## 可読性
- [命名規則](https://java-standard.example.com) に従う
- コードコメントは、JavaDoc形式（Markdown）をなるべく使用する
## ログ
- loggerはxxxを利用すること
- ループ内で大量に呼び出される場合は、debugログであっても性能劣化懸念があるため不可とする
## テスト
- カバレッジはCO 90%程度が目安
- バックエンドについては、デシジョンテーブル（決定表）からテストケースを作成する関連: コードレビューの観点 | google-eng-practices-ja
実装する内容が、設計書に基づく新規機能の改修や、不具合改修でチケットに原因分析や対応方針が明記されている場合は、チーム内で改めて対応方針のすり合わせを行う必要はない。
考慮の残がありかつ、すでに稼働済みの機能の挙動変更を行う場合や、原因が特定されていない不具合修正の場合は、以下の内容をチームで合意形成してから実装に入った方が、認識齟齬や手戻りを減らせ、レビュイー／レビュアー双方の負荷を下げることができる
::: tip
スキーマファイルは事前にレビューしておくと手戻りを防ぎやすい
「WebAPIのスキーマ定義」や「DBスキーマ定義」など、システム構成要素の境界となるようなスキーマファイルの変更は、システムに対する影響度が大きいためレビュアーとしても設計を妥協しにくい。また、これらのファイルに指摘が入ると、ほぼ必然的にアプリケーションコードも修正することになるため、手戻りも大きくなる。これら境界となるようなファイルは先行して、Draftプルリクエストでレビュアーに確認してもらってから作業する工夫は有効である。また、そのような開発ルールにするチームも多い。
:::
参考: Design Doc でチームを跨いだ開発を円滑に行う - 一休.com Developers Blog
複数の改修内容が単一のプルリクエストに含まれており、変更内容が多い（20ファイル以上などの）場合、レビュアーの負荷が高くなってしまう。コミットを意味のある粒度に保ちつつ細かくするか、プルリクエストの粒度を細かくするかの2通りの対応が考えられる。
本規約の推奨は以下。
::: tip 大きな変更の場合、目的別にプルリクエストを分ける
プルリクエストの粒度を細かくすることで、レビュアーの負荷を下げることができる。例えば、ある機能改修を行う際に、リファクタリングを行ってから作業をおこないたい場合は、リファクタリングのみを行うプルリクエストと、機能追加を行うプルリクエストを分けることがあげられる。なお、ローカル変数の名称を変更すると言った小さな変更は、わざわざプルリクエストを分ける必要はない。
:::
commitlint を適用し、レビュー負荷軽減のためコミットメッセージの統制を図るという考え方も存在する。
本規約の推奨は以下。
理由は以下。
プルリクエストのタイトルに命名規則をもたせることで、レビュアーの負荷軽減に繋げることができる。また、履歴からのトラッキングが容易になる。
本規約の推奨は以下。
{type}: {subject} や
{type}: {feature_id}: {subject} という構成に従う| type | 説明 |
|---|---|
| feat | 新機能の追加 |
| mod | 機能変更（仕様変更） |
| fix | バグの修正 |
| refactor | リファクタリング |
| docs | ドキュメンテーションの更新 |
::: tip ラベルとの使い分け
プルリクエストのラベルでも同じ意図を持たせることができるが、レビュアーにとっての視認性を重視するため、重複した内容であってもタイトルにプレフィックスを含ませる。
関連: Gitブランチフロー規約
> ラベル
:::
レビュー負荷軽減や、影響度の極小化の観点から、なるべく差分を小さくすることを志向した改修を行うべきか迷う場面がある。
本規約の推奨は以下。
リファクタリングや構成の整理で、ファイルの移動やリネームを行う場面は多々ある。
本規約の推奨は以下。
git mv で操作してからファイルを変更するgit commit
し、その後にファイルの変更を行う何かしらの理由で、マージされたくない作業途中の状態で、プルリクエストを作成したい場合がある。例えば、作業途中の内容を部分的にメンバーに確認してもらいたい場面や、セルフチェックが完了前に別タスクを行う必要が出た場合などがある。誤ってレビューやマージされたくない
本規約の推奨は以下。
::: tip Draftプルリクエストの早期作成のススメ
作業途中であっても、早期にDraftプルリクエストを作成することで、作業の可視化／透明性に繋げることができる。他にも開発者が作業を再開しやすくなるなどメリットが大きいため、ローカルでの開発作業が完了する前の段階でDraftプルリクエストを利用することを推奨する。
:::
トレーサビリティのため、チケット番号（のURL）や、設計検討したドキュメントのリンクや、議論スレッドのURLなどを、チケット本文に記載することで、後からトレース可能にする。チリツモであるがチームの開発生産性を上げることに繋がるため。
作業目的（どういった要望／不具合に対する対応か）などは、作業チケットに記載されているのであれば、プルリクエスト本文での記述は不要である。
なお、サマリ事項だけ転記するなど、詳細な運用ルールは各チームごとに決めて良い。
GitHubでは <details> タグで折りたたんだセクションが作成可能である。例えば、単体テストケース化できないような動作確認を行い、かつ動作検証ログをエビデンスとして貼り付けるような場面では、折りたたんだセクションを活用することで、本文を簡潔に保つことができる。
::: tip スラッシュコマンドを利用する
スラッシュコマンドを用いることで、簡単に折りたたみできる詳細領域を挿入できる。入力時に
`/` を押すとメニューが表示されるため、「Details」を選択する。
:::
2023年5月9日のアップデートで、プライベートリポジトリのプルリクエストなどにアップロードされたファイルの閲覧に、認証が必須となった（それまではURLを知っていれば無認証でアクセスができてしまっていた）。
本規約の推奨は以下。
なお、GIF動画を作成するには、以下の記事で紹介されているツールの利用者も多い。
ジュニアメンバー／シニアメンバー問わず、特に新規参画した当初は新しい環境に慣れるのに手一杯で、誤った操作をしがちである。よくあるミスに、余分なファイルをコミットに含めてしまうことがある。
本規約の推奨は以下。
Files changed
タブを開き、対象のファイルが想定通りか確認するこのように、 Files changed
を見ると、ローカルのエディタで見るのとは違ったセルフレビューの効果があるため、レビュアーになった気持ちで変更内容を確認することも推奨する。
プルリクエストで、特に念入りに確認して欲しいポイントや、リファクタリングなど複数のコミットが混ざり、メインの改修ポイントがわかりにくくなってしまう場合がある。後者はコミットごとにレビューすれば解決するケースもあるが、featureブランチはスカッシュマージするチームでは、必ずしもきれいなコミットを求めないケースも多い。
本規約の推奨は以下。
Files changed
でセルフコメントを付ける::: warning 以下の場合は、セルフコメントは非推奨である。
意図や背景情報をレビュアーに強調したい場合は、コードコメントに対して「実装意図はコードコメントに記載」などセルフコメントすると良い。
:::
プルリクエストのAssigneesは基本的にはレビュイー本人にする。もし、作成したプルリクエストを別の担当者に引き継ぐときは、Assigneesも変更する。これにより、最終的にどの開発者がどのような作業をしたか可視化しやすくなる。
Assigneesで指定された人が、そのプルリクエストの責任を持つ。つまり、マージさせるかクローズさせるか最終的に意思決定されるよう、推進する必要がある。
Reviewersに設定することで、レビュアー側が自分がアサインされていることに気が付きやすくするメリットや、後からどれくらいレビューしたか把握しやすくできる。
本規約の推奨は以下。
ローカルでLinterやテストが通っていると、プルリクエスト作成後、すぐにレビュー依頼を行いたくなる。しかし、何かしらの理由でレビュアーが確認した際にCIによるチェックが失敗している場面もよく見る。
本規約の推奨は以下。
理由は以下。
プルリクエストのタイトルや、#511などの番号だけで依頼を出すのではなく、URLをSlackなどチャットに貼ることを推奨する。
理由は以下。
コードレビュー待ちは他作業が捗らないことが多いし、別作業に移ったとしても集中したタイミングでレビューフィードバックがあり、集中が途切れることもしばしばある。
本規約の推奨は以下。
Markdown記法を活用することで、Descriptionやレビューコメントを視覚的に整理し、意図を明確に伝えることを推奨する。
特に、見出しやコードブロックやリストを使用することで、よりわかりやすいコメントを提供する事ができる。
Gitブランチフロー規約で記載された推奨ルールに従う。チームの自律性と生産性を重視するのであれば、レビュイーがマージすべきである。
信頼関係が構築できていれば、以下のようなやり取りが可能となり、スピード感が上がるためである。
何かしらの理由でレビュアーの関与度を増やす必要がある場合は、レビュアーがマージする。
レビューはコミュニケーションである。コメント記載時には相手への敬意を忘れないこと。
特にレビュアー側はその責務上、指摘コメントを多く行う役割であり、より一層の配慮（上下関係を作らないやりとり）が求められる。
レビューは作成者を詰問する場ではなく、より良いものをチーム一丸となって作るという考えが重要である。
本規約の推奨は以下。
理由は以下。
::: tip コードレビューの優先度を上げる
チーム開発において、レビュー待ちでの待機時間を最小化することでチームでの生産性を最大化できるとされる。
:::
プルリクエストに対して、レビューを実際に行う場合はReviewersに自分を選択する。以下の理由がある。
通常のプルリクエストであれば、featureブランチはデフォルトブランチ（多くはdevelopブランチ）がデフォルトで選択されるため、間違えは少ない。一方でhotfixブランチの場合は、main、developの2ブランチにマージ先がありえるため間違えやすい。
特に慣れていないメンバーの場合は作業ミスも多発しやすいため、最初にマージ先のブランチが正しいか確認するクセを付けると良い。
::: tip 複数のdevelopブランチがある場合も間違えやすい
複数のリリースバージョンを並行して開発する場合、一時的に
develop、develop2
と複数のアップストリームを保つ場合がある。このケースは特にマージ先を間違えやすいため注意する。
:::
GitHubでのコードレビューは、Start a review を行うと
Submit review
しない限り、レビューコメントが自分以外のメンバーが参照できない。
Add single comment の場合は投稿が即参照可能となる。
本規約の推奨は以下。
Add single comment の利用を推奨する理由は以下。
レビュー対象の変更内容が、機能上は問題なくても、他の実装方針と揺れている場合が存在する。
本規約の推奨は以下。
暗黙知を形式期にすることで、チームの方針が明文化され、透明性のある運営に繋げることができる。新規参画者にとってのオンボーディング負荷が下がり、戦力化が早まるメリットがある。
レビューコメントにはラベル（プレフィックス）をつけることを推奨する。レビューコメントの意図を明確にすることで、開発者間の非同期的なコミュニケーションを円滑にし、作業効率を向上させる。
以下にプレフィックスを利用した例を示す。
| プレフィクス | 説明 | 例 |
|---|---|---|
| LGTM | Look Good To Me（良さげです） | [LGTM] 責務分解が明瞭で、良い実装です！💯 |
| ASK | 質問 | [ASK] この実装意図が理解できず、どうして必要なのか教えてもらえますか❓️🤔 |
| Q | ASKと同じ意味 | [Q] 同上 |
| MUST | 必須 | [MUST] ソート処理が漏れているため、順序が不正になってしまう可能性があります！ |
| SHOULD | 提案 | [SHOULD] 愚直なループではなく、□□□の書き方の方が可読性および性能が良いです。書き換えを検討してください。 |
| IMO | In My Opinion（自分ならこうしますが、どうでしょうか？） | [IMO] スコープの広さの割に変数名が短いため、 `name` ではなく ` pre_ordered_product_name` などが良いかと思いました |
| NITS | Nitpick（枝葉ですが直して欲しい） | [NITS] この変数名は、単数形よりも複数形 `users` または `usersList` が適しているかと思います。 |
それぞれの定義は以下。特に「ASK（Q）」と「MUST」はレビューコメントが解決しないと、プルリクエストの承認を行うべきではない。
| 分類 | プレフィクス | 承認のために解決が必須 | レビュイーの対応 |
|---|---|---|---|
| 礼賛 | LGTM | リアクション不要 | |
| 質問 | ASK | ✅️ | 回答が必須 |
| Q | ✅️ | 同上 | |
| 修正依頼 | MUST | ✅️ | 対応しない場合はコメントバックまたは、コード変更の対応が必須 |
| SHOULD | 対応しない場合はコメントバックまたは、コード変更をできる限り実施する。対応見送りの場合は、チケット起票および、次リリースまでの対応が目安 | ||
| 提案 | IMO | 絵文字リアクションやコメントバックが推奨だが、対応の是非はレビュイーが判断する | |
| NITS | 同上 |
参考: コード品質向上のテクニック：第51回 確信的な質問 | LINEヤフー にも、レビューアの「要求」であるのにもかかわらず、単なる「質問」にも読み取れてしまうミスコミュニケーションの例があり、 コードレビューにおいては要求と質問は明確に区別できるように書かれるべき とある。
心理的安全性を担保しつつレビューを楽しい場とするためには、ポジティブな絵文字（🚀✨️💯）を使うとともに、レビューコメントに適度にユーモアを入れると良い。
以下に例を示す。
レビューコメントで「可読性が低いです」 といったコメントは、レビュイーが受け取るとどのように修正して良いか判断ができず、次のアクションに繋がりにくいため不毛である。そのため、面倒であっても、どのように直して欲しいか、できる限り具体的に指示する方が建設的である。
本規約の推奨は以下。
指摘内容の補足情報があれば追加するとベターである。例えば、「◯◯の観点で、このコードは△△の懸念があるため、□□□になるように修正をお願いします。公式ブログの{URL}にも記載があり、参考にできると思います。」といった形式である。
レビュイーが自分で調べれば事足りる場合もあるが、レビュアー側が出典を示すことで納得感が増す。また、誤った記事（過去のバージョンの記事や、見当違いの記事）を読んでしまい空回ってしまうことも、探す手間も減る。
「なんとなく汚いです」は指摘ではない。悪いコードだと感じたのであれば、相手が納得できるように論理的に説明する。論理的に説明できないのであれば、指摘をすべきではない。
レビュイーからすると、レビューの「依頼1→指摘1→対応1→確認依頼1→別の指摘2→対応2→確認依頼2→別の指摘3→…」と、依頼のたびに指摘をもらうとタスクの予実管理も作業見積もりも建てられずストレスである（ムービング・ゴールポストである）。
本規約の推奨は以下。
プルリクエスト上のコメントで、レビュイーに対して直接変更内容を提案を行うことができるSuggested changesという機能がある。
本規約の推奨は以下。
Suggested changesを使わないほうが良い場合もある。
参考: 君は GitHub の Suggested change を知っているか？ - BASEプロダクトチームブログ
対応が必須ではない、いわゆるnitsな指摘事項だけの場合がある。
この場合の推奨は以下。
レビュー対象のリモートブランチを、ローカル環境にてテストなど動作確認すべきか迷う場合がある。レビュアーがローカルで動作検証を行うと、おそらく品質は高まるはずだが、レビュイーとの作業重複でもあるため工数は非常に高くなり、費用対効果は必ずしも良いと言えない。
本規約の推奨は以下。
レビュアーは意図が掴みきれない部分があった場合は、そのままにせず理解できるように務める必要がある。分かったふりをすると、後でその部分が火を噴くことが多いからである。また、「チーム全員が同じ知識を共有する」というコードレビューの意図からすると、レビュイーだけ分かっていればよいという状態は不健全である。レビュイーがリーダー（テックリード）であっても臆する必要はないし、レビュイーが自分より若手であったとしても、自分が調べても分からないことを聞くことは恥ではないので、堂々と教えてもらう。
本規約の推奨は以下。
レビュアーという立場だからといって、必ずしも開発チームが利用する全技術スタックや、開発プロダクトの過去の経緯に精通している訳ではない。それでも、自信が無い領域についてコードレビューを求められるケースも多々ある。
本規約の推奨は以下。
対象領域のスキル不足などが原因で、レビュー事項が大量にある場合に対応をどうすべきか迷う場合がある。
本規約の推奨は以下。
なお、類似の指摘がある場合に、省略するかどうか判断に迷う場面もある。これについては以下を推奨する。
レビュー終了した場合、レビューが終わった旨をプルリクエスト上でメンションをつけてコメントしたり、Slackなどのチャットで通知することで、レビュアーに対応を促すことができる。「Add single comment」の場合は、レビュー終了の切れ目が分からないためである。
本規約の推奨は以下。
理由は以下。