eng-docs

コードレビュー(書籍第9章)

10 min read

コードレビュー(書籍第9章)

『Google のソフトウェアエンジニアリング』第9章を要約したもの。本文の引用はしていない。 関連論文 “Modern Code Review: A Case Study at Google” (ICSE-SEIP 2018) の PDF が 同ディレクトリの modern-code-review-at-google.pdf にある。

実際にレビューで何を見るかの手順は コードレビューの進め方 にある。

承認は3種類ある (書籍9.2)

Google では1つの変更に対して、性質の違う3つの承認が求められる。 これを分けておくと、筆頭レビュアーはコードの正しさと妥当性に集中でき、 オーナーは各行の詳細を追わずに「自分の担当領域に入れて適切か」だけを見られる。

承認誰が何を見るか
LGTM別のエンジニア1名正しさと、意味が把握できるか
オーナー承認そのディレクトリのオーナーコードベースの自分の担当部分への追加として適切か
リーダビリティ承認その言語のリーダビリティ保持者その言語のスタイルとベストプラクティスに従っているか

LGTM とリーダビリティ承認が分かれているのは保守性のため。

オーナーが見る問いは3つ。

このコードは保守が簡単か、難しいか
このコードは自分の技術的負債を増やすか
このコードの保守に必要な専門知識はチーム内にあるか

OWNERS ファイル

ディレクトリとその子ディレクトリのオーナーを列挙したファイルを置く。 他の OWNERS ファイルや外部のアクセス制御リストを参照してもよい。運用上の注意は2つ。

  • 登録者のリストは小さく絞る
  • オーナーシップを通過儀礼として使わない

新プロジェクト発足時にオーナー権限の登録を差配する中央集権的な監督者は置かない。

レビューによる恩恵

書籍9章が挙げている6つ。(書籍9.3)

コードの正しさをチェックする
その変更を他のエンジニアが理解できる状態にする
コードベース全体での一貫性を強制する
チームのオーナーシップを心理的に促進する
知識共有を可能にする
コードレビュー自体の履歴の記録を提供する

重みづけは「正しさ」ではない。時間が経ってコードベースがスケールしたときにも、 その変更が理解可能で意味を成すと保証できることの方が意義が大きい、というのが書籍の立場。

レビューは正しさの万能薬でも唯一の検査でもなく、多重防御の一要素。 だから成果を上げるのに「完璧」である必要はない。

心理的・文化的な効果 (書籍9.3.4)

  • コードは個人のものではなく集合的な事業の一部という認識が強化される
  • レビューがなければ、大半のエンジニアは自己流のスタイルと設計に自然と引き寄せられる。 レビューは他者の言い分を聞くことと、大義のために妥協することを強制する
  • あらかじめ規定された中立的な方法で異議を唱えるので、 プロセスがなければ熱くなっていたはずのやり取りを和らげる。 職務として批判的になったレビュアーが非難されるいわれはなくなる
  • レビューがなければ手を抜くのが自然。レビューは送る前に直すことを強制する

知識共有 (書籍9.3.5)

情報は双方向に流れる。作者もレビュアーも新しいテクニックとパターンを学ぶ。

  • レビュアーは作者にドメイン知識を渡せる。行動を求めない参考情報には FYI と印を付ける
  • なぜその方法で書いたのかを問うこと自体が知識共有になる
  • 大規模リファクタリングを通じて、新しいパターンがレビューを背景に告知されることも多い
  • 元の作者とレビュアーにとどまらず、はるかに多くのエンジニアに見識が広がる

意味の把握については「顧客は常に正しい」 (書籍9.3.2)

設計上の決定では作者を尊重する、という原則とは別に、 コードの意味が分からないという質問は、一つ残らず妥当なものとして扱う。

これは「批評に応じてアプローチやロジックを変えろ」という意味ではない。 自分のコードをもっと明確に説明する必要があるかもしれない、という意味。

作者と違う観点を持つレビュアーを見つけると有益なことが多い。 特に、そのレビュアーがその変更を業務の一環で保守したり使ったりする可能性がある場合。

変更の種類ごとの見どころ (書籍9.5)

まったく新しいコード

設計レビューの比重が大きくなる
APIが合意された設計に一致しているか
デザインドキュメントがレビューされているか
全APIエンドポイントがユニットテストを伴って完全にテストされているか
前提条件が変わったときにそれらのテストが失敗するか
適切なオーナーがいるか(最初のレビュー対象がその新ディレクトリ用のOWNERSファイルだけ、ということも多い)
十分にコメントが付いていて、必要なら補足のドキュメントがあるか

挙動の変更・改善・最適化

変更の大半はこれ。最初に問うのは入れるかどうか。

この変更は必要か
この変更はコードベースを改善するか
※最善の修正がコードの削除であることも実際にある
※実行されないコード、旧式になったコードの除去
新しい挙動すべてに対応するテストの修正が含まれているか
その修正が既存テストの前提条件を壊していないか
最適化ならパフォーマンスベンチマークの結果が添えられているか

バグ修正とロールバック

バグ修正以外の問題にも手を出す誘惑を避ける。 規模が膨らむだけでなく、リグレッションテストの実行と、他者によるロールバックが難しくなる。

  • そのエラーをそもそも捕捉できるように関連テストを更新しておく
  • ロールバックは、依存が壊れた下流の顧客が行うことが多い。それでもコードレビューは必要
  • 新しいコードに他の開発者がすぐ依存し始めるため、 ロールバックがその人たちのコードを壊すという問題が起きる。変更が小さいとこの懸念が軽減される

リファクタリングと大規模変更(LSC)

自動生成された変更でもレビューは要る。ただしレビューの範囲を限定する。

  • 見るのは自分たちのコードに特有の懸念点だけ。 土台のツールや LSC そのものにフラグを立てない
  • 変更を生成するプロセス全体は既にレビュー済みで、個別のチームに拒否権はない

関連

0件のページ