コードレビューの生産性を上げるには
こんにちは、Media Analyticsを運営しております、DAIです。
コードレビューについての学びについて、メモしておきます。
TL;DR
- 全体方針の問題と、個別修正の問題を切り分けよう
- 全体方針の問題は、PRのみで議論するのではなく、全体方針の問題として議論を切り分けよう
- 3往復以上するレビューコメントに関しては、ビデオ会議して話そう
全体方針の問題と、個別修正の問題を切り分けよう
コードレビューを行う際に気をつけたいのが、PR中では、全体の方針の問題に関して議論すべきではないという点です。そこはしっかりと「全体方針の問題」として切り分けて、エンジニアチーム全体で議論しましょう。
全体の方針の問題とは結構決めの問題になるケースが多いです。
例えば
- プルリクエストの ブランチの名前をイシュー番号にするのか、機能の名前にするのか
- コードのコメントは英語にするのか、日本語にするのか
- RSpecでitかexampleを使うか
などですね。
このように全体方針の問題は、個別のPRで話し合っても解決することができません。
だいたい担当者の個人哲学の話になって、局所最適な話になります。
全体の方針の問題は、宗教論争になりがちなので、チーム全体でコンセンサスを取らない限りは方針がまとまりません。
このような問題が発生したときに、全体のルールを先に決めておくと自動で意思決定ができます。
例えばこういうルールがあれば、少なくとも単体テストと結合テスト、どちらで何を保証すべきか、みたいな議論を延々とする必要はなくなります。（あくまでも例です）
基本的には単体テストと結合テストを作成することが望ましい。
単体テストで動作を確認する事はその単体テストのファンクションが動いてるかどうかを確認する。
request specのような結合テストは、以下のレスポンスが返ってくるか確認するのみにする。
単体テストで網羅されていることを、request specでは書かないこと。
・200番のレスポンスが返ってくるか
・業務エラーの場合、正しいエラーレスポンスがかえるか
・システムエラーの場合、Exceptionのメッセージが処理できているのか
※ただ事情があって単体テストにテストケースを追加できない場合や、今単体テストの網羅性を上げるとリリースが間に合わず、いったんは結合テストで動作確認せざるを得ない場合や、短期的な時間軸ではROIが高い場合（既存コードを捨てる可能性があるような意思決定をする場合）に関しては、例外的に結合テストに単体テストの動作保証を行うことを許容する。
ただしその場合は、責務が異なることをコードにFIXMEコメントを追加し、イシューとして切り出すこと
全体方針が決まれば、後は自動で意思決定が決まるような議論に関しては、個別のPRで宗教論争のようにレスバするのではなく、全体方針のコンセンサスを決めるようにしましょう。
弊社では、全体方針の問題はGitHubのディスカッション機能を利用して、方針を決めるべきテーマを個別に起票しています。例えば単体テストとRequest Specをどこまでテストすべきかのようなテーマだったりとか、コードのコメントは英語にするのか日本語にするのかだったり、インデントは3文字なのかタブなのか等のようなルールになっています。
また積極的にLinterを利用して、レビュワーが判断しなくても良い問題については、自動で判断してもらうようにしています。弊社はrubocop / slimcop / rubocop-rspec などをCIに載せてチェックしています。
考えてみるとわかるのですが、エンジニアの時間単価が仮に〜5000円になるとします。
2人のエンジニアが全体方針の問題について1時間コードレビューにかけるとするとこの議論にかかるお金は
- ¥5,000 * 1時間 * 2人 = ¥10,000
かかります。そしてこのような議論が年に20回行われると、この議論に年間¥200,000かかることに。
方針さえ決まれば、この議論をする代わりに、チームで美味しい寿司を何回か食いに行けたはずです。
3往復以上するレビューコメントに関しては、ビデオ会議して話そう
レビューを行うと、何度もレビューコメントが往復しているケースがあります。
正直にいって時間の無駄なので、3往復しても伝わらない場合は直接話して前提をすり合わせましょう。
このレビューに対して、ほとんどの場合、
- 前提が異なるため、確認に時間がかかる
- ソースコードであえてやらなかったことが書かれておらず、レビュワーが無駄に指摘せざるを得ない状況になっている
みたいな理由で時間がかかります。
開発体験もあまりよくないし、すり合わせに対する心理的負荷も高いし、そもそも生産性が非常に低くなるので、3回以上レビューで往復する場合、ささっと口頭コミュニケーションで前提をすり合わせてから、後でイシューに議論内容をまとめましょう。
Discussion