最近業務でレビューをする機会が増え、
自分のレビューの質や早さについて考えることが多くあります
そこで先輩に相談したところあるサイトを教えていただきました
今回はそのサイトを読みながら、コートレビューについて考えていきたいです
参考
Googleのコードレビューガイドです
レビュワーのポイント
観点
レビューの観点で大事だと思うのは以下の4つ
- 設計
ガイドによると最も確認すべきことだという
コードの各部分が相互に連携しているかを見る - 機能性
ここは仕様通りに動くかと解釈した
またエッジケースを想定するのもここ - 複雑さ
サッと読んだ時に引っ掛かりがないかを見る
ここで「ん?」となる場合はゆくゆくの改修で時間がかかってしまう
この場合の解決策としてコード修正でなく、コメントを残すことも有効である
またオーバーエンジニアリング(まだ必要のない機能を盛り込む)も警戒すべき - 命名、コメント
サイト読んでて意外にここはスルーしているなと思ってしまった
今度から命名、コメントは目を通そう
実装の背後にある理由は残せているかを特に見ていきたい
特にコメントはAI駆動で有効になってくる
レビューの手順
個人的に一番知りたかったことです
1. 変更を広く眺める
プルリクが何をしたいのかをまず見極めます
不具合の原因は何か、どんな方針で修正するかを読んで理解します
動作確認やテストも理解の参考になります
そしてその変更が行ってはいけないものか確認しましょう
2. 変更の主要部分を見る
1の後にようやくコードを見ます
まずはメインのファイルを見つけること（1つとは限らない）
ロジックの変更が大きなファイルはどれか考えよう
プルリクが大きく、どれが主要部分かわからない時は
「開発者にどこを最初に見るべきか聞く」「プルリクを分割する」のはなるほどと思った
ここで設計上の問題があればコメント返すこと（以降のレビューが無駄となる可能性）
3. 残りを適切な順序で見る
適切な順序は「レビューアーが意図、ロジックを効率的に理解できる順序」と解釈した
例えばデータの流れに沿って読む、プログラムの実行順序で読むなど
APIの追加はインターフェース定義（proto/IDL） → コントローラー/ハンドラー → ビジネスロジック → データベース関連で見てくとわかりやすいようだ
あとはツールが表示してくれる順序（アルファベット順?）で見ると漏れがない
レビューのスピード
サイト曰く、チーム全体の開発スピードに実装のスピードより
レビューのスピードの方が影響を与えるようです
またレビュいーの不満の大抵はレビューのスピードが改善で解決するらしい
ではどれほど急ぐべきか？
目安は最長「１営業日」、そのため基本レビュー依頼が来たら取り組むべき
例外はコーディングなど集中して取り組む作業となる（フローへ戻るのに時間かかる）
自身のレビュースピードは、ガイドラインに準拠してレビューを厳密にしていけば、
だんだんと各プロセスが早くなっていくので焦らなくても良いとのこと
レビューコメント
- 丁重に
必ずコードに対して行い、開発者本人へコメントしないこと
自身が攻撃されていると思ってしまうのを避けます - 指摘理由を含める
基本的な指摘でない時は、その提案がどうコード改善につながるか、
参考にしたベストプラクティスを加えると良い - 指示を与える
これは最近私も気づいたのだが、問題を指摘するだけでなく、
どう実装するかを直接指示することが有益となる場面もある
例えば自分の指摘の意図が伝わりづらい時など
レビューで使えるプレフィックス
IMO: 修正を強く推奨(in my opinion)
IMHO: IMOより少し軽い(in my humble opinion)
NITS: ささいな指摘
これらは指摘の心理的負荷を下げることができる
レビュイーのポイント
コミットコメント
ここはあまり意識されておらず、大事なポイントに感じた
1行目
- 何をしたのかを短く要約する
- 完全な分で、命令形で書く
- 次の行は空行に
主要部に有益な情報を盛り込む
2行目以降は有益な情報を書く
- 解決された問題の短い説明
- なぜこれか最良かの説明
- 関連するバグチケット
プルリクを小さくする
よく言われますが小さいプルリクは以下のメリットがあります
プルリクの小ささと作成の手間が天秤ですね
- 早く、隅々までレビューできる
- バグの混入リスクが減る
- 差し戻し時の無駄な作業
- マージ、ロールバックがしやすい
"小さい"とは
簡単な目安は100行が適切なサイズで、1000行となると大きい
ただ200行でも50ファイルに跨ると"大きい"と判断される
例外に大きくても許される場合があり
- １ファイルの削除は1行の変更とみなせる
- リファクタリングツール(変数名の一括変換など)や自動生成
※コミットは分離する方が良い