レビューが爆速になる！レビュアーに優しいPull Requestの極意
はじめに
こんにちは、JAXA認定の宇宙ベンチャー企業、株式会社 天地人 (てんちじん) でエンジニアリングマネージャーをしている白井と申します。普段は天地人コンパス (Tenchijin COMPASS) のフロントエンドまわりの開発を行っています。
推し人工衛星はだいち4号です（昨年7月に種子島まで打ち上げを見にいきました）
レビューを依頼したときにありがちなこと
チーム開発を行う上で、Pull Request（以下PRと略します）ベースのコードレビューは当たり前になってきていると思います。
コードレビューは、他のメンバーにあなたのコードを見てもらい、フィードバックをもらうための重要なステップです。
しかし、レビューを依頼した際に以下のような状況に直面したことはないでしょうか？
- 😇 レビュー依頼してしばらく経っても返信がなく、状況を質問してようやく 「あ、今から見ます」 と言われてしまう
- 😇 レビューが返ってくるまでに何日もかかる
- 😇 何日もかけて作ったPRにもかかわらず、そもそもの前提をひっくり返されて全部書き直しになる
- 😇 レビューでの指摘事項が大量になって修正コミットを積みすぎてしまった結果、コンフリクトの解消が困難になった
- 😇 レビューしてもらえたものの、バグや問題点が見過ごされてしまった
このような場合、多くはレビューを依頼する側の工夫で改善できると私は考えています。（もちろん、レビュアー側にも原因がある場合もありますが）
レビュアーの視点では何が起こっているか？
レビュアーから見ると、上記のような状況が起こる場合、以下のようなPRがレビュー依頼されている場合が多いです。
-
🤔 そもそも文脈がわからない
- レビュアーは別の作業に取り組んでいるため、あなたが四六時中考えてきた内容を知っているわけではありません。適切に文脈を補ってあげる必要があります。
-
🤔 レビューの優先度がわからない
- 同様に、レビュアーにはレビュアーの仕事があります。あなたが緊急と思っているタスクも、レビュアーにとってはそうではありません。優先度とその理由を適切に伝える必要があります。
-
🤔 方針レベルで改善点がある
- 細部を精緻に仕上げることは重要ですが、そもそも方針レベルで間違っていれば手戻りが大きくなり、お互いにつらいことになります。（レビュアーだってひっくり返すのはつらいのです。。）
- まずは方針レベルですり合わせを行い、フィードバックをもらうようにしましょう。
-
🤔 PRが大きすぎる
- 大量の変更点が含まれるPRを受け取ると、レビュアーは「ウッ！」となってその場で見なかったことにしがちです。
- また、変更点が多くなるほど、レビュー時にスルーされやすくなります。PRはなるべく小さくすることが重要です。
-
🤔 変更の意図が伝わりにくい
- コードは究極の詳細です。コメントやドキュメント等の補足説明なしに、コードから全体の方針やあなたの意図を汲み取ることは非常に難しいです。
- 一方的にコードを投げつけるだけでなく、適切な補足説明を添えてあげましょう。
-
🤔 コミットが整理されていない
- 1つのコミットに複数の変更が混入していたり、作業内容がそのままコミットログになったようなPRは、読み解くのに非常に時間がかかります。
- 適切な粒度や順番でコミットを積むことで、レビュアーの負担を軽減することができます。
重要なのは、レビュアーの負担をいかに減らすかということです。
コードレビューは相手の時間を使って行われる行為です。レビュアーは自分の仕事の集中を切って、あなたのために時間を使っています。相手の負荷を減らすために、最大限の努力をすることが重要だと考えます。（コードレビューだけでなく、さまざまな仕事に言えることですが）
この記事で説明すること
この記事では、天地人社内で使われているレビュアーに優しいPull Requestの作り方のガイドラインについてご紹介します。
ガイドラインは以下のパートから構成されています。
- PRを作る前にやること
- いきなりPRを作るのではなく、まずは「方針」レベルで検討を行い、フィードバックをもらうようにします。
- ブランチの切り方
- PRを適切な大きさにするためのブランチの切り方について解説します。
- コミットの積み方
- レビュアーが読みやすく、自分でもメンテしやすいコミットの積み方について解説します。
- PRの説明文の書き方
- コミットから読み解きにくい、その他の情報を共有する方法について解説します。
この記事でご紹介する内容を実践していただくことで、以下のような理想的なコードレビューに近づくことができたら嬉しいです。
-
フロー効率が高まる
- レビューが高速に返されるようになると、リリースまでの時間を短縮することができ、より早くユーザーに価値を届けることが可能になります。
-
より良いフィードバックをもらうことができる
- PRの内容がレビュアーに十分に理解されるようになり、あなたの成長につながるより良いフィードバックをもらえる可能性が高まります。
-
バグの見逃しが少なくなる
- 理解しやすいPRは、コードレビュー中にバグや設計上の問題を見つけやすくします。
それでは、各パートについて具体的なやり方を説明していきます。
PRを作る前にやること
レビューしやすいPRを作る準備は、コードを書き始める前から始まっています。
✅ コードを書き始める前に、まずは「設計」する
設計なしにいきなりコードを書き始め、そのままコードレビューを依頼していないでしょうか。
大きな変更を行う場合、まずは設計を行いましょう。
- コンポーネントをどう分割するか
- クラスの責務は何か
- どういうテーブルが必要か
- など
設計なしに大量のコードだけレビュー依頼された場合、以下の弊害があります。
- コードという「究極の詳細」から「方針」や「設計」を読み解く必要があり、非常に困難
- そもそも方針や設計自体がない場合も。。
- 既に実装されてしまっているので、レビュアー側は根本的なフィードバックがしにくい
その結果、微妙な設計のままコードがマージされることになります。
まずは設計を行い、設計に対してフィードバックをもらうようにしましょう。
✅ プロトタイプ実装のススメ
とはいえ、いきなり良い設計が見えるわけではありません。
まずは 「プロトタイプ実装」 を行うことをおすすめします。
プロトタイプ実装とは、コードの品質やユニットテストの追加は最低限にし、 「とにかく動くものを作ってみる」 ことです。
そのプロセスを通じて思考が深まり、設計上の論点が洗い出されてきます。それを整理し、あらためてしっかりと設計を行います。
間違っても、プロトタイプ実装をそのままレビューに出してはいけません。それはあなたの下書きにすぎないのですから。
✅ 設計資料を作る
設計したら、Notion等のドキュメントに設計資料をまとめましょう。
会社やチームの慣習や変更の大きさによって必要となるドキュメントは変わってくると思いますが、とにかくコードの詳細からはわかりづらい、全体的な方針をまとめることが重要です。
この資料ベースで設計に対するフィードバックが受けられるほか、PRをレビューに出す際、再度設計について詳細に記述する必要がなくなります。（設計資料のリンクを貼るだけになる）
✅ ペアプロ・モブプロのススメ
レビュアーがチームメンバーであれば、ペアプロやモブプロを通じて文脈を共有するのもおすすめです。
一緒にコードを書くことで目線が揃っていれば、あらためて用意すべき設計資料は少なくなります。
ブランチの切り方
ブランチの切り方には戦略が必要です。
1つのPRで一気にすべてを解決しようとするのではなく、適切な粒度の複数のPRに分割し、レビュー負荷とリリース時のリスクを下げましょう。
✅ ブランチを小さく分割する
1つのPRが大きくなりすぎないように、ブランチの分割を検討しましょう。
大きすぎるPRには以下の弊害があります。
-
レビューがつらくなる
- 前述のとおり、大きすぎるPRをレビューするのは大変負荷がかかる仕事です。
- その結果、レビューが返ってくるまでの時間が長くなってしまいます。
-
修正が困難
- 指摘事項を反映するときに、どのコミットに対する修正なのかわかりにくくなります。
-
コンフリクトの解消が困難
- マージ先のブランチとのコンフリクトが発生した場合に、解消が困難になります
チームのブランチ戦略やデプロイ方法にもよりますが、ある大きめの機能を追加する場合、たとえば以下のように細かくブランチとPRを分割します。
- 既存機能に不足していたユニットテストの追加
- 必要なリファクタの実施
- DBのマイグレーションを追加
- バックエンドのAPIを修正
- UIへの機能追加（これも大きくなりすぎないように適切な単位で分割する）
- など
ユーザーから見えない変更は一気に行う必要がない場合が多いです。
このようにPRを小さくすることで、レビュアーの負担も減りますし、リリース時のリスクも下げることができます。
✅ 大規模なブランチはできる限り作成しない
上記と同じ理由で、大規模なブランチの作成は避け、可能な限り小さなPRを頻繁にマージするようにします。
たとえば、以下のような開発スタイルを見かけることがあると思います。
- 大規模な機能追加のためのブランチ
topic/hogeを作成する - 関連する修正を上記から派生した
feature/fugaブランチで行い、topic/hogeブランチにマージする - 最後に
topic/hogeをメインブランチにマージする
このようなブランチ運用には以下の弊害があるため、できる限り避けることが望ましいでしょう。
-
PRのマージ先を間違える可能性が高まる
- 上記の例では
feature/fugaブランチのマージ先はtopic/hogeブランチであるべきですが、あやまってメインブランチに向けてPRを作成してしまう場合があります。 - 機械的なチェックがあれば別ですが、レビュアーもレビュー時に気づくことは難しいでしょう。
- 上記の例では
-
メインブランチの変更を取り込む作業が面倒
-
topic/hogeブランチにメインブランチの変更を取り込む場合、topic/hogeブランチに取り込んでから、派生したfeature/fugaブランチに変更を取り込む必要があり、手間がかかります。
-
-
コンフリクトの可能性が高まる
-
topic/hogeブランチの生存期間が長いため、コンフリクトのリスクがどんどん高まっていきます。
-
ただし、フレームワークのメジャーアップグレードなど、半端な状態でマ－ジしてしまうとアプリケーションの動作やUIに影響を与えてしまうような大規模な開発についてはその限りではありません。
その場合でも、メインブランチに先にマージできる変更が出てきた場合は別のブランチに切り出し、ブランチに含まれる変更点が小さくなるように努力します。
✅ ダウンタイムが発生するPRを作らない
たとえば、以下の変更を1つのPRで一気に行ってしまうと、APIが疎通できない瞬間が生じ、ダウンタイムが発生します。（フロントエンドとバックエンドを1つのリポジトリで管理している場合の例）
- バックエンドに新しいAPIを追加
- フロントエンドは新しいAPIを利用するように修正
- バックエンドから古いAPIを削除
このような破壊的変更はPRを分け、リリースタイミングが別になるように調整します。
✅ フィーチャーフラグの活用
フィーチャーフラグとは、新機能のリリースや変更をフラグ1つで簡単に行えるようにする仕組みです。
これを使うことで以下のような開発を行うことができます。
- ある機能を実装し、メインブランチにマージ
- この時点ではユーザーは利用できない状態になっている
- 特定のユーザーや社内環境でのみ有効化してテストを実施する
- テストの結果を踏まえ、ブラッシュアップする
- フィーチャーフラグを有効化し、全ユーザーに機能を開放する
この仕組みを使えば、開発中のコードであってもメインブランチにどんどんマージしていくことが可能になり、1つ1つのPRを小さく保つことが可能になります。
ただし、それなりに複雑な仕組みでリスクもありますので、用法・用量を守って適切にお使いください。
✅ ブランチはなるべく最新のメインブランチから切る
開発が長期化すると、派生元のブランチとの乖離が生じやすくなり、コンフリクトの発生確率が高まります。
PRの作成前には、 git rebase <メインブランチ> を実行し、最新のメインブランチから切り直すようにします。
コミットの積み方
適切な粒度でコミットを積んでいくと、非常にレビューしやすくなります。
✅ 作業ログをそのままレビューに出さない
レビュアーはあなたの作業工程に興味がありません。
例えば、以下のようなコミットは非常に読みづらく、レビューがしづらいです。
-
49e6342 修正を行った- レビュアー：「ここに問題があるのでレビューコメントをつけておこう」
0014dd1 別な修正を行った-
fc685c7 やっぱり必要がなくなったので、最初の修正は取り消し- レビュアー：「さっきコメントする必要なかったじゃーん！！！」
修正内容が完成したらコミットを整理し、 「最初から全てを知っていた状態で最短距離で実装した」 かのように最低限のコミットを積んでいくようにします。
コミットの整理には git reset や git rebase コマンドが利用できます。この記事では詳しく解説しませんので、気になる方は調べてみてください。
✅ 意味のある単位でコミットを分割する
コミットは意味のある単位で積むようにします。
良い例：
- リファクタリング
- リンターやフォーマッターの適用
- ユニットテストの追加
- APIの修正
- UIの変更
悪い例：
- ここまで作業したから一旦コミット
- デバッグ用に
console.log()を追加 - デバッグが終わったので
console.log()を削除 - ここまでテストした
✅ 別な種類の作業を1つのコミットに混ぜない
例えば、「機能追加」と「リファクタリング」を1つのコミットに混ぜないようにします。
目的の異なる作業が混ざっていると、非常にレビューしにくくなります。
✅ Conventional Commitsのススメ
前述のように、コミットを目的ごとに分割するためには、Conventional Commitsに従うのがおすすめです。
これはコミットメッセージに以下のような接頭辞をつけるやり方です。
-
fix:バグの修正 -
feat:新しい機能の追加や変更 -
docs:ドキュメントやコメントのみの追加や変更 -
test:テストコードの追加や変更 -
refactor:リファクタリング（挙動は変更しない） -
style:インデント変更やフォーマッターの適用など -
perf:パフォーマンスの改善 -
ci:CIの改善 -
chore:その他、コード本体と別の部分の修正
これを意識すると、自然と目的ごとにコミットを分割できるようになります。
✅ 1つのコミットを大きくしすぎない
目的が1つであっても、1つのコミットに大量の変更を含めないようにします。
変更点が大量だと、レビュアーの認知負荷が高まり、確認が大変になります。
✅ 機械的な変更と手動の変更はコミットを分ける
フォーマッターやリンターなどで機械的な修正を行ったものと、手動で変更したものを混ぜてはいけません。
機械的な変更は（それが信頼できるツールであれば）細かくレビューする必要はありませんが、人間が行った変更にはミスがある可能性が高いからです。
✅ インデントの変更はコミットを分ける
インデントが大幅に変更された場合、GitHub上での差分が正常に表示されない場合があります。
その場合、インデント変更のみのコミットを積むことをおすすめします。（というか、とてもレビューしづらいのでしてほしいです）
なお、コミットを分けなくとも、GitHubの Hide whitespace の設定をオンにすると差分が正常に表示される場合もあります。
✅ パッケージの追加は独立したコミットにする
パッケージを追加し、 package.json や package-lock.json 等のファイルに差分が出た場合は独立したコミットにするようにします。
これらのファイルは他のエンジニアが更新した場合にコンフリクトしやすく、他の作業とコミットを混ぜてしまうとrebaseなどが難しくなるためです。
✅ 新規ファイルの場合は変更を複数のコミットに分けない
新規ファイルの場合、1つのコミットで追加するようにします。
例えば、以下のようにコミットを分けてしまったとします。
- ファイルを追加
- メソッドAを実装
- メソッドBを実装
- リファクタリング
上記の場合、「ファイル名はXXXの方が適切では？」といったレビューコメントが付いた場合、修正が非常に面倒になります。
✅ 動作を壊した状態でコミットしない
PR単位で動作を保証するのではなく、1つ1つのコミットで動作を保証するようにします。
ただし、変更が大きすぎてコミットが大きくなりすぎる場合はその限りではありません。
✅ コミットメッセージをちゃんと書く
コミットメッセージには具体的な内容を書くようにします。
良いコミットメッセージの例：
b3af28f fix: ログイン時にセッションタイムアウトが発生する問題を修正8ddadf5 test: Userクラスにユニットテストを追加3d6f1f3 refactor: UserServiceをリファクタし、可読性を向上
悪いコミットメッセージの例：
1b22af5 問題を修正be017cd とりあえずコミット82641bf レビューの指摘事項を適用
また、コミットメッセージは複数行書くことができます。追加で以下のような項目を記載すると、レビュー時や後から変更内容を調査する人にとって貴重な情報になります。
- 何を変更したか
- なぜ変更したか
- 注意すべきこと
- 参考になった情報
PRの説明文の書き方
PRの説明文を注意深く書くことで、レビューの負荷を軽減できます。
レビュアーは別の作業をしているため、あなたのようにずっとその問題に没頭していたわけではありません。
そのため、文脈を補ってあげないことには適切なレビューができません。
✅ 参考資料をリンクする
PRでの変更に至った経緯や、設計資料があればリンクを貼ります。
- 関連するIssue
- Slackのスレッド
- 設計資料
- 参考にしたライブラリのドキュメント
- など
✅ 説明を具体的に書く
以下のような内容を読みやすく記述します。
- なぜ変更したか
- どのような方針で変更したか
- 何を変更したか
- テストはどうやって行ったか
- 実際の挙動を確認するにはどうしたらいいか
- 他システムへの影響はあるか
- など
なお、書きすぎても書く側・読む側双方の負荷が高まりますが、書かないよりは書いた方が良いです。
✅ PRだけでコミュニケーションしない
一方、テキストだけでは十分な背景情報を伝えられない場合があります。その場合、別途口頭などで相手の理解度に合わせて補足するようにします。
✅ レビューの緊急度を明示する
バグ修正など、早くレビューを通してほしい場合はその旨を伝えます。
それにより、レビュアーはレビューの優先度を調整したり、指摘事項をどこまで反映するかを調整することができます。
その場合であっても、レビューは相手の時間を奪う行為であることに変わりはないので、最大限の配慮を行うようにしましょう。
✅ 通常と異なるブランチにマージする場合は明示する
通常と異なるブランチにマージする場合（例：本番環境に緊急でパッチを当てる）はその旨がわかるように明示します。
✅ UIの変更点は画像や動画で伝える
テキストだけで変更点を伝えることは難しいため、画像や動画を貼ってあげると変更点を視覚的に伝えることができます。
✅ 何をテストしたのか伝える
ユニットテストの変更点はPR上で把握できますが、手動で行ったテストや観点についてレビュアーは把握できません。
どのようなテストを行ったのかを記述すると、レビュアーはテスト項目や観点の漏れについて指摘することができます。
✅ PRのテンプレートを作成する
上記のような内容を網羅できるように、チームの状況に合わせたPRのテンプレートを用意するのがおすすめです。
まとめ
以上、レビュアーの負担を減らし、効果的なコードレビューを受けるためのプラクティスを紹介してきました。
AIが生成するコード量が増えてきた昨今、コードレビューのあり方もどんどん変わってきているかと思いますが、今回ご紹介した内容を参考に、より良いチーム開発のやり方を考えるきっかけになりましたら幸いです。
株式会社天地人では、人工衛星などの宇宙ビッグデータを活用し、地球規模の課題に取り組むためのオンラインGISプラットフォーム天地人コンパス（Tenchijin COMPASS）を開発しています。
私たちと一緒に天地人コンパスを開発してくれる仲間を募集しております。ご興味のある方は以下のページよりエンジニアリングの募集の求人にてご確認下さい。
Discussion