🪄
Google コードレビュー・ガイドライン要約【コードレビューの観点編】
はじめに
本記事は「Google コードレビュー・ガイドライン要約」シリーズの【コードレビューの観点編】です。
Google Engineering Practices Documentation (日本語訳) の要約になります。
未読の方は、はじめに 本記事のポジションと用語 をお読みください。
シリーズ構成
※リンクが貼られていない箇所は未要約です
- コードレビューの仕方
- コードレビューの基準
- コードレビューの観点 ← here!!
- コードレビューの進め方
- コードレビューのスピード
- コメントの書き方
- 取り下げへの対応
- CL 作成者のガイド
- 適切なディスクリプション
- 小さな CL
- レビューコメントへの対応
- 緊急事態の CL
コードレビューの観点
設計
- CL の全体的な設計
- レビューで確認すべき最も大切なこと
- 問いかけリスト
- CL のコードの各部分は相互にきちんと連携するでしょうか？
- この変更が属する場所はどこでしょうか？コードベースか、それともライブラリ？
- システムの他の部分とうまく統合するでしょうか？
- この機能を追加するタイミングは今がふさわしいでしょうか？
機能性
- コードの動作
- この CL の動作はコードのユーザーにとって適切か
- エンドユーザー
- 開発者（将来このコードを「使う」必要のある人）
- この CL の動作はコードのユーザーにとって適切か
- 新しい視点の提供
- エッジケースを想定する
- 並行処理の問題を探す
- ユーザーになりきって考える
- 動作の検証
- ユーザーに影響する変更
- 例えば、UI の変更など
- レビュアーが CL の動作を確認するべき最も重要な機会
- 純粋な実行では不具合の発見が難しい変更
- 例えば、並行プログラミングなど（デッドロックや競合状態）
- コード全体を見て、問題がないか注意深く確認する人が必要（開発者とレビュアーの両方）
- ユーザーに影響する変更
複雑性
- シンプル
- 複雑性を排除する
- CL のあらゆるレベルで確認する
- 問いかけリスト
- CL が必要以上に複雑になっていないでしょうか？
- 一行一行は複雑すぎないでしょうか？
- 関数は複雑すぎないでしょうか？
- クラスは複雑すぎないでしょうか？
- 「複雑すぎる」ことの例
- コードを読んですぐに理解できない
- 開発者がこのコードを呼び出したり修正するときに、不具合を生み出す可能性がある
- 複雑性を排除する
- オーバーエンジニアリング
- 課題に対する過剰な解決策のこと
- 必要以上なコードの一般化
- 現在のシステムにとってまだ必要のない機能
-
現在、解決する必要のある既知の問題に取り組む
- 将来、解決する必要が出てくるかもしれない、推測に基づいた問題は解決しない
- レビュアーが特に警戒すべき部分である
- 課題に対する過剰な解決策のこと
- 複雑さの排除
- 新たな変更があったとき、小さな複雑性でも混入させない
- ほとんどのシステムは小さな変更が積み重なり、だんだんと複雑化する
テスト
- 適切な選択
- 変更に適したユニットテスト、結合テスト、E2E テストを依頼する
- 妥当性
- CL のテストが正確で、適切で、有用であることを人間が確認する
- テストがテストコード自体をテストすることはない
- 問いかけリスト
- コードが壊れているときにテストはきちんと失敗するでしょうか？
- そのテストの下でコードを変更すると、テストが誤検知を起こさないでしょうか？
- 各テストはシンプルで有用なアサーションを使っているでしょうか？
- テストは異なるテストメソッドごとに適切に分割されているでしょうか？
- テストも保守対象
- テストもまた保守すべきコードである
- メインのバイナリに含まれないからといって、テストが複雑になるのを許容しない
命名
- 適切な名前
- 開発者はあらゆるものに適切な名前を与えているか
- それが何であるか、何をするかを伝えるのに十分に長く
- 読むのに困難を覚えないほど短い
- 開発者はあらゆるものに適切な名前を与えているか
コメント
- 問いかけリスト
- 開発者はわかりやすい言葉で明確なコメントを書きましたか？
- すべてのコメントは実際に必要でしょうか？
- コメント
- コードが「なぜ」存在するのかを説明する
- コードが「何」をしているのかを説明すべきではない
- 「何」を説明する必要があるなら、コードをもっとシンプルにすべきである
- しかし、例外はある
- 正規表現や複雑なアルゴリズムを説明するコメントなど
- ドキュメンテーションコメントとは異なる
- ドキュメンテーションコメント
- クラス、モジュール、関数を説明するコメント
- コードの目的、使い方、使われたときのふるまいを記述する
スタイル
- スタイルガイド
- CL が適切なスタイルガイドを従っているかを確認する
- 例えば、Google のスタイルガイドは こちら
- スタイルガイドを満たしていればOK
- エビュアーがスタイルガイドに記載のないスタイルの改善をしたいとき
- コメントに「Nit:」プレフィックスを付ける
- コードの修正が強制でないことを伝える
- 個人的なスタイルの好みで CL の提出をブロックしない
- エビュアーがスタイルガイドに記載のないスタイルの改善をしたいとき
- スタイルの変更は分ける
- CL の作成者は、スタイル上の大きな変更を、他の機能的な変更に混ぜないこと
- CL で何が変更されているのかを見るのが難しくなる
- マージ後のロールバックが大変になる
- たとえば、ファイル全体を再フォーマットしたいとき
- 再フォーマットだけを一つの CL として提出する
- CL の作成者は、スタイル上の大きな変更を、他の機能的な変更に混ぜないこと
[OSS]ドキュメンテーション
- 関連ドキュメントの更新
- CL がコードのビルド、テスト、相互連携、リリースのやり方を変更するとき
- それに関連するドキュメンテーションも更新しているか確認する
- コードの削除・非推奨
- CL がコードを削除あるいは非推奨にしたとき
- ドキュメンテーションも削除するか検討する
- ドキュメンテーションが存在しなければ、作成するように依頼する
すべての行を見る
- コードの理解
- レビュアーのスタンス
- 全コードが何をしているかを確実に理解する
- コードの中に書かれているものが正しいと決めてかからない
- 詳細に見る対象
- 人間の書いたクラス、関数、コードブロックなど
- ざっと見る対象
- データファイル、自動生成されたコード、巨大なデータ構造など
- レビュアーのスタンス
- [Google] 読みにくいコード
- コードが読みにくく、それがレビューを遅らせているとき
- レビューをいったん置いて開発者に知らせ、コードを明確にしてくれるのを待つ
- Google には優秀なソフトウェアエンジニアが雇われており、あなたはその一人
- あなたがコードを理解できないのなら、他の開発者もきっと理解できない
- レビューをいったん置いて開発者に知らせ、コードを明確にしてくれるのを待つ
- コードが読みにくく、それがレビューを遅らせているとき
- [Google] 自分には適していないと感じたとき
- その CL について他に適切なレビュアーがいることを忘れないこと
- 特に、セキュリティ、並行処理、アクセシビリティ、インターナショナライゼーションといった複雑な問題に関しては適任者がいる
コンテキスト
- コードの俯瞰
- CL を広いコンテキストの中に置いて眺めると有意義なことがよくある
- 変更がうまく機能することを確認するために、ファイル全体を見なければならないときもある
- システムの俯瞰
- CL をシステム全体のコンテキストの中に置いて考えてみることも有益である
良いこと
- 励ましや感謝
- CL の中に素晴らしいものを見つけたら、開発者に伝える
- 特に、あなたのレビューコメントの一つに取り組んでやり遂げたそうする
- メンタリング
- 開発者が正しいこと行ったときにそれを伝えるほうが、間違いを指摘するよりもずっと価値があることもある
まとめ
コードレビューをするときに確認すること。
- コードがうまく設計されていること
- 機能性がコードのユーザーにとって適切であること
- UI の変更がある場合、よく考えられていて見た目も適切であること
- 並行処理がある場合、安全に行われていること
- コードが必要以上に複雑でないこと
- 開発者は将来必要になるかもしれないものではなく、現在必要だとわかっているものを実装していること
- コードには適切なユニットテストがあること
- テストがうまく設計されていること
- 開発者はあらゆるものに明確な名前を使っていること
- コメントは明確で有意義なもので、「何」ではなく「なぜ」を説明していること
- コードは適切にドキュメント化されていること
- コードはスタイルガイドに準拠していること
レビューを依頼されたら留意すること。
- コードのすべての行をレビューすること
- コンテキストを確認すること
- コードの健全性を改善しているかを見極めること
- 開発者が良いことをしたらそれを褒めること
おわりに
ソフトウェア設計から心構えまで、かなり広範囲に渡ってまとめられています。
以下、印象的だった部分をまとめます
- CL の全体的な設計
- レビューで確認すべき最も大切なこと
- 原著にはもう少し具体的な例を書いてほしい...!
- 複雑性を排除する
-
CL のあらゆるレベルで確認こと
- コードを読んですぐに理解できるか
- 新たな変更があったとき、小さな複雑性でも混入させない
- ほとんどのシステムは小さな変更が積み重なり、だんだんと複雑化する
-
CL のあらゆるレベルで確認こと
- テストも保守対象のコード
- メインのバイナリに含まれないからといって、テストが複雑になるのを許容しない
- コメント
- 処理のコメント
- コードが「なぜ」存在するのかを説明する
- シンボルのコメント (クラス、モジュール、関数)
- コードの目的、使い方、使われたときのふるまいを記述する
- 処理のコメント
- コードの理解
- 全コードが何をしているかを確実に理解する
- コンテキスト
- コードを俯瞰し、ときには差分以外のファイル全体を見て理解する
- システムを俯瞰し、他のシステムとうまく結合できるか考える
- 新しい視点の提供
- エッジケースを想定する
- 並行処理など、単純なコードの実行では発見しづらい問題を探す
- ユーザーになりきって考える
- 励ましや感謝
- CL の中に素晴らしいものを見つけたら、開発者に伝える
感謝を伝えることもいつも忘れず、レビューしたいなと感じました。
Discussion