コードレビューガイド（草案）

コードレビューの目的

大目的

実装者の他に少なくとも一人、マージされたコードをメンテナンスできる人が存在するようにする。

実装内容について合意を形成する。

合意内容は要求しようと実装が一致していることの他に、既存のコードとの一貫性やスタイルに関するものなど全て含まれる。

小目的

大目的達成の必要条件として、レビューが終わったとき、以下の小目的も達成できているようにする。これはメンテナンスの負担を下げるためである。マージされるすべてのコードが以下の条件を満たしていれば、大きなバグを埋め込む可能性を長期に渡って低く保つことができると考えられる。

コードがシステム全体の信頼性を（大きくは）下げないこと

Googleのeng-practicesでは"Don’t accept CLs that degrade the code health of the system"と言い切っているが、場合によっては多少の信頼性低下もトレードオフとして受け入れてよいと思う。その境界はチームとプロジェクトに強く依存する。

テストカバレッジやコードの複雑性が信頼性の指標となる。

チェックリスト

以下のチェックリストを活用するとよい。

高優先度の指摘を一通り洗い出すまで、より低い優先度のレビューについては考えないようにする。

優先度の低い指摘はそれと分かるようにする。コメントの文頭に［nit］を付けるなど。

優先度の低い指摘が対応されていなくても、機能の重要度や時間的制約を加味して考え、場合によってはGoサインを出す

優先度の低いものは、大抵後からでも修正できる。後からの修正が難しいような問題は優先度が高いものとして考える。

優先度高（大きな損失を生む問題・後からの修正が困難な問題）

要求仕様と実際の機能が一致しているかどうかの確認

コードやテストのfailureとして直接現れない、外部のシステム構成との不一致や問題になりやすい点の指摘

外部サービスの特殊な挙動やセキュリティなど

将来的に修正しづらくなると（経験的に）分かっているデザインや実装方針の指摘

大クラス主義、よりよいデザインパターンが知られているなど

直接の実装者以外に該当コードをメンテナンスできるレベルに理解している人の確保

優先度中

複雑性の評価

シンプルな実装やライブラリで置き換えられないか？

理解の難しいコードは適切にコメントされているか？

既存ロジックとの一貫性

将来的な拡張性の評価（そもそも拡張されうるか、新規実装より拡張のほうが好まれるかも含む）

コーナーケース等バグの発見

テストが書いてあるかの確認

優先度低（システムに大きな影響を与えない問題・後からの修正が容易な問題）

コーディングスタイルの確認

外部ドキュメントの更新

Reviewer向けアドバイス

細かいコーディングスタイルより、設計を第一にレビューする

設計レビューのほうがより広範な知識を必要とする。すなわち、あなたと同じレビューコメントを付けられる人のいる可能性が低くなるため、相対的にコメントの価値が高い。

間違った設計は、その後に書かれるコードにも影響を与える。

コーディングスタイルはlinterやauto formatterで自動的に修正できるのが理想

よっぽど変なスタイルでない限り、後からリファクタリングできる

人ではなくコードを批判する

可能な限りコードを書いた個人への言及を避ける。「このコードを書くようなやり方は悪い」ではなく「このコードはこういう問題があるため好まれない」、「この設計は考え方がおかしい」ではなく「この設計はこういう場合に破綻するため修正が必要である」のようにする。

人への言及を避ける方法は言語や文化によって様々である。一般論としては、論文に書くような文体を意識すると行為者への言及を避けやすい。

良い設計やスタイルを見つけたら褒める

悪いパターンと同様に、積極的に意識するべき良いパターンというものも存在する。そういうパターンを見つけたらちゃんと褒める。

Revieweeはたまたま良いパターンを書いただけで、その真の価値に気づいていないかもしれない。実験的に新しいパターンを試してみただけで、保守性を上げるかどうかは深く検討していないかもしれない。あなたがそのパターンが良いと知っているのであれば、その知見を共有することで学習効率を上げることができる。

ポジティブなコメントは、単にそれだけで「認められている」という意識を与え、モチベーションの向上につながる。

「自分がこのコードをメンテナンスする」という観点でレビューする

コードレビューの大目的のひとつは、複数人でコードに関する知識を共有することで、誰か一人が単一障害点となることを防ぐことにある。その共有先はあなたである。

自分が理解でき、正しく保守や拡張ができると言えるコードであるかどうかを最優先に考える。

「自分だったらこう設計する」というのは、レビューされている設計が著しく保守性を欠いているのでない限り、低優先度のアドバイスの域を出ない。正しく動き、正しくテストされているコードなら後でリファクタリングできる。

Revieweeからの質問には必ず答える

Reviewerがコードの内容や背景を理解できないことがあるのと同様、Revieweeも知識量の差や単なる説明不足によって、レビューコメントを理解できないことがある。Revieweeはそのギャップを埋めるために質問をしているのだから、Reviewerは必ず答える義務がある。

Revieweeからの質問の結果、当初のコメントが間違っていたと分かることもある。その場合、「こういう時は正しい」等の正当化を並べる前に、大筋では自分のコメントが間違っていたことを認めてはっきりと述べる。

Reviewee向けアドバイス

新しいコードの影響範囲を小さくする

新しいコードはなるべくprivateな関数に閉じ込める

新しいフィールドよりローカル変数、新しいローカル変数より新しい関数を好む

影響範囲の小さいコードは多少の瑕疵があってもOKを出しやすい

既存の振る舞いが壊れないことが分かりやすい

後でリファクタリングがしやすい

コンテキストをMerge Requestやcommit message、チケット等で可能な限り説明する

Reviewerも詳細なコンテキストや要求仕様を知らないことは多々ある

Reviewerは「なぜ」この変更を「しなくてはならないのか」が知りたい。この質問に端的に一文で答えられるとよい。答えられない場合、問題の理解が甘いか、チケットの粒度が大きすぎる。

Reviewerの質問は攻撃ではない

Reviewerはそのコードを自分がメンテしないといけないという前提でレビューする

Reviewerから質問が出るのは、プレゼンすると質問が出るのと同じ。チケットとコードだけでは共有できない思想はめちゃくちゃ多い。

Reviewerの質問には必ず答える。答えられない場合、Reviewerの質問がおかしいか、もしくはReviewerがあなたの知らない知識を（この領域に関して）知っているということである。どちらにせよ「分からない」ということを明らかにして、逆にReviewerの意図を聞き出す。

「分からない」ということを明示しないと、よほど文章構成がうまくない限り、Reviewerはあなたが「Reviewerにとって未知の知識を元にして、信頼できる返事を書いている」と解釈する。そうなると認識に齟齬が生まれ、無用ないさかいの元になる。

コードへのコメントはあなたへの攻撃を意図してはいない

自分の書いたコードには愛着が多少なりともあるだろうから、批判的なコメントに傷つくことはあるかもしれない。ただ、Reviewerはあなたを傷つけるためにコメントしているのではない。単に、そのコードが問題を起こす可能性が高いと思うからコメントをしている。

批判的なコメントを受けることが多い場合、あなたの能力が低いと糾弾されているように感じるかもしれない。

「能力が低い」はある意味で正しい。問題を起こしにくいコードを最初から書ける人に比べると、あなたの生産性は相対的に低くなる。

このことについて、あなたが精神的な負担を感じるかは個人のキャリア観と、チームの一員としてのあなたに対する期待値設定の問題である。レビューコメントが精神的な負担になっているのであれば、マネージャー等に相談すること。