はじめに
こんにちは、tokuです。
SEになって3年ほど経ってからコードレビューをする機会が増えてきましたが、実装以上に難しく感じることも多く、今だに慣れない部分があります。
そこで、Googleのコードレビューガイドライン「How to do a code review」を自分なりに整理し、今一度自分の行動を反省をしてみようと思います。
用語の補足
CL（Change List）：一連の変更を表し、PRの差分に相当するもの
1. コードレビューの前提
要点
- コードレビューの目的はソースコードの健全性担保
- 健全性担保が厳格になるほどタスクが進捗しなくなる＝トレードオフの関係
- 完璧なコードよりもより優れたコードを
- コードの健全性が確実に改善される状態になったらCLを承認しよう
- 今全てを改善するのではなく、継続な改善を求めよう
- 健全性担保以上を求める場合はコメントでそれを明示しよう
- 教育目的のアドバイスなど
- 好みより事実を、好みより原理原則を、好みより一貫性を
- 喧嘩はせども放棄はするな
自戒
好みより事実を、好みより原理原則を、好みより一貫性を
個人的に特に注意を払っているのは、自分の思想の強さに自覚的になることです。
エンジニア界隈では思想や宗教といった言葉が良く出てきますが、健全性の担保以上にそれを振りかざしてプロジェクトの進捗に悪影響を及ぼしていたらただの自己満足です(自戒)。
中途半端に歴が長くなってくると中途半端に思想が強くなりがち、というのは様々な分野で言えることだと思います。ダニング=クルーガー効果というやつですね。
私は綺麗なコードが好きで、油断するとこう書いた方が美しいという判断基準に寄って行ってしまうので、「好みの話を不必要にしていないか」「好みの話がこれは当然だという語り口になっていないか」は常に意識するようにしています。
2. コードレビューでの着眼点
要点
- 設計は適切か
- コード全体を俯瞰して一貫性が取れているか
- 変更するタイミングは適切か
- 意図通りに動作するか
- ユーザー目線で考え動作確認を実施もしくは依頼しよう
- 並列作業中の変更と競合しないか
- 読み手がすぐに理解できるほど簡潔か
- テストは有効か
- ネーミングの単語や長さは適切か
- コメントには「なぜ」が書かれているか
- 複雑なアルゴリズムは何をしているかの説明もOK
- スタイルは規約に沿っているか
- 既存のコードとの一貫性は取れているか
- ドキュメントは更新されているか
- 全ての行をチェックしたか
- コード全体を俯瞰して見て、健全性を損ねていないか
自戒
読み手がすぐに理解できるほど簡潔か
コードが読みづらく感じると自分の理解力の低さを責めてしまいがちですが、そういったコードはおそらく他の人も大して理解できていません。また可読性が低いと「全ての行をチェック」することへのハードルも上がります。
レビューの滞留を防ぐためにも、レビュイーが先輩であろうが恐れずリファクタを提案しましょう。異議を唱えるのも仕事ですよ、仕事してください(自戒)。謙虚な姿勢を忘れなければ先輩もきっと理解してくれるはず！
3.コードレビューの流れ
要点
- CLを全体像を確認し、方向性がずれていないかチェック
- 主要な変更を探す → 分かりづらければCLの分割を依頼
- 主要な変更を確認
- 全てのファイルを漏れなく確認
自戒
CLを全体像を確認し、方向性がずれていないかチェック
「ざっと見る」は間違いなく最優先タスクです。
皆さんもレビュイーとして大きな手戻りを食らったことはあるはず。そういった指摘は早めに貰えた方が嬉しいのも理解しているはず。レビュワーになって自分のタスクの多さを言い訳にして後回しにしないでください(自戒)。
最終締め切りに間に合わないといった事態を避けるためにも、早い段階での確認はとても重要です。
4. コードレビューのスピード
要点
- コードレビューが遅れると…
- プロジェクト全体の遅延に繋がる
- レビュイーのフラストレーションの原因になる
- レビュイーが時間を優先するようになる結果、コードの健全性が下がる
- なるべく1営業日以内にレビューしよう
- 軽微な修正はレビュイーを信頼して再レビュー不要としよう
- CLが大きすぎる場合は分割を依頼しよう
自戒
コードレビューが遅れると…プロジェクト全体の遅延に繋がる
仕事ができる人はレスポンスが早いとよく言いますが、コードレビューも当然同様です。
レビュワーは何人かいることも多いので誰もレビューしていないと自分も後回しにしがちですが、逆ですよね。LINEのグループチャットじゃないんですから。
誰もやっていないからこそ自分がいち早く確認しましょう。自分がレビューを止めることでプロジェクトが遅延するという感覚をしっかり持ってください(自戒)。
5. レビューコメントの書き方
要点
- 礼儀正しい口調で
- 意図やベストプラクティスを挙げつつ何故を明確に
- レビュイーに考えさせるため、具体的なコードは提示しない
- 重要度を明示しよう
- コードに対する説明はソースコードに残そう
- リファクタで解決するならコメントで残すのではなくリファクタしよう
- コードレビューツール上の情報は将来の読み手に伝わらない
自戒
レビュイーに考えさせるため、具体的なコードは提示しない
以前教えるのが上手い友人に教え方を聞いたところ、「何を教えるかより何を教えないか考えろ」と言われました。
コードレビューでも親切心で具体的なコードを提案してしまいがちですが、親切になっていないこともあるみたいです。教えないことの有益さを理解してください(自戒)。
6.コードレビューの衝突対応
要点
- 通常レビュイーの方がコードに詳しいということを踏まえ、冷静に判断しよう
- 自分が正しいと考えるなら
- 健全性を妥協してはいけない。変更を主張し続けよう
- 礼儀忘れるべからず。レビュイーに理解を示した上で同意できないことを伝えよう
- すぐ修正してもらおう
- 緊急の場合は例外だが、将来的に必ず修正されることが保障される状態にしよう
- 緊急とする条件：重大なインシデントが差し迫った状態かつ変更が小さい
自戒
健全性を妥協してはいけない。変更を主張し続けよう
意思を持って仕事をしているなら考え方の違いで衝突するのは当たり前。何も考えずにただ衝突を怖がり回避するのは怠慢です(自戒)。
重要なのはそれが建設的かどうか。ヤンキー漫画で不良たちが喧嘩を通して互いの理解を深めるように、エンジニアも相手を理解する努力を忘れず、良い喧嘩(レビュー)をしましょう。
まとめ
コードレビューは人間の目でコードを確認する最後の防波堤という重要な位置づけでありながら、プロジェクトのスピードという観点でも人間関係という観点でもネックになりやすい難所だと思います。
プロジェクトメンバー全員で今何を優先すべきかを擦り合わせつつ、レビュイーとしてもレビュワーとしても対等に意見を言える、また言ってもらえるよう、これからも努力していきたいです。