はじめに
PRレビューは、より良いプロダクトを作るために欠かせないプロセスです。
実装者が気づけなかった観点をレビュアーが補い、設計、保守性、セキュリティ、テスト、既存仕様への影響などを確認することでプロダクトの品質は上がっていきます。
一方で、レビュー指摘が多いと少し申し訳なさを感じたり自分の実装力が足りないのではないかと感じたりすることもあると思います。
少なくとも私は指摘が多いPRを見ると「もっと最初から気づけたのではないか」と思うことがあります。
ただ、レビューコメントを少し違う見方で捉えると「単なる修正依頼ではなく、チームが大事にすべき設計原則」とも言えそうです。
そう考えると自分のPRへの指摘だけでなく、他人のPRについたコメントも次の実装に活かせる学習材料となります。
この記事では、過去のPRレビューコメントを見返し、自分用のチェックリストに変換してみた話を書きます。
目次
- なぜレビューコメントを見返そうと思ったのか
- やったこと
- レビューコメントから見えてきた観点
- PR作成前チェックリスト
- やってみて気づいたこと
- まとめ
1. なぜレビューコメントを見返そうと思ったのか
レビューコメントはその場の修正対応として処理しがちです。
- 指摘を受ける。
- 修正する。
- 再レビューしてもらう。
- マージされる。
もちろんこの流れ自体は自然です。
ただ、指摘を直して終わりにしてしまうと次のPRでも似たような指摘を受けることがあります。
それはレビューで得た観点を再利用できる形にできていないからかもしれません。
レビューコメントには実装者本人が見落としていた観点だけでなく、チームが大事にしている判断基準が含まれています。
例えば、以下のようなものです。
- 責務をどう分けたいのか
- 既存実装をどの程度踏襲すべきなのか
- 命名で何を大事にしているのか
- データ取得やキャッシュをどう扱うべきなのか
- テストで何を保証したいのか
- PR本文でどこまで説明してほしいのか
これらは明文化されたドキュメントに書かれていないこともあります。
だからこそレビューコメントを見返して分類すれば、次回以降のPRに活かせるチェックリストになるのではないかと考えました。
2. やったこと
やったことはシンプルです。
過去半年分くらいのPRレビューコメントを見返し、指摘コメントを抽出しました。
このとき、すべてのコメントを見るのではなく以下のようなコメントを中心に見ました。
- 実装方針への指摘
- 責務分離への指摘
- 命名への指摘
- 既存実装との整合性への指摘
- テスト観点への指摘
- セキュリティやバリデーションへの指摘
- レビュー前の説明不足への指摘
集めたコメントはそのまま読むだけではなく似た指摘ごとにカテゴリ化し、
各カテゴリを「次回PR前に自分で確認できる質問」に変換しました。
3. レビューコメントから見えてきた観点
レビューコメントを分類すると、いくつかの観点が見えてきました。
もちろんプロジェクトや技術スタックによって細かい内容は変わります。
ただ、今回見返したコメントの多くは特定の言語やフレームワークだけに閉じたものではなく、他の開発現場でも使える観点だと感じました。
| 観点 | 指摘されがちな内容 | PR前に確認する問い |
|---|---|---|
| 責務分離 | この処理はここに書くべきではない。別の層や関数に分けるべき | なぜこの場所に書くのか |
| 既存実装の踏襲 | 似た処理が既にあるのに、別の書き方をしている | 似た既存実装はないか。なぜ既存実装ではダメなのか |
| 命名 | 変数名や関数名から意図が読み取りづらい | 名前から役割や意図が伝わるか |
| データ取得 | 必要以上にデータを取っている。取得場所や取得条件が不自然 | 何を入力にして、何を返すのか。必要以上に取得していないか |
| validation / security | 入力チェックや権限チェックが不足している | 想定外の入力や権限不足をどう扱うか |
| cache | キャッシュの更新・削除・不整合を考慮できていない | cacheの更新、削除、不整合の可能性を考慮したか |
| migration / generated files | migrationや自動生成ファイルの扱いが不適切 | migrationや自動生成ファイルの扱いは適切か |
| 既存挙動の保持 | 既存仕様や既存ユーザーへの影響を考慮できていない | 既存の挙動を変えていないか |
| テスト粒度 | 実装詳細に寄りすぎている。振る舞いをテストできていない | テストは実装詳細ではなく振る舞いを見ているか |
| レビュー前の説明責任 | なぜこの実装にしたのか、PR上で説明できていない | なぜこの実装にしたのかをPRで説明できるか |
こうして見ると、レビュー指摘はチームが大事にしている観点の集まりというのがわかります。
また、この問いに答えられない場合実装が悪いというよりまだ説明できる状態になっていない可能性があります。
PRを出す前にこの観点で見直すだけでも、レビューの質は変わるのではないかと思います。
4. PR作成前チェックリスト
実際に使うなら、チェックリストは長すぎると続きません。
そのため、短時間で見る版と、じっくり見る版に分けると良さそうです。
PR作成前5分版
- [ ] なぜこの場所に実装したのか説明できるか
- [ ] 似た既存実装を確認したか
- [ ] 変数名・関数名から意図が伝わるか
- [ ] 既存挙動を変えていないか
- [ ] 異常系や権限チェックを考慮したか
- [ ] テストは振る舞いを確認しているか
- [ ] PR本文に判断理由や見てほしい点を書いたか
じっくり版
## 設計・責務
- [ ] この処理を書く場所は適切か
- [ ] 1つの関数やクラスに責務が集まりすぎていないか
- [ ] 既存のレイヤー構造を壊していないか
## 既存実装
- [ ] 似た実装が既にないか
- [ ] 既存の書き方と不自然に違っていないか
- [ ] 既存挙動を変えていないか
## 命名
- [ ] 変数名・関数名から役割が分かるか
- [ ] 意味の曖昧な名前になっていないか
- [ ] マジックナンバーが残っていないか
## データ取得・状態管理
- [ ] 必要以上にデータを取得していないか
- [ ] データ取得場所は適切か
- [ ] cacheの更新・削除・不整合を考慮したか
## validation / security
- [ ] 入力値のvalidationは適切か
- [ ] 認証だけでなく認可を確認しているか
- [ ] 想定外の入力や権限不足を扱っているか
## migration / 自動生成
- [ ] migrationの内容は意図通りか
- [ ] 自動生成されるファイルを手で直接編集していないか
- [ ] schema変更に伴う影響を確認したか
## テスト
- [ ] 正常系だけでなく異常系を確認しているか
- [ ] 実装詳細ではなく振る舞いをテストしているか
- [ ] 変更によって壊れうる既存挙動をテストしているか
## PR説明
- [ ] なぜこの実装にしたのかを書いたか
- [ ] レビュアーに特に見てほしい点を書いたか
- [ ] 迷った点や代替案を書いたか
チェックリストは完璧に全部埋めるためのものというより、PRを出す前に一度立ち止まるためのものだと思います。
5. やってみて気づいたこと
レビューコメントを見返して一番大きかったのは、指摘の見え方が変わったことです。
以前は指摘が多いと「もっと最初から気づけたのではないか」と感じることがありました。
しかし、コメントを分類してみるとそれは単なる修正依頼ではなくチームが大事にしている設計原則や暗黙知のログに見えてきました。
特に同じような指摘が繰り返されている場合、それは自分を責める材料ではなく次に見るべき観点です。
また、自分のPRだけでなく他人のPRを見ることにも価値があり、
他人への指摘は自分がまだ踏んでいない地雷を先に教えてくれるものとなります。
レビューコメントはその場限りの修正依頼ではなく、次のPRに活かせる知識として再利用していくことこそ自身の成長となり、レビューをする側になる上でも必要な事です
6. まとめ
PRレビューはより良いプロダクトを作るための大事なプロセスです。
一方で、指摘が多いと申し訳なさを感じたり自分の実装力に不安を感じたりすることもあります。
だからこそレビューコメントをその場限りの修正依頼で終わらせず、次に活かせる知識に変えることが大事なのではないかと思います。
レビューコメントにはチームが大事にしている設計原則や暗黙知が含まれています。
それを見返し、分類し、自分用のチェックリストに変換すれば次のPRで使える実践的なレビュー観点になります。
レビュー指摘を落ち込む材料にするのではなく、より良いプロダクトを作るための材料に変える。
その積み重ねが実装力やレビュー力を高める一つの方法であり、自身の成長技術力向上なのではないでしょうか。