コードレビューの目的と考え方

まえがき

コードレビューの有効性が説かれるようになって久しい。しかし、コードレビューをするべきという観念ばかりが先立ってしまい、何のためにコードレビューをするのか、どのような点をレビューするべきなのかといった、目的や進め方に対する意識が曖昧なケースも数多くあるように思われる[6]。コードレビューの目的を理解せずに惰性でレビューしているだけでは、いずれレビューそのものが形骸化し、単に承認のハンコをもらうためだけの無価値なプロセスになってしまう。

かくいう自分も、はっきりとコードレビューの目的を意識したことはなかった。単純に、人間は間違えるものだし、様々な要因によってより良い解法を見落としてしまうものだから、そういった間違いや改善を指摘するためにコードレビューがある、という程度の漠然とした理解だった。コードに関わる人数が少ないうちはそれでもいいかもしれないが、コミッターが増えてくるにつれ、レビューで指摘した方がいいのか微妙なケースや、レビュワー間での意見の食い違い、更にはレビュワーの意見を頑として受け入れないコミッターが出てくるなど、目的が明文化されていないことによる弊害が目に付くようになってきた。

本稿では、コードレビューの目的が何であるかを再考するとともに、その目的を達成するための鍵となる考え方をまとめる。これらは自分の経験に基づくものもあるが、インターネット上に公開されている文書から拝借したものも含まれる。本稿を書くに際して、特に参考にしたWebサイト等の一覧は末尾に載せている。その他にサーベイ段階で発見した文書のまとめは Scrapboxで公開している。

本稿に加えて他に参考文献を読む場合、まずはCode Review Best Practices[3]を読むことをお勧めする。この記事はコードレビューの進め方について、本稿では省略した部分も含めて大変よくまとまっている。さらに時間があるのであればeng-practices | How to do a code review[1]とeng-practices | The CL author’s guide to getting through code review[2]を読むとよい。

コードレビューの目的

大目的

  • 実装者の他に少なくとも一人、マージされたコードをメンテナンスできる人が存在するようにする。
  • 実装内容について合意を形成する。合意内容は要求仕様と実装が一致していることの他に、既存のコードとの一貫性やメンテナンスのしやすさ、スタイルに関するものなど全て含まれる。

コードレビューの主要な目的は、書いたコードの内容を他のチームメンバーに理解してもらうことである[1][3][7]。「3か月前の自分は他人」という言葉に代表されるように、コードを書いた本人ですら時間が経った後には詳細を忘れてしまっており、もう一度コードを読み直すしかないというケースは非常に多い。今すぐに他人が読んで理解できないコードは、もちろん将来も理解されていないし、何なら書いた本人も3か月後には理解できなくなってしまう。そうなったコードは拡張することも難しいし、直接そのコードを触らなくても他の箇所の変更が間接的に影響を及ぼすかもしれないため、システムの安定性に対して大きな負債を抱えることになる。

問題を引き起こしている箇所のコードが読めないときはシステムが壊れても諦める、という選択を取るのでない限り、最低でも他に1人、理想的には全てのチームメンバーが変更内容を理解しているべきである。逆にレビュワーは、いずれ自分がそのコードを保守、修正、拡張しないといけないという意識でコメントをつけるべきである。したがって、レビューコメントはミスや改善可能な点の指摘だけではなく、コードの意図に関する質問も含まれる。

よく言われるようなバグの指摘は目的としては含めない（指摘するなと言っているわけではなく、バグを発見するためにレビューするのではないという意味）。理由は以下の2点である。

  • バグの指摘はレビューコメント全体の15%を占めるにすぎず[5]、レビューで見落とされるバグも多いという指摘がある[4][5]。
  • 自動テストの網羅性を上げたほうが中長期的にはメリットが大きい。

小目的

どのように大目的を達成するかはプロジェクトやチーム規模、企業風土など様々な要因によって変わりうる。ここでは、大多数の環境で大目的の一部（あるいは、大目的の具体的な言い換え）になると考えられるものを挙げる。

  • コードがシステム全体の信頼性を（大きくは）下げないこと
    • Googleのeng-practices[1]では"Don’t accept CLs that degrade the code health of the system"と言い切っているが、場合によっては多少の信頼性低下もトレードオフとして受け入れてよいと思う。その境界はチームとプロジェクトに強く依存する。
    • テストカバレッジやコードの複雑性が信頼性の指標となる。

チェックリスト

コードレビューに際し、見落としがないようにチェックリストを活用するとよい[8]。チェック項目を考える際には以下の点に注意する。

  • 高優先度の指摘を一通り洗い出すまで、より低い優先度のレビューについては考えないようにする[1]。
  • 優先度の低い指摘はそれと分かるようにする。コメントの文頭に[nit]を付けるなど（nitはnitpickingの略で、ささいな粗探しという程度の意味）[1][3]。
  • 優先度の低い指摘が対応されていなくても、機能の重要度や時間的制約を加味して考え、場合によってはGoサインを出す。
    • 優先度の低いものは、大抵後からでも修正できる。後からの修正が難しいような問題は優先度が高いものとして考える。

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

  • 要求仕様と実際の機能が一致しているかどうかの確認
  • 外部サービスの特殊な挙動やセキュリティ機構など、コードやテストのfailureとして直接現れない、環境との不一致や問題になりやすい点の指摘
  • 将来的に修正しづらくなると（経験的に）分かっているデザインや実装方針の指摘

優先度中

  • 複雑性の評価
    • シンプルな実装やライブラリで置き換えられないか？
    • 理解の難しいコードは適切にコメントされているか？
  • 既存ロジックとの一貫性
  • 将来的な拡張性の評価（そもそも拡張されうるか、新規実装より拡張のほうが好まれるかも含む）
  • コーナーケース等バグの発見
  • テストが書いてあるかの確認

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

  • コーディングスタイルの確認
  • 外部ドキュメントの更新

レビューを負担にしないために

レビューサイズのコントロール

大きい変更をレビューするのは負担が大きい。規模が大きくなればなるほど相互作用するコードも増えてくるし、見落としもその分多くなる。また、大量のコメントがついたコードレビューは単純に見づらい。Code authorの視点からも、レビューに時間がかかってマージが遅れるのは望ましくない。

レビュワーは、コードレビューが大きすぎると感じた場合は細かく分割することをcode authorに求めてよい[1]。レビューに20分以上かかるラインがひとつの目安となる[3]。レビューが大きくなりすぎる理由としては以下のような理由が考えられる。

  • 互いに無関係な変更が1つのレビューにまとめられてしまっている。
  • リファクタリングと機能変更が一緒くたになっている。
  • 既存のコードまたは実装方針が悪く、巨大な変更が余儀なくされている。

このうち最初のものは、機能同士が無関係であることを説明できれば分割してもらうのは容易い。2つ目は、リファクタリングと機能変更を分けてレビューに回すことで解消できる。普通はリファクタリングで触る個所と機能変更で触る箇所は重なっているため、code authorも両者の分離を意識してコードを書く必要がある。一般論として「機能は変更しない大規模なリファクタリング」と「機能を変える小規模な変更」を別々にコミット、レビュー、デプロイすると認知負荷を軽減しやすい。

最後のものに関しては、おそらくコードレビューよりも外側での対処が必要となる。既存のコードがあまりにも複雑で、そのままでは変更が巨大になることをどうしても避けられないのであれば、リファクタリングに一定の工数を割くことを検討した方がよい。実装方針が悪い場合は、code authorのスキル不足が考えられるため、レビューに留まらず1 on 1やペアプログラミング等を活用した教育も視野に入れる必要がある。場合によっては無理を通してマージしてしまっても良いが、レビューが不完全になるぶん、挙動に対してはっきりとコンセンサスの取れていないコードがマージされてしまうリスクを意識する必要がある。

誰がレビューをするか

コードを理解している人を可能な限り増やす、という点ではコードに関わる人全員がレビューすることが望ましく思えるが、全員がレビューに割く時間を捻出することは現実的ではないし、非効率でもある。さらに議論に関わる人間が増えると意見がまとまりにくくなる。このような理由から、レビュワーの人数は1～2人程度が望ましいと思われる[3]。この場合、1人はTech Lead等プロジェクト全体を俯瞰する役割の人、もう1人は変更されている箇所に特に詳しいと思われる人（git blameしたときにたくさん名前が出てくるなど）にする。

人数を限る理由は、先述の通り非効率性と議論のまとまりやすさによるものが大きい。したがって、これらの問題を別の手段で解決できているなら多すぎることを特別問題視する必要はない。例えば、レビュワーとしてコードを読むこと自体が人のコードを読む練習として機能するし、そのプロジェクトやプログラミング自体に不慣れな人にとっては望ましいパターンを覚える絶好の機会でもある。議論が紛糾しそうになった時にどうやって落としどころを見つけるかが事前に共有されていれば、多人数でレビューすることは特別危険ではない。

議論をどうまとめるか

コードは書いたようにしか動かない。コードを批判するにせよ、批判から防衛するにせよ、そこには明確なロジックとメリット・デメリットの説明があるはずである。そういった説明をできないのであれば、基本的には相手の言っていることを受け入れるのが正しい。

議論に参加している各々が少しずつ異なる軸で実装を評価し、こっちがいい、いやあっちがいいと議論が紛糾することはよくある。実際にレビューされている実装と、それに対して提案された修正がどっちも甲乙つけがたいのであれば、Code authorの意見を尊重する[1]。既に実装が書かれている分、手戻りが少なく楽だからである。適切なコードレビューの手順を踏めば、その実装に対する問題点は既に共有されているはずであるから、将来その問題が表面化したときの問題解決は、レビューがされなかった場合よりも格段に楽なはずである。

批判と個人攻撃

「コードレビューでの批判はコードに向けられたものであり、個人に向けられたものではない」というメッセージは様々なところで強調されている[1][2][3][6]。コードレビューを個人攻撃だと捉えてしまう人に対し、そうではなくてこれは論理的な議論のプロセスなんだよ、だから傷つく必要はないんだよと諭し、心理的安全性を向上させるためのメッセージだとされている。このメッセージは欺瞞とまでは言わないが、多分に建前を含んだ言い方だとは思う。

コードレビューの際、code authorが誰であるかは大きな情報を含んでいる。過去に同じ箇所を何度も変更したことがある人のコードは信頼できるだろう。逆に、初めてその箇所を変更する人や、他のレビューで問題の多い変更を出してきた人に対しては注意深くなるだろうし、そうするべきだろう。同じような理由により、レビューコメントに何を書くかもcode authorによって変わりうる。以前の会話やコミット内容などから一定以上の習熟度があると分かっている人に対しては簡潔なコメントを書くだろうし、習熟度の足りていなさそうな人には時間をかけてでも詳細なコメントを書くだろう。こういった事情から、レビューコメントはcode authorに対して多かれ少なかれ、また意識的にせよ無意識的にせよカスタマイズされることになる。これは単にcode authorも人間なのだからウェットな配慮も必要であるというだけの話ではなく、その人が必要としている情報を過不足なく提供することが議論の進め方としてもっとも効率が良い、という実用上の理由も大きい。人のアレコレに触れなければpolitically correctだし心理的安全性も害さないからみんなハッピー、というような単純な話ではない。

このようなレビュワーの事情に加えて、code author側にもコメントを個人的に受け取る理由がある。プログラマーにとって自分の書いたコードというものは、少なからず自分のアイデンティティへと結びついているものである。例えば仕事で書いたコードであれば、それは給与に直結する。そもそもプログラムの良し悪しは、思考の質の良し悪しが色濃く反映される。レビューを書いた側も個人に向けた要素があり、受ける側もコードと個人を結び付けて考えるのであれば、レビューコメントに対して個人的な感情が入ってしまうことはなんら不思議ではない（この辺の話は以前のエントリでもまとめている[9]）。

以上の議論を踏まえると、より適切なメッセージは「コードレビューでの批判は、少なくともコードに疑問の余地があるからなされている」だと思う。そもそもコメントを攻撃と受け取るかどうかは、受け手の思考パターンによるものが大きい。場合によっては心に大きく刺さるかもしれないが、何もレビュワーもcode authorをめちゃくちゃに破滅させてやろうと思ってコメントしているのではなく、追加の対話なしでは受け入れがたいコードがただそこにあるからコメントしているのである。もちろんレビュワーの方も、読み手がどう思うかを多少なりとも考えて、あまり度が過ぎないように注意してコメントするべきであるし、code authorの習慣や能力に対する悪口は言うべきではない（どうしても言いたい場合は組織ならマネージャーに言う。インターネットなら単に相手を無視する）。この辺の機微は普通の人間関係、普通の会話と変わらない。

レビュワー向けアドバイス

  • 細かいコーディングスタイルより、設計を第一にレビューする[1]
    • 設計レビューのほうがより広範な知識を必要とする。すなわち、あなたと同じレビューコメントを付けられる人のいる可能性が低くなるため、相対的にコメントの価値が高い。
    • 間違った設計は、その後に書かれるコードにも影響を与える。
    • コーディングスタイルはlinterやauto formatterで自動的に修正できるのが理想
    • よっぽど変なスタイルでない限り、後からリファクタリングできる
  • 「自分がこのコードをメンテナンスする」という観点でレビューする
    • コードレビューの大目的のひとつは、複数人でコードに関する知識を共有することで、誰か一人が単一障害点となることを防ぐことにある。その共有先はあなたである。
    • 自分が理解でき、正しく保守や拡張ができると言えるコードであるかどうかを最優先に考える。
  • 自分の考え方を押し付け過ぎない
    • Code authorの書いたコードと自分のイメージが食い違っていた場合でも、書かれたコードのほうが劣る点を明確に指摘できないのであれば、Code authorの方針を尊重する[1]。
    • もちろん「自分ならこう書く」といったアドバイスを否定するものではない。
  • 人ではなくコードを批判する
    • 可能な限りコードを書いた個人への言及を避ける。「このコードを書くようなやり方は悪い」ではなく「このコードはこういう問題があるため好まれない」、「この設計は考え方がおかしい」ではなく「この設計はこういう場合に破綻するため修正が必要である」のようにする。
      • 人によってはコードと自我がかなり密接にくっついている場合があるので、言葉遣いは注意するに越したことはない。
    • 人への言及を避ける方法は言語や文化によって様々である。一般論としては、人ではなく物を主語にした論文のような文体を意識すると、個人への言及と取られるような文を避けやすい。
  • 良い設計やスタイルを見つけたら褒める
    • 悪いパターンと同様に、積極的に意識するべき良いパターンというものも存在する。そういうパターンを見つけたらちゃんと褒める。
    • Code authorはたまたま良いパターンを書いただけで、その真の価値に気づいていないかもしれない。実験的に新しいパターンを試してみただけで、保守性を上げるかどうかは深く検討していないかもしれない。あなたがそのパターンが良いと知っているのであれば、その知見を共有することで学習効率を上げることができる。ポジティブなコメントは、単にそれだけで「認められている」という意識を与え、モチベーションの向上につながる[4]。
  • Code authorからの質問には必ず答える[1][3]
    • レビュワーがコードの内容や背景を理解できないことがあるのと同様、Code authorも知識量の差や単なる説明不足によって、レビューコメントを理解できないことがある。Code authorはそのギャップを埋めるために質問をしているのだから、レビュワーは必ず答える義務がある。
    • Code authorからの質問の結果、当初のコメントが間違っていたと分かることもある。その場合、「こういう時は正しい」等の正当化を並べる前に、大筋では自分のコメントが間違っていたことを認めてはっきりと述べる。

Code author向けアドバイス

  • レビューはなるべく小さくする[2][3]
    • レビュワーへの負荷を軽減するため、コードレビューの大きさは小さい方がよい。可能な限り小さい変更で目的を達成できるようにする。
    • 新しいコードはなるべくprivateな関数に閉じ込める
    • 新しいフィールドより新しいローカル変数、新しいローカル変数より新しい関数を作るようにする。影響範囲の小さいコードは多少の瑕疵があっても、以下の理由によりOKを出しやすい。
  • コンテキストをMerge Requestやcommit message、チケット等で可能な限り説明する[2]
    • レビュワーも詳細なコンテキストや要求仕様を知らないことは多々ある。
    • レビュワーは「なぜ」この変更を「しなくてはならないのか」が知りたい。この質問に端的に一文で答えられるとよい。答えられない場合、問題の理解が甘いか、チケットの粒度が大きすぎる。
  • レビュワーの質問は攻撃ではない
    • レビュワーはそのコードを自分がメンテしないといけないという前提でレビューする
    • レビュワーから質問が出るのは、プレゼンすると質問が出るのと同じ。チケットとコードだけでは共有できない思想はめちゃくちゃ多い。
    • レビュワーの質問には必ず答える[1][3]。答えられない場合、レビュワーの質問がおかしいか、もしくはレビュワーがあなたの知らない知識を（この領域に関して）知っているということである。どちらにせよ「分からない」ということを明らかにして、逆にレビュワーの意図を聞き出す。 -「分からない」ということを明示しないと、よほど文章構成がうまくない限り、レビュワーはあなたが「レビュワーにとって未知の知識を元にして、信頼できる返事を書いている」と解釈する。そうなると認識に齟齬が生まれ、無用ないさかいの元になる。
  • コードへのコメントはあなたへの攻撃を意図してはいない
    • 自分の書いたコードには愛着が多少なりともあるだろうから、批判的なコメントに傷つくことはあるかもしれない。ただ、レビュワーはあなたを傷つけるためにコメントしているのではない。単に、そのコードが問題を起こす可能性が高いと思うからコメントをしている。
      • 世の中は広いので、本当に攻撃を意図してコメントする人もたまにはいる。そういうケースはもはやコードレビュー以前にコミュニケーションの問題なので、通常の社会的制裁の手段に則って対処する（上司に報告する、無視するなど）。
    • 批判的なコメントを受けることが多い場合、あなたの能力が低いと糾弾されているように感じるかもしれない。
      • 「能力が低い」はある意味で正しい。問題を起こしにくいコードを最初から書ける人に比べると、大量のレビューコメントを受け取るような人の生産性は相対的に低くなる。このことについて精神的な負担を感じるかは、個人のキャリア観と期待値設定の問題である。レビューコメントが精神的な負担になっているのであれば、マネージャー等に相談すること。

参考文献