コードレビュー
このディレクトリ
- チェックリスト — レビュー時にひととおり見る短縮版
自分のノートに『Google のソフトウェアエンジニアリング』第9章の内容を織り込んである。 書籍由来の箇所は (書籍9章) と注記した。本文の引用はしていない。 関連論文 “Modern Code Review: A Case Study at Google” (ICSE-SEIP 2018) の PDF が 同ディレクトリの modern-code-review-at-google.pdf にある。
前提
エンジニア間の信頼と尊敬
エンジニア間の信頼と尊敬からくる暗黙の了解
フィードバックと批判がプロフェッショナルな領域にとどまっていて建設的かどうか
外部からの変更でも、改善であれば受け入れ、敬意を払う
レビュアーの人数
基本は1人
※筆頭レビュアー
※場合によって該当箇所の専門家が加わる
※関係するメンバーは任意で加わるが、コメントに対応するのは任意
承認は3種類ある (書籍9.2)
Google では1つの変更に対して、性質の違う3つの承認が求められる。 これを分けておくと、筆頭レビュアーはコードの正しさと妥当性に集中でき、 オーナーは各行の詳細を追わずに「自分の担当領域に入れて適切か」だけを見られる。
| 承認 | 誰が | 何を見るか |
|---|---|---|
| LGTM | 別のエンジニア1名 | 正しさと、意味が把握できるか |
| オーナー承認 | そのディレクトリのオーナー | コードベースの自分の担当部分への追加として適切か |
| リーダビリティ承認 | その言語のリーダビリティ保持者 | その言語のスタイルとベストプラクティスに従っているか |
LGTM とリーダビリティ承認が分かれているのは保守性のため。
オーナーが見る問いは3つ。
このコードは保守が簡単か、難しいか
このコードは自分の技術的負債を増やすか
このコードの保守に必要な専門知識はチーム内にあるか
OWNERS ファイル
ディレクトリとその子ディレクトリのオーナーを列挙したファイルを置く。 他の OWNERS ファイルや外部のアクセス制御リストを参照してもよい。運用上の注意は2つ。
- 登録者のリストは小さく絞る
- オーナーシップを通過儀礼として使わない
新プロジェクト発足時にオーナー権限の登録を差配する中央集権的な監督者は置かない。
第三者のレビューをする意味
1人でやるよりもよりよくなる
1人だと気づけない・気付くのが遅くなるものが早く気付ける
※後でこうしておけば良かったと思うことが減る
※リリース後に早く修正しておけばよいと思う内容はないか
※ビジネスサイドまたはユーザーからのツッコミがあり得るもの
※処理の全体像を見て正しくない使い方をしていないか
※非効率な処理でアクセス数が増えた場合に問題になるコード
※既存と一貫性のない書き方をしていないか
レビューによる恩恵
書籍9章が挙げている6つ。(書籍9.3)
コードの正しさをチェックする
その変更を他のエンジニアが理解できる状態にする
コードベース全体での一貫性を強制する
チームのオーナーシップを心理的に促進する
知識共有を可能にする
コードレビュー自体の履歴の記録を提供する
重みづけは「正しさ」ではない。時間が経ってコードベースがスケールしたときにも、 その変更が理解可能で意味を成すと保証できることの方が意義が大きい、というのが書籍の立場。
レビューは正しさの万能薬でも唯一の検査でもなく、多重防御の一要素。 だから成果を上げるのに「完璧」である必要はない。
心理的・文化的な効果 (書籍9.3.4)
- コードは個人のものではなく集合的な事業の一部という認識が強化される
- レビューがなければ、大半のエンジニアは自己流のスタイルと設計に自然と引き寄せられる。 レビューは他者の言い分を聞くことと、大義のために妥協することを強制する
- あらかじめ規定された中立的な方法で異議を唱えるので、 プロセスがなければ熱くなっていたはずのやり取りを和らげる。 職務として批判的になったレビュアーが非難されるいわれはなくなる
- レビューがなければ手を抜くのが自然。レビューは送る前に直すことを強制する
知識共有 (書籍9.3.5)
情報は双方向に流れる。作者もレビュアーも新しいテクニックとパターンを学ぶ。
- レビュアーは作者にドメイン知識を渡せる。行動を求めない参考情報には FYI と印を付ける
- なぜその方法で書いたのかを問うこと自体が知識共有になる
- 大規模リファクタリングを通じて、新しいパターンがレビューを背景に告知されることも多い
- 元の作者とレビュアーにとどまらず、はるかに多くのエンジニアに見識が広がる
目的
バグの混入を防ぐ
※コードベースに入る前に検知する
改善・悪化を防ぐ
技術的負債を減らす
※コードレビューをしたことでない場合と比べてどれだけ改善があったか
※一貫性の維持
※保守性
※可読性・理解可能性
重複を防ぐ
※すでにあるものがいくつも作られることを防ぐ
スタイルのシステム内で一貫性の維持
※スタイルの選択に前例がなければ、開発者のやり方を受け入れるべき
コードの正しさ
コード変更の全般的な妥当性
適切な変更だけ取り入れる
履歴の記録
意味の把握については「顧客は常に正しい」 (書籍9.3.2)
設計上の決定では作者を尊重する、という原則とは別に、 コードの意味が分からないという質問は、一つ残らず妥当なものとして扱う。
これは「批評に応じてアプローチやロジックを変えろ」という意味ではない。 自分のコードをもっと明確に説明する必要があるかもしれない、という意味。
作者と違う観点を持つレビュアーを見つけると有益なことが多い。 特に、そのレビュアーがその変更を業務の一環で保守したり使ったりする可能性がある場合。
レビュアーがやること
レビュープロセス
重量級のコードレビュープロセスにしない
適切にスケールしないレビュープロセスにしない
※持続可能ではなくなる
最もふさわしいレビュアーかどうか
コードベースのオーナーかどうか
コードベースの領域を熟知しているか
リーダビリティーのあるコードを書けるかどうか
もっとふさわしいレビュアーがいる場合はふさわしいレビュアーに渡す
レビューのある部分で自分にはレビューする資格がないと感じる場合、その変更について他に適切なレビュアーに渡す
修正箇所の専門家かどうか
既存コードについて
設計上の以前の決定を蒸し返したり論争したりしない
レビューは過去の決定を見直す機会ではない
把握
仕様を把握する
変更のディスクリプションと変更が大まかに何をしているかを把握する
変更を広く眺める
※変更が書かれる前にもっとコミュニケーションが必要
※大きな変更をして後で破棄や大部分の書き直しを迫られるよりは、事前に「ノー」と言う
変更の主要部分を調べる
※変更の主要部分に設計上の重大な問題が見つかれば、すぐにコメントを残す
※変更の残りの部分をレビューする時間があってもコメントを送るのが先
※変更のやり直しが重大なものであるほど、できるだけ早くやり直しに着手してもらう
不要な機能追加がないかどうか
レビュアーが望んでいない機能をシステムに追加している場合
そもそもこの変更が行ってはならないもの場合は削除依頼か代わりに何をすべきかを伝える
その変更の保守に必要な専門知識をチームが持っているかどうか
変更をベストなタイミングにする
変更をベストなタイミングにする
※この機能を追加するタイミングは今がふさわしいかどうか
※今がいいのか、別の機会がいいのか
ファイル全体を再フォーマットしたいと思ったら、再フォーマットだけを1つの変更として提出しているかどうか
※再フォーマットの差分を他に混ぜない
巨大な変更は小さな変更に分割するよう依頼する
設計
コードの各部分は相互にきちんと連携するかどうか
※システムの他の部分とうまく統合するかどうか
動作・挙動
意図通りに動作するかどうか
レビューによって意図通りに動くことを保証する
この変更は機能しているか
開発者の意図はこのコードのユーザー(エンドユーザー,他の開発者)にとって適切かどうか
バグを招き入れていないかどうか
※早く見つけるほど修正に必要な時間が短くなる
※レビュー段階でバグを見つけることでデバッグ、リグレッションテストの時間が減らせる
※最も早期に発見・解決する
コードを読むだけではわからない不具合がないかを確認する
※エッジケースを想定する
※並行処理の問題を探す
※ユーザーになりきって考える
必要に応じて変更を検証する
レビュアーも動作を確認するべき
変更の変更を反映して自分で動作確認する
※変更がユーザーにどのような影響を与えるかはコードを読むだけではわかりにくいことがある
※難しければ、開発者にその機能のデモを依頼することもできます。
各ケースで挙動が正しいこと
※データ的にあり得るケース
※テストケースになければ追加してもらう
無限ループやデッドロックや競合状態を引き起こす可能性があるかどうか
※コード全体を見て問題が発生していないことを注意深く確認する(開発者とレビュアーの両方)
大きな視点でチェックする
大きな視点できちんと確認する
※少なくとも全コードが何をしているかを確実に理解するようにする
※変更を広いコンテキストの中に置いて眺める
※変更がうまく機能することを確認するためにファイル全体を見なければならないときもある
※骨の折れるやっかいな仕事になることがある
※ある変更がより広い対象にとって理解可能かどうか
前に進めるべきかどうかの必要性を、提案されている変更の重要性と比較して、バランスよく検討すべき
レビュアーが追求すべきは、完璧さではなく継続的な改善
もし重要でない指摘であれば、細かい指摘であることを明記する
開発者が新しいことを学ぶヒントになるコメントは、いつでも歓迎されます
レビュアーは開発者の選好を受け入れるべきである
※有効であると開発者が(データを示したり堅固な工学原理に基づいて説明したりして)証明できる場合
対面でミーティングした内容は変更のコメント残す
一貫性を維持しているか
その言語のベストプラクティスに従っているかどうか
コードベースと一貫性があるかどうか
システムのコードの全体的な健康状態を悪化させない限りは、現在のコードベースとの一貫性を維持するよう求める
将来のリファクタリングの際にツールによる修正が可能かどうか
複雑すぎないか
必要以上に複雑になっていないかどうか
※「複雑すぎる」とは普通、「コードを読んですぐに理解できない」という意味
※開発者がこのコードを呼び出したり修正したりしようとするときに不具合を生み出す可能性があるかどうか
コードが読みにくい場合はコードを明確にしてくれるよう促す
複雑なコードを見つけたら説明、コードをシンプルにしてもらう、コードにコメントを追加してもらう
オーバーエンジニアリングしていないか
将来解決する必要が出てくるかもしれない推測に基づいた問題には目を向けないよう勧める
※開発者が必要以上にコードを一般化していないか
※現在のシステムにとってまだ必要のない機能を盛り込んでいたりしないか
テスト
テストが複雑になっていないか
コードが壊れているときにテストはきちんと失敗するかどうか
※そのテストの下でコードを変更すると、テストが誤検知を起こさないかどうか
※挙動を変えたなら、新しい挙動すべてに対応するテストの修正を必ず含める
各テストはシンプルで有用なアサーションを使っているかどうか
テストは異なるテストメソッドごとに適切に分割されているかどうか
※privateメソッドはpublicメソッドを通してテストする
※特定のヘルパークラスは使われているクラスを通してテストする
命名
適切な名前かどうか
※何であるか／何をするかを伝えるのに十分に長く、しかし読むのに困難を覚えないほど短いもの
コメント
明確なコメントかどうか
実際に必要なコメントかどうか
コメントは決定の背後にある理由といった、コード自体が語ることのできない情報を含んでいるかどうか
※何ではなく、なぜか
コードがそれ自身を説明するほど明確でないのなら、コードをもっとシンプルにすべき
※コメントが多い場合はコードがよくない場合が多い
ドキュメンテーションコメントはコードの目的や、使い方や、使われたときのふるまいを記述する
スタイル
変更が適切なスタイルガイドを従っているかを確認
※ツールでチェック
※ツールでチェックできないものはレビューでチェック
スタイルガイドに記載のないスタイルの改善をしたい場合、コメントに細かい指摘であることを明記
※スタイルの選択に前例がなければ、開発者のやり方を受け入れるべき
※個人的なスタイルの好みで変更の提出をブロックしていないかどうか
ドキュメンテーション
関連するドキュメンテーションも更新しているかを確認
知識共有
レビュー対象領域についての知識
ドメイン知識
レビューコメント
丁重に
理由を説明する
問題の指摘に加えて明確な方向性を示すことと、開発者本人に決定を委ねることをバランス良く行う
まとめてコメントする
※少しずつフィードバックしない
※フィードバック対応後に無関係なフィードバックをし続けない
代案
アプローチが不十分かどうか
アプローチに欠陥があるかどうか
※本当に欠陥か調べる。それでも分からなければ開発者に質問をする
※欠陥があった場合開発者、レビュアー双方にとっての学びの場になる
代案は個人の意見ではないか
意味の把握を強める代案かどうか
複雑さを減らす代案かどうか
機能性を向上させる代案かどうか
効率を上げる代案かどうか
取り下げ
開発者があなたの提案に同意できないとき、最初に少し考慮していただきたいのは、開発者のほうが正しいのではないかということ
※開発者はあなたよりもコードを間近で見ているため、ある側面では開発者のほうが優れた洞察を持っていることもあります
開発者の議論は筋が通っているか
コードの健康状態という視点から理にかなっているか
※開発者が正しい場合は認め、問題を水に流す
いつでも開発者が正しいとは限らない
※レビュアーはどうして自分の提案が正しいと思うのかを踏み込んで説明する
※開発者の応答に理解を示し、それから変更が必要な理由を付け加えるのが良い説明
コードの健康状態を良くする場合はその変更によってさらに作業が発生するもののそれに見合うだけコードの品質が改善すると見込まれれば、粘り強く変更を勧めるべき
※コードの健康状態の改善は、スモールステップで行われる
開発者が現在の変更の直後に問題を片付けるのでなければ、その機会は完全に消失する
※最良の選択肢は普通、変更がコードベースに取り込まれて「完了 (done)」する前に、開発者に今片付けてもらうこと
レビューのスピード
素早く(24勤務時間以内)行うべき
※少しずつ断片的にコメントではない
※フィードバックに対処してから無関係にフィードバックを受け続けることほどコード作者にとって苛立たしいことも滅多に無い
※遅いとチーム全体の開発速度が減少する
※遅いとコードをきれいにしたり、リファクタリングしたり、既存の変更をさらに改善したりする意欲をそぎます
※遅くなる場合は変更に目を通しているのか、あとどれくらいかかりそうか連絡する
あるタスクに集中的に取り組んでいる最中でなければ、コードレビューの依頼が来たらすぐに着手する
※非常に忙しくて変更のレビュー依頼をされても十分な時間が取れない場合は広い観点で見た初期段階のコメントを残したりするか他のレビュアーを紹介
※変更の全体的な設計に関してコメントを書き送り、開発者に改善を求めることができる
※コードレビューのリクエストに返信するまでの最長の時間は1営業日
※典型的な変更は(必要なら)1日以内に複数ラウンドに渡ってレビューが行われる
変更の種類ごとの見どころ (書籍9.5)
まったく新しいコード
設計レビューの比重が大きくなる
APIが合意された設計に一致しているか
デザインドキュメントがレビューされているか
全APIエンドポイントがユニットテストを伴って完全にテストされているか
前提条件が変わったときにそれらのテストが失敗するか
適切なオーナーがいるか(最初のレビュー対象がその新ディレクトリ用のOWNERSファイルだけ、ということも多い)
十分にコメントが付いていて、必要なら補足のドキュメントがあるか
挙動の変更・改善・最適化
変更の大半はこれ。最初に問うのは入れるかどうか。
この変更は必要か
この変更はコードベースを改善するか
※最善の修正がコードの削除であることも実際にある
※実行されないコード、旧式になったコードの除去
新しい挙動すべてに対応するテストの修正が含まれているか
その修正が既存テストの前提条件を壊していないか
最適化ならパフォーマンスベンチマークの結果が添えられているか
バグ修正とロールバック
バグ修正以外の問題にも手を出す誘惑を避ける。 規模が膨らむだけでなく、リグレッションテストの実行と、他者によるロールバックが難しくなる。
- そのエラーをそもそも捕捉できるように関連テストを更新しておく
- ロールバックは、依存が壊れた下流の顧客が行うことが多い。それでもコードレビューは必要
- 新しいコードに他の開発者がすぐ依存し始めるため、 ロールバックがその人たちのコードを壊すという問題が起きる。変更が小さいとこの懸念が軽減される
リファクタリングと大規模変更(LSC)
自動生成された変更でもレビューは要る。ただしレビューの範囲を限定する。
- 見るのは自分たちのコードに特有の懸念点だけ。 土台のツールや LSC そのものにフラグを立てない
- 変更を生成するプロセス全体は既にレビュー済みで、個別のチームに拒否権はない
アンチパターン
どんな変更に対してもレビュアーがいちいち難色を示する
※コードの隅々まで磨きをかけることを要求すべきではない
※開発者は改善を行う意欲を失う
細かい点(リファクタ・変更とは関係ない既存ソースの改善)から指摘する
指摘がシステムのコードの健康状態を悪化させる
指摘によりコードがより複雑になる
開発者
姿勢
変更は自分のものではなく、チームのもの
※自分流ではなく、既存と一貫性を保つ
※理解可能で保守性があるコードを書く責務
自分のアプローチについての質問を歓迎し、説明する心構えをしておくべきだ
コメントに同意しない場合は理由を説明し、代案を出して、もう一度見てもらう
代案を出す機会が双方に与えられた状態にする
適切な変更のディスクリプションを書く
将来の開発者が変更をディスクリプションに基づいて検索しやすいようにしておく
1行目: 何を行っているのかを短く要約する
2行目: 改行
3行目以降:
どうしてこれが最良の方法なのか
欠陥があればそのことにも言及
バグ番号
ベンチマーク結果
設計ドキュメントのリンク等背景が分かるようにする
変更内でいくつか関連する変更箇所があった場合、リスト内にそれらの変更箇所を列挙する
小さな変更(プロダクトコード基準)にする
速くレビューできる
隅々までレビューできる
バグが混入する可能性が減る
変更が却下されても無駄になる作業が少ない
マージしやすい
設計を改善しやすくなる
レビューによって作業がブロックされなくなる
ロールバックしやすい
1つの問題に集中したものにする
※変更の適切なサイズは単一の自己完結的な変更
※ただ一つのことに取り組むミニマルな変更かどうか
※完全なプロジェクトの形を取るものが複数入っている大規模な変更は行わない
※ある機能の全体を一度に実装するというより、その中の一部分だけを実装する変更
※小さすぎる変更を書いて失敗するほうが大きすぎる変更を書いて失敗するよりもよほど良い
※大雑把には100行の変更は適度なサイズで、1000行になると大きすぎると言えます
※プロダクトコードが200行の変更が行われていてもそれが1つのファイルで完結していれば許容できるかもしれない
※50ファイルにもわたる変更であれば普通は「大きすぎる」と判断される
機能変更やバグ修正と、リファクタリングは別の変更にするのが普通はベスト
※クラスを移動したりクラス名を変更したりはそのクラスのバグ修正とは別の変更にするべき
大規模な変更を書く前に、事前にリファクタリングだけの変更を送ればもっとすっきりした実装をする方法が用意できるのではないか
大きな変更でも許される状況
1つのファイルをまるごと削除する場合は、1行だけの変更とみなしても構いません
大規模な変更がリファクタリングツールによって自動生成される場合
※レビュアーの仕事は正常に動作していることを確認して、意図通りに変更されていると言うだけ
ツール
要件
機能が必要最低限でシンプルで本当に必要なものだけある
差分が見やすい
追加は緑、削除は赤
行内の差分も見えるようにする
大きな変更でも重くならない
最適なレビュアーの提案
アナライザーによる解析
コメントができる
コメントの未解決・解決済みのステータス管理ができる
リバートが楽
LGTM, 承認, 未解決がないときだけコミットできる
ユーザーにコントリビュートする権限があるかどうか