Open8
コードレビューの方法
akitoshi_nモチベーション
コードレビューという仕事を効率よく片付けたい。
まとめ
- コード全体の健全性が維持もしくは向上する場合は approve する
- チェックポイントは https://zenn.dev/link/comments/e805cbc94ba88c を参照
- 健全性が悪化する場合は approve してはいけない
- 逆に、健全性が悪化しないのであれば完璧は求めすぎない
- コードレビューは今やっている作業がひと段落つき次第実施する
- 遅いレビューは健全性悪化につながる
akitoshi_nコードレビューの目的
コード全体の健全性を保つこと
レビュアの役割
- 開発者のタスクを前に進めることを阻害することなく、コードの健全性が低下しないよう保証すること
- このトレードオフのバランスをどう保つのかが肝
- 完璧でなくてもコード全体の健全性が向上するのであれば、承認する
- あらゆる細かい部分まで洗練させることは要求しない
- ただし、継続的な改善があることが前提
- もっと改善できるはずだと思う点については自由にコメントして良い
- 重要でない場合は、コメントの最初に
nitをつけて改善は作者に委ねる
- 重要でない場合は、コメントの最初に
優先順位
- 個人の好みよりも、技術的な事実やデータ
- 個人の好みよりも、スタイルガイド
- 個人の好みよりも、コード全体の一貫性
akitoshi_nコードレビューで保証すべき内容
- コードがよく設計されていること
- コードが適切な場所にあるか
- システムとの統合に問題がないか
- 追加するタイミングは今が適切か
- 必要以上に複雑になっていないこと
- 必要以上にコードを一般化してしまっていないか
- コードを素早く理解できるか
- そのコードを呼び出したり修正しようとした時にバグが入り込みやすくないか
- テストが記述されていて、かつ正しく、意味があり、役立つものになっているか
- コードが壊れた時に失敗するテストになっているか
- コードの変更があったときに誤って失敗するテストになっていないか
- テストが異なるモジュールで適切に分離されているか
- 良い名前をつけているか
- その要素が何であるか、または何をするものであるかが伝わるか
- 長すぎないか
- コメントが役立つものかどうか
- Whyを説明しているコメントになっているか
- スタイルガイドに沿っているか
- 関連するドキュメントが更新されているか
レビュワーがやらなくても良いこと
- 品質チェック
akitoshi_n読み進め方
- 全体としてCLが何を行なっているのかを理解する
- CLの「メイン」となる部分を確認する
- メイン部分に設計の問題がある場合は、残りの部分はあと周りにして、コメントを送る
- 開発者がそのCLを元に追加作業を行う前に、引き止めるため
- 開発者が締め切りを守れるよう助けるため
- メイン部分に設計の問題がある場合は、残りの部分はあと周りにして、コメントを送る
- すべてのファイルを読み、論理的な順序になっているか確認する
akitoshi_nコードレビューのスピード
- コードレビューはすぐに行う
- レビュー対象のコードが来たらすぐにコードレビューを行う
- ただし、コードを書くなど1つのタスクに集中状態にある場合は、コードレビューを後回しにして良い
- 遅いコードレビューは開発パフォーマンスとコード全体の健全性を悪化させる
- チーム全体の開発速度が低下
- 開発者がコードレビューのプロセスを嫌がる
- コードを改善するモチベーションが下がる
- 逆にレビューが早ければ、大きな変更を要求したとしても不満は少なくなる
- すぐにレビューできない場合は、いつ取り掛かれるか、もしくは他のレビュアの提案をすぐに返信する
- 開発者の作業をブロックしない、と意識することが重要
- 上記のガイドラインに従えば、コードレビューのプロセス全体が早くなり、コードが健全な状態になっていくのが目に見えて感じられるようになる
akitoshi_nコードレビューのコメントの書き方
- 礼儀正しく敬意を払った書き方をする
- 開発者ではなく、コードに対して指摘する
- コメントの意図を説明する
- 直接的な指示、提案、コードを必要に応じて提供する
- 判断軸は、より最善なコードになるかどうか
- 複雑なコードの説明を求めた場合は、コードへのコメントやリファクタリングを要求する
akitoshi_nコードレビューでの反対の扱い方
- 提案によってコードの健康状態を改善でき、追加作業を行うことが理にかなう場合、その変更を主張し続ける
- 開発者が同意せず反対する場合は、意見に耳を傾け理解しようと心がけつつ、提案の背景となった考えを詳しく説明する
- 対立が解消できない場合は対面のMTGを実施し、それでも解消できない場合はチームで議論する、チームリーダーに仲裁に入ってもらうなどを検討する