TL;DR
的を射たレビューをするのが難しく、ふとしたときにどうなっていれば良いレビューなのか？と何度か考えたことがあるが自分がきちんと良いレビューを行えていないなと感じたので
自分がされて良いと感じたレビューの共通項や悪いレビューをしてしまったなと感じたときの原因などを記載して良いレビューをするためのチェックリストを作成。
随時更新していく。
よい意見などがあれば採用、追加していくので多分永遠にFixしない。
良いレビュー
-
コードの裏にある背景が理解した上で実装されたコードがそこから逸脱していないか指摘する
- 書かれているコードがチケットやissueの課題を解決するか？
- 書かれているコードがチケットやissueの課題の本質的な問題を解決しているか？
- 書かれたコードが暫定的な対応になっていないか？(最初から暫定対応すると決めている場合は除く)
-
完了条件が明確なチケットであること
- 解決したいことではなく、問題が共有されていること
- 変更されたコードだけでなく変更されていないが影響を受けるコードに関して指摘がある
- 「私はこう思う」ではなくロジックで「こうなっているほうがよい」という説明になっている
-
議論可能なレビューコメントになっている
- 「何故疑問に思ったのか？」という背景が書かれている
- 同じ指摘内容でも「わかるでしょ？」という感じで省略しない。コピペでよいのでちゃんと指摘する。あまりに多い場合はその旨をきちんと伝える。
-
レビュー指示が明確である
- 確認なのか、質問なのか、修正依頼なのか、代替案なのかひと目でわかるようなコメントになっている
悪いレビュー
-
レビューコメントを読んだがレビューされた側がどうアクションすればいいのかわからない
- 質問なら質問、改善なら改善、意見なら意見ということがわかるように伝えていない
-
完了条件があいまい
- レビューというかチケットの問題だがチケットの完了条件、解決したい問題がレビュアーに伝わらないとレビューできない
-
テストコードがない
- レビューできる状態にないので論外
-
コーディング規約に違反したコードでレビュー依頼がされる
- Linterなどで自動で検知する、あるいは何かしらのツールでリフォーマットされる、テスト項目に違反しているコードが含まれていないかチェックするものを追加する
- 基本的にレビュー依頼した段階で論外。
ユースケース1: テストの粒度が大きすぎる
多くの場合テストの粒度がレビュイーからすると適切でないとき、問題のメソッドやAPIが変更されることが考慮されていないことが多いように思う。
背景としてそのテスト対象が不変なものであるかどうかが焦点になると思う。
- 不変であることが自明の場合、そこに対して無用なテストを書いていたら無用なテストを書いていることを指摘する
-
不変であるかどうかわからない、もしくは変わり得る可能性がある場合は書かれているテスト対象のメソッドやAPIが変更されることが考慮されていないケースが多いと思うので「このテスト対象が変わったらどうなりますか？」と質問する
- このとき「テスト対象が変わったときにテストが失敗するように書かれていないので修正してください」と書いてしまうとレビュイーの考える余地がほぼなくなってしまうので気をつける。
ユースケース2: テストケースが不足している
テストケースが不足している場合考えられることは多々あるが一番やりがちなのが複雑な振る舞いや仕様のためわかりやすい異常系のテストだけ実装されているケース。
- 複雑な仕様や複数の状態、ふるまいが考えられるチケットの場合は事前にチケット登録者か担当者を決める人がチケットの完了条件の1つに「TDDで実装されること」という条件を追加する