コードレビューをパタン化した
いくつかのプロジェクトを掛け持ちでコードレビューをしていると
レビューにムラがでるので自分なりにパタン化しました
レビュー観点に従ってレビューする
レビュー観点
- 機能の目的が正しいこと
- 実装が機能を満たすこと
- デグレがないこと
- 今すぐリリースしても良いこと
- 未来にバグる可能性がないこと
機能の目的が正しいこと
勝手な機能を盛り込まないための確認です
この機能がちゃんとした案件起因なら、それがわかるようにサービス仕様書とかのリンクを貼ってもらいます。改善ネタなら議論してるissueのリンクを。急に思いついたネタならPRの中で説明してもらいます
実装が機能を満たすこと
先ほどの「機能の目的」を実装が満たすかを確認します
基本的には実装をサラっと見てテストコードを見て、網羅性を見ます
可読性とかオブジェクト指向としてどうかはここでは見ません
あくまで機能が正しく動くことだけ見ます
デグレがないこと
自動テストが通ってる前提でデグレを確認します
enumへの項目追加した場合、それを使ってる箇所がただしく動作するかは要注意
他にもいろいろチェックポイントがありそうですがまだ体系化できていません
デグレチェックは悪魔の証明みたいものなので別途いろんなケースを経験しなきゃですね...
今すぐリリースしても良いこと
サービスイン前に出てはいけない機能の場合はコードが良くてもマージはできません
ただこのPRをそのまま放置すると、いざマージするタイミングでコンフリクトをおこします。時が進むので...
それを避けるために、PRの内容でサービスインに影響しない部分だけを抜き出してPRしてもらいます
そんな感じに工夫するとPRの量が減って読みやすいし、加えて、サービスイン当日のリリースを最小限に出来る効果もあります
(サービスイン当日のリスクを最小限にする技は別途まとめたいなぁ)
未来にバグる可能性がないこと
可読性や責務などを見ます
急いでいるときは「別途修正版をPRしてね!」で済ませます。バグではないので
レビューコメント
コードレビューは指摘事項を書くことがメインですが、それだとレビューされた側は、指示に従ってただ直すだけで、指摘の意図が読み取れないことがあります
なので「何がOKで何がNGなのか」を明確にする意味で、先ほどのレビュー観点に従った判定結果を書きます
例えばこんな感じ
観点 | 判定 | 理由 |
---|---|---|
機能の目的 | OK | 仕様書リンクがある |
実装の機能 | OK | テストを網羅している |
デグレなし | OK | 既存コードのテストがあり、テストを通ってる |
今すぐリリース | OK | 改善ネタだから影響なし |
未来のバグ | NG | getHoge()がnull返すとか無いわー |
1ヶ月くらいやってみた
いいところ
- レビューがパターン化されてやりやすい
- 「今どんな観点でコードを見ているか」を認識して読むと読みやすい
- いつのまにか「重箱の隅レビュー」になるのを防げる
- レビューの記録が残るので、指摘事項修正後、チェックすべき項目が明確(同じことをチェックするのを防げる)
- 判定理由を書いてるときに「本当にOKか?」て疑問が生まれる
わるいところ
- いまのところないけど、思考停止にならなければいいなー
まとめ
レビュー観点を決めてレビューすることで、レビューのムラが減りました
判定結果を相手に明示することでレビュー結果を明確にすることができました