コード・レビューでの5つのアンチパターン

※本記事は、Trisha Geeによる”Five Code Review Antipatterns“を翻訳したものです。

ベスト・プラクティスは誰もが気にするが、ワースト・プラクティスの方が参考になることもある

著者：Trisha Gee
2020年5月4日

コード・レビューは欠かせませんが、常に正しく行われているとは限りません。本記事では、コード・レビューを受けたときや、プル・リクエストを送信したときに、すべての開発者がおそらく経験してきた具体的なアンチパターンを取り上げ、正したいと思います。

アンチパターン：つまらぬあら探し

以下のシナリオについて考えてみてください。コード作成者たちは、何日とまではいかなくても、何時間もかけて、最適と思われるソリューションを作成しました。複数の設計オプションを考えたうえで、もっとも適切と思われる方法を選択しました。既存アプリケーションのアーキテクチャも考慮し、しかるべき場所に変更も加えました。そのうえで、プル・リクエストとしてそのソリューションを送信するか、またはコード・レビューのプロセスを開始しました。そして、エキスパートから寄せられたフィードバックは次のようなものでした。

• 「空白ではなく、タブを使うべきだ」
• 「このセクションの波括弧の位置が気に入らない」
• 「ファイルの最後に空行が入っていない」
• 「列挙型が大文字になっている。大文字は先頭の1文字だけにすべきだ」

スタイル面で新しいコードと既存のコードが一貫しているのは重要なことですが、どう考えても人間がスタイルについてレビューする必要はありません。人間によるレビューは高価で、コンピュータではできないことが可能です。スタイルの基準が守られていることをチェックする作業はコンピュータで簡単に実行できるものであり、コード・レビューの真の目的からは外れます。

コード・レビューの際に開発者がこういったコメントを多く見かけるとしたら、チームにスタイル・ガイドがないか、スタイル・ガイドはあってもスタイルのチェックが自動化されていないかのいずれかでしょう。これを解決するためには、checkstyleなどのツールを使ってスタイルのガイドラインが遵守されるようにするか、sonarqubeを使って一般的な品質やセキュリティの問題を見つけます。このような問題への警告は、人間によるレビューに頼らずに、継続的インテグレーション環境によって行うことができます。

場合によっては、このようなチェックを自動化しにくいこともあります。たとえば、コードのガイドラインが存在しない場合や、時間とともに社内のコード・スタイルが進化し、さまざまな部分で違いが生じている場合です。このような状況でチェックを自動化するアプローチも存在します。たとえば、取り決められたコード・スタイルを適用し、その他の変更は行わないというコミットを1回だけ行うと合意することもできます。または、バグ修正や機能追加によってファイルを変更するときは、そのファイルに対して新しいスタイルを適用するように申し合わせることもできます。自動化ツールは、変更されたファイルのみをチェックするように構成できます。

チーム内にさまざまなコード・スタイルが存在し、スタイルのチェックを自動化できない場合は、次のわなにもはまりがちです。

アンチパターン：一貫性のないフィードバック

コード・レビューに開発者を呼ぶたびに、少なくとも1つの意見が増えることになります。増える意見は、おそらく1つだけではないでしょう。同時に複数の意見を持っている可能性もあります。ときに、コード・レビューが、アプローチの違いに関するレビュー担当者間の議論に移ってしまうことがあります。たとえば、ストリームまたは従来型のforループのいずれが最適かといったようなことです。チームのメンバーが同じコードについて正反対の意見を持っていたとしたら、開発者はどのようにして変更を行い、レビューを終えて、コードを本番環境にプッシュすればよいのでしょうか。

レビュー担当者が1人だけであっても、1回のレビューの中で、あるいは何回かのレビューの中で、意見が簡単に変わることもあります。レビュー担当者は、あるレビューでO(1)読取り操作のデータ構造を使うように作成者に強制しているかもしれません。一方で、次のレビューでは、同じレビュー担当者がさまざまなユースケースで複数のデータ構造があるのはなぜかと問い、単一構造で線形検索を行ってコードを簡略化することを提案するかもしれません。

このようなシナリオは、「ベスト・プラクティス」がどのようなものかについてチームに明確な考え方がない場合や、チームで何を優先すべきかがわかっていない場合に発生します。たとえば、次のような状況です。

• Javaの最新スタイルに近づくようにコードを移行していくべきか。それとも、コードの一貫性の方が重要であるため、すべての場所で「従来型」の構造を使い続けるのか。
• システムのすべてのパーツでデータ構造にO(1)読取り操作を持たせることが重要なのか。それとも、O(n)でも構わない部分があるのか。

設計に関する質問は、ほぼすべての場合において、「状況による」という答えになります。優れた答えに近づけるように、開発者はアプリケーションやチームの優先事項を理解しておく必要があります。

アンチパターン：最後の最後での設計変更

コード・レビューにおいて開発者が受け取る可能性のあるフィードバックのうち、もっとも落胆するのは、レビュー担当者がソリューションの設計やアーキテクチャに根本的に反対し、コードの完全な書き直しを強制した場合です。一連のレビューにおいて徐々に書き直させられるとき（次のセクションを参照）も、コードが容赦なく却下されて最初からやり直さざるを得なくなるときもあります。

コード・レビューは、設計レビューを行う時間ではありません。チームが従来型の「ゲートウェイ」コード・レビューを行っている場合、最終ステップとして別の開発者がコードを確認する前に、コードは動作してすべてのテストに合格しているはずです。その時点で、レビュー対象のコードに関して、数時間、数日、場合によっては数週間の作業（ただし、数週間ではないことを切に期待します。コード・レビューは小規模であるべきですが、それはまた別の話題です）が行われています。コード・レビューの際に、ベースとなる設計が誤っているという話をするのは、全員の時間を無駄にすることになります。

コード・レビューで設計レビューを行うことも可能ですが、それが目的なら、実装の最初の段階で行うべきです。その後、先に進みすぎる前に、開発者は意味のある名前や手順を含むいくつかのスタブ・クラスやスタブ・メソッド、テストなどを作成し、考え方の概略をまとめることができます。さらに、チームのメンバーからそのアプローチに関するフィードバックをもらうために、開発者がテキストや図を提出することもできるかもしれません。

チームのメンバーがゲートウェイ・レビュー（つまり、コードが完成して動作しているタイミング）で根本的な設計上の問題を見つけるようなら、もっと早い段階で問題を見つけるようにプロセスを更新すべきです。これは、前の段落で示したような別種のレビューを行うことかもしれません。あるいは、アイデアをホワイトボードに書き出して議論することや、ペア・プログラミングを行うことや、提案されたソリューションについてテック・リードと話し合うことかもしれません。最終コード・レビューで設計上の問題を見つけるというのは、開発の時間の無駄であり、コード作成者を著しく落胆させることでもあります。

アンチパターン：ピンポン・レビュー

理想の世界では、作成者がコードをレビューに提出したら、レビュー担当者がいくつかのコメントとともに明快なソリューションを提示します。作成者が提案された変更を反映させてコードを再提出したら、レビューが完了してコードはプッシュされます。これが一般的であれば、コード・レビューのプロセスを正す必要などありません。

現実の世界では、次のようなことがよく起こっています。

1. コード・レビューが始まります。
2. 複数のレビュー担当者がさまざまな提案を行います。小規模で簡単なものもあれば、明らかな解決策のない曖昧なものや、複雑なものもあります。
3. 作成者が何らかの変更を行います。最低でも、前述の簡単なものは修正します。あるいは、レビュー担当者を喜ばせるためにいくつかの変更を行うかもしれません。作成者は、詳細を確認するためにレビュー担当者に質問することもあるでしょう。または、変更を行わなかった理由を説明するためのコメントを追加するかもしれません。
4. レビュー担当者が戻ってきて、いくつかの更新を受け入れ、その他についてさらにコメントしたうえで、元々のコードで気に入らない部分を他にも見つけ、質問に回答し、レビューのコメントについて他のレビュー担当者や作成者と議論します。
5. コードの作成者はさらに変更を加え、コメントや質問の追加などを行います。
6. レビュー担当者は変更をチェックし、さらにコメントや提案の追加などを行います。
7. ステップ5と6が繰り返されます。永久に終わらないこともあるかもしれません。

理論上、このプロセスでは、変更やコメントはゼロに向かって収束し、やがてコードが完成するはずです。もっとも意気消沈するのは、繰り返しのたびに、クローズした古い問題の数以上の新しい問題が出てくるケースです。そのような場合、チームは「コード・レビューの無限ループ」に入っています。この無限ループは、いくつかの理由で起こります。

• レビュー担当者が細かいあら探しをした場合や、一貫性のないフィードバックを行った場合に発生します。このような傾向を持つレビュー担当者にとって、指摘すべき問題や行うべきコメントはいくらでも存在します。
• レビューに明らかな目的がない場合や、レビューの際に従うべきガイドラインがない場合に発生します。そのような場合、すべてのレビュー担当者が存在するすべての問題を発見しなければならないと感じているからです。
• レビュー担当者のコメントにおいてコードの作成者に要求していることが明確でない場合に発生します。各コメントは必要な変更を示しているのでしょうか。すべての質問は、コードが自己記述的でなく改善する必要があると暗黙的に言っているのでしょうか。それとも、一部のコメントは次回のためにコードの作成者を教育しようとしているだけであり、提示されている質問はレビュー担当者の理解を助け、認識を深めるためだけのものなのでしょうか。

コメントは、根本的な問題なのかそうでないのかがわかるようにすべきです。また、承認前にコードを変更する必要があるかどうかを決めるのがレビュー担当者であるなら、コード作成者が取るべき正確なアクションが明確になっている必要があります。

さらに、レビューが「完了した」と判断する責任者は誰なのかを理解することが重要です。これを実現するのは、チェック済み項目や合格した項目を記したタスク・リストや、「問題なし」と判断する資格がある個人です。また、一般的には、こう着状態を打破して意見の相違を解消できる人も必要になります。これは上級開発者や、リーダー、アーキテクトかもしれませんし、場合によっては、チームで信頼を集めているコード作成者かもしれません。ただし、誰かがいずれかのタイミングで「レビューは完了した」または「この手順が終われば、レビューは完了する」と言う必要があります。

アンチパターン：ゴースト・レビュー

ゴースト・レビューは、筆者が一番犯しがちなアンチパターンです。レビュー担当者やコード作成者が、コード・レビューに対応しないとき（場合によっては最初から）があります。重要な機能や興味深い機能の確認を依頼されているため、「きちんと見ること」ができる「しかるべきとき」までそのままにしているのかもしれません。または、レビューが大規模で、時間を長く取っておきたいのかもしれません。あるいは、コード作成者が、1回（または20回）の反復の後、それ以上コメントを読んで回答することができなくなり、「頭がすっきりするまで」待つことにしているのかもしれません。

似たような経験はないでしょうか。

原因は何であれ、レビュー・プロセスを担当する誰かがレビューに対応しないことがあります。その場合、その人がコードを見るまでレビューが停止状態になる可能性があります。これは無駄なことです。誰かが時間をかけてアセット（新しいコード）を作成しても、本番環境にアップされるまでは何の価値も生みません。実際、コードベースに含まれる他のコードにどんどん後れを取り、腐ってしまうでしょう。

ゴースト・レビューが発生する要因はいくつかあります。その1つが、大規模なコード・レビューです。変更されたファイルを何十、何百と苦労して確認したい人はいません。コード・レビューが、実際の仕事や、成果物の一部とは見なされていない点も要因の1つです。難しいコード・レビューや落胆するコード・レビューの経験も、大きな要因です。コーディング（通常、開発者はこれを楽しんでいます）を中止して、うんざりするほど単調で時間がかかる作業に向かいたいと思う人はいません。

以下に、ゴースト・レビューに対処するための提案を記します。

•     コード・レビューを小規模に保ちます。どの程度の規模かはチームごとに決める必要がありますが、レビュー作業は数週間とはならず、数時間か数日となるべきでしょう。
• コード・レビューの目的や、レビュー担当者が確認すべきポイントを明確にします。「コードに潜んでいるかもしれないあらゆる問題を探す」というようなスコープでは、モチベーションを維持するのは困難です。
• 開発プロセスの中でコード・レビューの時間を設けます。

最後のポイントには、チームの規律が必要になるかもしれません。または、設定した目標や、開発者の生産性を判定する何らかの仕組みによって優れたコード・レビューを報奨するなどして、レビューを行う時間を取ることをチームで推奨した方がよいでしょう。

チームでできること

確かなコード・レビュー・プロセスを作成することに集中してください。筆者のブログにも記載していますが、ここでそのプロセスの一部について説明します。

コード・レビューを行う際には、考慮すべきことが数多くあります。開発者がコード・レビューのたびにそういった点をすべて心配するなら、どんなコードでもレビュー・プロセスの通過はほぼ不可能でしょう。誰にとってもうまくいくコード・レビュー・プロセスを実現する最適な方法は、以下の問いかけについて考えることです。    

• なぜチームはレビューを行うのでしょうか。明確に定義された目的があれば、レビュー担当者は仕事がしやすくなります。また、コード作成者も、レビュー・プロセスで予期せぬ嫌なことに遭遇しにくくなります。
• チームのメンバーは何を求めているのでしょうか。目的がある場合、開発者はコードをレビューする際に、その目的にいっそう合った成果を生み出すことができます。
• 関与するのは誰でしょうか。レビューを行うのは誰で、意見の対立を解決する責任者は誰で、最終的にコードにゴー・サインを出すのは誰でしょうか。
• チームはいつレビューを行うのでしょうか。また、レビューはいつ完了するのでしょうか。レビューは、開発者がコードに手を入れている間や、そのプロセスが終わるタイミングで、繰り返し行われる可能性があります。最終的にコードにゴー・サインを出すのはいつかという明確な取り決めがなければ、レビューが延々と続く可能性もあります。
• チームはどこでレビューを行うのでしょうか。コード・レビューに特定のツールは必要ありません。そのため、作成者が自席で同僚に順を追ってコードを説明するなどして、簡単にレビューを行うこともできます。

このような問いかけに答えたら、チームは適切に機能するコード・レビュー・プロセスを作れるはずです。なお、レビューの目的はコードを本番環境に投入することであり、開発者の賢さを証明することではないことは心しておいてください。

まとめ

明確なコード・レビュー・プロセスを準備すれば、コード・レビューのアンチパターンを排除できます。少なくとも、緩和することはできます。コード・レビューを行うべきと考えているチームは多くても、なぜレビューを行うのかについての明確なガイドラインを定めているチームは多くありません。

チームが異なれば、必要になるコード・レビューの種類も異なります。アプリケーションが異なれば、ビジネスやパフォーマンスの要件が異なるのと同じです。最初のステップは、なぜチームにコード・レビューが必要なのかを明らかにすることです。そのうえで、チームは以下の作業に入ることができます。    

• 簡単なチェックを自動化する（コード・スタイルのチェック、よくあるバグの検出、セキュリティ問題の発見など）
• いつレビューを行うか、何を求めるのか、誰がレビューの終了を判断するのかに関する明確なガイドラインを作成する
• コード・レビューを開発プロセスの重要なポイントにする

なぜコード・レビューを行うのかにフォーカスすることは、チームがコード・レビュー・プロセスのベスト・プラクティスを作成する際に役立ちます。そうすることで、コード・レビューのアンチパターン回避がより一層容易になります。