コードレビューはつまらないから丁寧なプルリクエストでチームの生産性向上を目指す

なぜコードレビューはつまらないのか

コードを読むのが得意なら、コードレビューもできてしかるべきなのだろう。 コードレビューが苦手なのは、スキルの欠如であり、コードを読む能力が低いからなのではないか。 熟達したプログラマなら、コードレビューに喜んで取り組み、上手に扱える人間である べき だ。 そんなふうに思っていた。

でも、いまはそう思わない。 ソースコードを読むことと、コードレビューは違う。 ソースコードを読むという行為には、未知のことがらを探索する楽しさや知る喜びがある。 ソースコードを読んで知りたいことにたどり着くと、知的好奇心が満たされる。 というのも、ソースコードを読むときには、対象のプログラムに興味があって、なにかしらの目的意識を持った状態で着手するからだ。

一方、コードレビューはそうではない。コードレビューは、検査であり間違い探しだ、という意識がある。 普段レビューするプルリクエストは、多くの場合、知的好奇心をくすぐるようなものではない。 すくなくとも、レビューを依頼されたその時点では興味を持っていない。 なぜなら、いつでも、最大の興味は自分が取り組んでいる別のタスクにあるので。 コードレビューは、自発的に開始するものではなく、自分の今現在の興味とは関係なく、いまこれを見ろと人から押し付けられるものだ。 だから、コードレビューはつまらないし、苦痛だ。

コードを読むこととコードレビューはまったく違う行為。だから、コードレビューをつまらなく感じるのは、しかたのないこと。

それから、コードレビューは、本質的に差分に対するチェックなので、その意味でも、ふつうのコードリーディングとは体験が違う。 GitHubのUI上では差分の周囲やファイル全体も見わたせるようになっているおかげで、だいぶ負担が軽減されはする。 けれど、それでも断片的な情報から判断しなければならないことには変わりがない。 差分は理解しづらい。これもコードレビューを苦痛にしているひとつの原因だと思う。

コードレビューのあるべき姿について考えるにあたって、これらのネガティブな点を認めた上で、はじめたい。

われわれは、なんのためにコードレビューをするのか

コードレビューには複数の機能がある。セキュリティーホールを防ぐ、コード品質を保つ、チーム内での知識やテクニックの伝達など。 中でも、設計的な誤りというのは後々への影響範囲が大きく、つまり修正も大変なので、コードレビューで正せれば見返りが大きい。 RDBのテーブル定義など、永続化されるデータの持ち方は、一度デプロイしてしまうと修正がめんどうなので、プルリクエストか、それよりも前の段階¹で、十分に議論して筋のいい形にしておきたい。

リファクタリングが活発に行われるという前提に立てば、ある問題の解決方法に対して、局所的なコードの記述方法が最適でないとかは、たぶん大きな問題ではない。気付いたらすぐに直すことができるから。もちろん、それらの小さなほころびも、積み重なってしまうと大きな問題になるので、日常的にリファクタリングが行われることが前提にはなるけど。

そういう意味では、チームの気付かない間に(望ましくない)変更が積み重なっていってしまうことが一番の問題だろう。 コードレビューは同期のための手段のひとつでもある。 チームが同期するための手段は、daily standupや、retrospective、slackでの非同期コミュニケーションなどいくつかあるけど、コードレビューもその重要な一つ。それらの手段が重層的に機能することで、チーム内での破滅的な認識の乖離を防ぐ。 社会のセーフティーネットでもセキュリティーでもそうだけど、単一の手段で満足するのではなく、複数の重なり合う防御手段を持つことが重要だと思う。

それから、コードレビューを欠陥を見つけるための検査だと考えるとモチベーションが上がらないと書いたけど、コードレビューを、プルリクエスト作者を助けるための行為、人助けと位置付ければ、すこしはやる気もでやすい……かもしれない。

レビューのコスト

チーム開発において、コードレビューは毎日何回も行う行為だ。 だから、前提として、一個一個のレビューにそんなに長い時間はかけられない。 できる限り効率的に行いたい。

上手に手際よくレビューできるかどうか、という問題は、実は自分の力だけで解決できる問題ではない。 なぜなら、レビューの効率性は、プルリクエストがうまく作られているかどうかに依存するからだ。 だから、プルリクエストが不十分なときの第一の対応は、効率よくレビューできるようにプルリクエストを わかりやすく作り直してくださいと要求することになる。 そのためには、なにが不足しているのかをまずは判別できなければならない。

レビューの観点

われわれの目標は、手早く効率的にレビューすることだ。つまり、どこに時間を かけない かを見定めて、見るべき観点をなるべく少なく絞る必要がある。

• 全体的な意図。なにをどうやって実現するのか(What, How)。
• 変更の文脈、位置付け。なぜこの変更が必要なのか(Why)。
• 複数の独立した問題が含まれているか
  • 含まれている場合、分割できないか検討する
• 設計の誤りは影響が後を引く可能性があるので、なるべくちゃんと見ておきたい。とくに永続化されるデータ構造のミスは、リリースしてしまうと修正が面倒なので、注意する必要がある。設計の誤りとは、例えば:
  • SQLアンチパターンに該当するようなテーブル設計になっている(実データが発生するとめんどう)
  • 本来別のAPIを新設すべきところを、既存APIへの追加パラメータで無理矢理処理している(修正後の挙動への依存が増えるとめんどう)
  • 手続の種類が増えても修正不要なように一般化できる(一度書けば済む)のに、手続の種類の数だけコード追加が必要な設計になっている(無駄な手数が増える)
• 追加・変更される挙動について、テストケースが追加されているか、テストケースの見出しレベルで簡単に見る
• 特殊な理由のある変更など、コードだけから理解できなさそうな変更は、コメントに経緯が書かれているかを見る
• UIの変更が含まれる場合、スクリーンショットが添付されているか
• QA手順が記載されているか
• これ以外のすべて: セキュリティー、局所的なコード品質などは最悪見なくてもいい。²

これらの大部分は、プルリクエストの説明が丁寧に書かれていれば、ほぼコードの詳細を読解しなくてもチェックできる点に注意。 コードを見ないならコードレビューじゃないじゃないかというツッコミが来そうだけど、詳細を隅々まで理解しようとすると時間がかかるので、実際なるべく立ち入りたくない。 もちろん、コードを眺めていてなにか気付いた点があるなら、それをフィードバックするのは何の問題もない。 ただ、一行一行目を注いで見るのは、つかれるし、時間もかかるので行わないということ。 コードレビューではなく、PRレビューとか、なにか別の、「体を表す」適切な名前があればいいんだけど…。

ちなみに、QA作業自体は、基本的にレビュアーは行わなくていいと思う。³ レビュイー自身が責任を持って動作確認を行ってからレビュー依頼を出すのは前提条件だからだ。

これら必要な情報が提供されていないプルリクエストは、まず詳細を見る前に、情報を足してもらうよう催促する必要がある。

文化を浸透させる

プルリクエストの書き方に注文があるなら、いちいち指摘するより、ガイドラインを書いた方がいいんじゃないかと思うかもしれない。実際ガイドラインは必要だけど、ガイドラインがあるだけで、だれもが理想的なプルリクエストを作ってくれて、自動的に問題が解決する、などということはまずない。実際には、ガイドラインを根付かせるために、絶え間ないフィードバックが必要。どういうプルリクエストが望ましいかについてフィードバックを与えることで、望ましいチーム文化の醸成を促進したい。

プルリクエストガイドラインには、お手本となるような、実際のプルリクエストの例もいくつか添えておけると良いと思う。

コードの質の担保について

これまでのことは、ある前提の上に書かれている。それは、コードの質は、コードレビューで漏れてしまったとしても、たぶんリファクタリングで挽回できるということだ。 もちろん、コードレビューの段階で改善できたなら、それに越したことはない。けど、レビュイーを待たせずプロジェクト全体を円滑に回すという観点を重視すると、コードの質をどう位置付けるべきかが見えてくる。

リファクタリングは、後でじっくり１人でも取り組めることだけど、レビューを待たせるのはチーム全体に悪影響をおよぼす。 小さなプルリクエストを促すためには、手早いコードレビューがとても大切だ。 だから、コードの質の担保はコードレビューにおいて、優先度は低でいいと、ぼくは位置付けている。

レビュアーの負担を最小化すべき

レビュイーとレビュアーでは、持っている情報量に大きな差がある。

レビュイーは、着手する前に、タスクの説明を読んで、その背景や理由を理解し、情報が不足していれば情報を持っている人に質問し、それらをどう実現するのがいいか何パターンか考え、良いと思った実装方針の落し穴に実装しだしてから気付いて、途中でやりかたを変えたり等等等、ひとつのプルリクエストを出すまでにもそれなりの時間をかけているはず。

コードだけから、それらの様々な経験に追い付くのは難しい。コードを一生懸命読んで意図を推測することもある程度はできるかもしれないが、コードを読み解くには時間がかかるし、第一それは思い込みで見当違いかもしれない。

レビュイーはすべて知識として持っているのだから、ちょっと手間をかけて、それらの情報を余さず文章に書き出したり、図表として表現しておく。そうすることで、チーム全体としての効率が良くって欲しい。これらを丁寧に記述しておけば後々の資産にもなる。ある変更がどういう理由で入ったかをgit blameなどから追跡するのは、日常の業務でよくあることだ。プルリクエストの説明が丁寧に書かれていれば、後から読んだ人はきっと感謝する。

丁寧なプルリクエストを起点にしてプルリクエストの回転数上昇を目指す

理想的には、ひとつのプルリクエストは、単一の目的だけを含む必要最小限の大きさであるべきだ。 そのほうがレビューは楽だし、リバートもしやすくなる。

けれども、レビューが遅いとこれが守りづらくなる。 ある変更に依存した変更を別のブランチとして分割したいということはよくあるだろう。 このとき、レビューが短時間で返ってくると期待できるのであれば、別のプルリクエストとして、分けて出しやすい。 しかし、一旦プルリクエストを出すと最初の反応が返ってくるまで数日も待たされる、というようなことが常態化 しているとこうはいかない。リファクタリングだけ切り出すなどして、できればいくつかのブランチに分割するのが理想的。それなのに、この先何回も待たされることを考えると、まとめられるものはまとめて出してしまいたい、という誘惑にかられる。

こうなってしまうと、大きなプルリクエストが常態化し、大きなプルリクエストはレビューもめんどうなので、より時間がかかり回転数が下がる。悪循環。

そうではなく、プルリクエストの説明に時間をかけることで、そこを起点にして、以下のような好循環を作りたい。

こうすることで、結果的にコード変更の回転数が上がって欲しい。

まとめ

• コードレビューがつまらないのは、その性質からして自然なこと
• コードレビューは、チームの同期を取るための手段のひとつ
• レビューの効率は、プルリクエストの質に依存する
• フィードバックによって文化を浸透させることを目指す
• コードの質はコードレビューに加えて、リファクタリングでもカバーできる
• レビュイーとレビュアーは持っている情報量がまったく違うので、ギャップを埋めるためにレビュイーが努力すべき

参考リンク