How to Do Code Reviews Like a Human (Part One)

Michael Lynch

人間らしくコードレビューを行う方法(前編)

原文は Michael Lynch により に公開されました。 このブログを購読する

最近、コードレビューのベストプラクティスについての記事をよく読んでいます。気づいたのですが、そうした記事はバグを見つけることばかりにフォーカスしていて、レビューを構成する他のほぼすべての要素を無視しています。発見した問題を建設的かつプロフェッショナルな方法で伝えること? どうでもいい! とにかくバグをすべて見つければ、あとはなんとかなる、といった調子です。

そこでひらめきました。コードでそれが通用するなら、恋愛でも同じではないか、と。というわけで、開発者の恋愛を支援するための新作電子書籍を発表します。

電子書籍の表紙

私の画期的な電子書籍では、パートナーの欠点をできるだけ多く見つけるための実証済みのテクニックを伝授します。この電子書籍では以下の内容は扱いません。

  • パートナーに問題を共感と理解をもって伝える方法。
  • パートナーが弱点を克服するのを手助けする方法。

コードレビューに関する文献を読んだ限りでは、恋愛におけるそうした側面は自明であり、わざわざ論じる価値はないのです。

この電子書籍は良い本だと思いますか? きっとあなたは「いやいやいや!」と叫んだはずです。

では、なぜコードレビューについてはそんな語り方がされているのでしょうか。

私が読んだ記事は、すべての開発者がロボットである未来から来たのだと考えるしかありません。そんな世界では、チームメイトは自分のコードに対する配慮のない批判を歓迎します。なぜなら、そうした情報を処理することが、彼らの冷たいロボットの心を温めるからです。

ここでは大胆にも、あなたが改善したいのは現在、つまりチームメイトが人間である世界でのコードレビューだと仮定します。さらに大胆に、同僚との良好な関係はそれ自体が目的であり、単に欠陥あたりのコストを最小化するために調整する変数ではないと仮定します。もしそうだとしたら、あなたのレビューのやり方はどう変わるでしょうか。

この記事では、コードレビューを単なる技術的なプロセスとしてだけでなく、社会的なプロセスとしても捉えるためのテクニックについて論じます。

コードレビューとは何か

「コードレビュー」という言葉は、チームメイトの肩越しにコードを眺めるだけの気軽なものから、20人が集まってコードを1行ずつ解剖する会議まで、幅広い活動を指すことがあります。本記事ではこの言葉を、形式的で文書に基づくものの、対面でのコードインスペクション会議を何度も行うほど重厚ではないプロセスを指すものとして使います。

コードレビューのフロー

コードレビューの参加者は、コードを書いてレビューに送る作成者(author)と、コードを読んでチームのコードベースにマージする準備ができたかを判断するレビュアー(reviewer)です。レビューには複数のレビュアーがいることもありますが、ここでは話を簡単にするため、あなたが唯一のレビュアーであると仮定します。

コードレビューが始まる前に、作成者はチェンジリストを作成しなければなりません。これは、作成者がチームのコードベースにマージしたいソースコードへの変更のまとまりです。

レビューは、作成者がチェンジリストをレビュアーに送ったときに始まります。コードレビューはラウンド単位で進行します。各ラウンドは、作成者とレビュアーの間での1回の往復に相当します。作成者が変更を送り、レビュアーがそれに対して文書でフィードバックを返すのです。すべてのコードレビューは1回以上のラウンドで構成されます。

レビューは、レビュアーが変更を承認したときに終了します。これは一般にLGTM(“looks good to me”の略)と呼ばれています。

なぜ難しいのか

プログラマーが自分では素晴らしいと思っているチェンジリストを送ってきたときに、あなたが「そうではない理由」を長々と書き連ねて返すのは、非常にデリケートなメッセージを伝えることになります。

それが私がITを懐かしく思わない理由の一つです。プログラマーはとても付き合いづらい人たちですから……。例えば航空業界では、自分のスキルを大きく過大評価している人はみんな死んでいます。

-Philip Greenspun, co-founder of ArsDigita, excerpted from Founders at Work

作成者は、自分のコードへの批判を、自分が無能なプログラマーだという含意として受け取ってしまいがちです。コードレビューは知識を共有し、十分な情報に基づいたエンジニアリング上の意思決定を行う機会です。しかし、作成者が議論を個人攻撃だと感じてしまえば、それは実現しません。

これだけでも十分難しいのに、さらに文章で考えを伝えるという課題も加わり、誤解のリスクはより高まります。作成者にはあなたの声のトーンも身振りも伝わらないため、フィードバックを慎重に表現することが一層重要になります。防衛的になっている作成者にとっては、「ファイルハンドルを閉じ忘れていますよ」という何気ない指摘でさえ、「まさかファイルハンドルを閉じ忘れるなんて信じられない! 本当に馬鹿だな」というように聞こえてしまうことがあるのです。

テクニック

  1. 面倒な部分はコンピュータに任せる
  2. スタイルの議論はスタイルガイドで決着させる
  3. レビューはすぐに始める
  4. 高い抽象度から始めて徐々に詳細へ
  5. コード例は惜しみなく示す
  6. 「you」は使わない
  7. フィードバックは命令ではなく依頼として伝える
  8. 指摘は意見ではなく原則に結びつける

面倒な部分はコンピュータに任せる

会議やメールなどの中断がある中で、コードに集中できる時間は限られています。精神的なスタミナはさらに不足しています。チームメイトのコードを読むのは認知的負荷が高く、高度な集中力を要します。コンピュータにできること、とりわけコンピュータの方がうまくできることに、こうした貴重なリソースを浪費してはいけません。

空白文字のエラーは分かりやすい例です。人間のレビュアーがインデントのミスを見つけて作成者と協力して修正するのにどれだけの労力がかかるか、自動フォーマットツールを使う場合と比べてみてください。

人間のレビュアーが必要とする労力フォーマットツールが必要とする労力
  1. レビュアーが空白文字の問題を探し、誤ったインデントを見つける。
  2. レビュアーが誤ったインデントを指摘するコメントを書く。
  3. レビュアーがコメントを読み返し、明確で非難めいていない表現になっているか確認する。
  4. 作成者がコメントを読む。
  5. 作成者がコードのインデントを修正する。
  6. レビュアーが作成者が指摘に適切に対応したか確認する。
何もなし!

右側が空なのは、作成者が「保存」を押すたびに空白文字を自動でフォーマットしてくれるコードエディタを使っているからです。最悪の場合でも、作成者がコードをレビューに送り、継続的インテグレーションの仕組みが空白文字が不正であることを報告します。作成者はレビュアーが一切気にすることなく問題を修正できるのです。

コードレビューの中で自動化できる機械的なタスクを探してみてください。よくあるものは以下のとおりです。

タスク自動化ソリューション
コードがビルドできることを確認する継続的インテグレーションソリューション(TravisCircleCIなど)。
自動テストがパスすることを確認する継続的インテグレーションソリューション(TravisCircleCIなど)。
コードの空白文字がチームのスタイルに合致していることを確認するコードフォーマッタ(ClangFormat(C/C++フォーマッタ)やgofmt(Goフォーマッタ)など)。
未使用のimportや未使用の変数を検出するコードリンター(pyflakes(Pythonリンター)やJSLint(JavaScriptリンター)など)。

自動化は、レビュアーとしてより有意義な貢献をするのに役立ちます。importsの順序やソースファイル名の命名規則といった一連の問題を無視できるようになれば、機能的なエラーや可読性の弱点といった、より興味深い問題に集中できます。

自動化は作成者にもメリットがあります。何時間もかかるはずの不注意なミスを数秒で発見できるようになります。即時のフィードバックにより、作成者の頭の中に関連するコンテキストがまだ残っているため、そこから学ぶのも修正するのも容易でコストも低く済みます。さらに、自分のくだらないミスについて指摘されるとしても、あなたではなくコンピュータから言われる方が自尊心ははるかに傷つきません。

チームと協力して、これらの自動チェックをコードレビューのワークフローに直接組み込みましょう(例:pre-commitフック in Gitやwebhooks in GitHub)。もしレビュープロセスで作成者がこれらのチェックを手動で実行しなければならないのであれば、そのメリットのほとんどは失われてしまいます。作成者はどうしても時々実行を忘れてしまい、結局あなたが自動化で対応すべき単純な問題をレビューし続けなければならなくなるからです。

スタイルの議論はスタイルガイドで決着させる

スタイルについての議論はレビューにおいて時間の無駄です。一貫したスタイルは確かに重要ですが、コードレビューは中括弧をどこに置くかで言い争う場ではありません。レビューからスタイル論争を排除する最善の方法は、スタイルガイドを維持することです。

典型的なスタイル論争

良いスタイルガイドは、命名規則や空白文字のルールといった表面的な要素だけでなく、特定のプログラミング言語の機能をどのように使うかも定義します。例えばJavaScriptやPerlは機能が豊富で、同じロジックを実装する方法が何通りもあります。スタイルガイドは物事を行う「唯一の正しい方法」を定義することで、チームの半分がある一連の言語機能を使い、もう半分がまったく別の機能を使っているといった事態を防ぎます。

スタイルガイドがあれば、どの命名規則が優れているかを作成者と議論してレビューのサイクルを無駄にする必要はありません。単にスタイルガイドに従えばよいのです。もしスタイルガイドで特定の問題について規則が定められていなければ、そもそも議論する価値はほぼありません。ガイドでカバーされていないスタイルの問題に遭遇し、それが議論するほど重要であれば、チームで話し合って決めましょう。そしてその決定をスタイルガイドに記録すれば、二度と同じ議論をする必要がなくなります。

選択肢1:既存のスタイルガイドを採用する

オンラインで検索すれば、すぐに使える公開されたスタイルガイドが見つかります。Googleのスタイルガイドが最もよく知られていますが、合わなければ他のものも見つかるでしょう。既存のガイドを採用すれば、一から作成する多大なコストをかけずにスタイルガイドのメリットを得られます。

欠点は、組織がスタイルガイドを自らの特定のニーズに最適化していることです。例えばGoogleのスタイルガイドは、新しい言語機能の使用について保守的です。家庭用ルーターから最新のiPhoneまで、あらゆる環境で動作しなければならない巨大なコードベースを抱えているからです。もしあなたが単一のプロダクトを持つ4人のスタートアップであれば、最先端の言語機能や拡張をより積極的に使うという選択もできるでしょう。

選択肢2:独自のスタイルガイドを段階的に作成する

既存のガイドを採用したくない場合は、自分たちで作成することもできます。コードレビュー中にスタイルの議論が起こるたびに、チーム全体に問題を提起し、正式な規約をどうすべきか決めましょう。合意に達したら、その決定をスタイルガイドに明文化します。

私はチームのスタイルガイドをMarkdown形式でソース管理下(例:GitHub pages)に置くことを好んでいます。そうすれば、スタイルガイドへの変更はすべて通常のレビュープロセスを経ることになり、誰かが明示的に変更を承認し、チームの全員が懸念を提起する機会を得られます。WikiやGoogle Docsも許容できる選択肢です。

選択肢3:ハイブリッドアプローチ

選択肢1と2を組み合わせることで、既存のスタイルガイドをベースとして採用しつつ、それを拡張・上書きするためのローカルなスタイルガイドを維持することができます。良い例がChromium C++スタイルガイドです。これはGoogleのC++スタイルガイドをベースにしながら、独自の変更や追加を行っています。

レビューはすぐに始める

コードレビューは優先度の高いものとして扱ってください。実際にコードを読んでフィードバックを与える際には時間をかけてもかまいませんが、レビュー自体はすぐに始めてください。理想的には数分以内です。

コードレビューのリレー競争

チームメイトがあなたにチェンジリストを送ってきた場合、あなたのレビューが完了するまで彼らは他の作業を進められない状態である可能性が高いです。理論上は、ソース管理システムを使えば作成者はブランチを切り、作業を続け、レビューからの変更を新しいブランチにフォワードマージできます。しかし現実には、それを効率的にこなせる開発者はほんの数人しかいません。他の大多数にとっては、三方向のdiffを解きほぐすのにあまりにも時間がかかり、レビューを待つ間に進めた作業が帳消しになってしまうこともあります。

レビューをすぐに始めると、好循環が生まれます。あなたのレビューのターンアラウンドは、作成者のチェンジリストのサイズと複雑さだけに依存するようになります。これは作成者が小さくスコープの狭いチェンジリストを送るインセンティブになります。そうしたチェンジリストはあなたにとってもレビューしやすく快適なので、より速くレビューでき、循環は続いていきます。

チームメイトが1,000行のコード変更を要する新機能を実装すると想像してください。あなたが200行のチェンジリストを約2時間でレビューできると分かっていれば、彼らは機能を200行ずつ程度のチェンジリストに分割し、1〜2日で全体をチェックインできます。しかし、サイズにかかわらずすべてのコードレビューに1日かかるのであれば、その機能のチェックインには1週間かかることになります。チームメイトは1週間も待っていたくはないので、500〜600行といったより大きなコードレビューを送るインセンティブが働きます。こうしたレビューはコストが高く、600行の変更では200行の変更よりもコンテキストを保つのが難しいため、フィードバックの質も低下します。

1回のレビューラウンドのターンアラウンドは、最大でも1営業日とすべきです。より優先度の高い問題に追われていて1日以内にレビューを完了できない場合は、チームメイトにその旨を伝え、他の誰かに再アサインする機会を与えてください。月に1回以上レビューを断らざるを得ない状況であれば、チームは健全な開発プラクティスを維持できるようにペースを落とす必要がある可能性が高いです。

高い抽象度から始めて徐々に詳細へ

1回のレビューラウンドで書く指摘が多ければ多いほど、作成者を圧倒してしまうリスクが高まります。正確な限界は開発者によって異なりますが、一般的に1ラウンドあたり20〜50件あたりから危険域に入ります。

作成者を大量の指摘で溺れさせてしまうのが心配なら、初期のラウンドでは高レベルのフィードバックに絞ってください。クラスのインターフェースの再設計や複雑な関数の分割といった問題に焦点を当てます。そうした問題が解決されるまで、変数名やコードコメントの明確さといった低レベルの問題に取り組むのは待ちましょう。

作成者があなたの高レベルの指摘を取り入れれば、低レベルの指摘は無意味になるかもしれません。それらを後のラウンドに先送りすることで、問題を指摘するために慎重に言葉を選んだコメントを書くという決して少なくない作業を省け、作成者にとっても不要な指摘を処理する手間を省けます。このテクニックは、レビュー中に注目する抽象度のレイヤーを分割することにもなり、あなたと作成者がチェンジリストを明確かつ体系的に進めていくのに役立ちます。

コード例は惜しみなく示す

理想の世界では、コードの作成者は受けるすべてのレビューに感謝するでしょう。それは学ぶ機会であり、ミスから身を守ってくれるからです。現実には、作成者がレビューを否定的に受け取り、指摘したあなたを恨む原因となり得る外的要因が数多くあります。締め切りのプレッシャーに追われていて、あなたの即時の承認印以外のものはすべて妨害に感じられるのかもしれません。あるいは一緒に仕事をしたことがあまりなく、あなたのフィードバックが善意に基づくものだと信頼されていないのかもしれません。

作成者にレビュープロセスを好意的に感じてもらう素晴らしい方法は、レビュー中に贈り物を渡す機会を見つけることです。では、すべての開発者がもらって嬉しい贈り物とは何でしょうか? もちろんコード例です。

コードという贈り物を受け取る

あなたが提案する変更の一部を実際に書き出すことで作成者の負担を軽くすれば、レビュアーとして自分の時間を惜しみなく使っていることを示せます。

例えば、Pythonのリスト内包表記という機能に馴染みのない同僚がいると想像してください。彼らが次のような行を含むコードレビューを送ってきました。

urls = []
for path in paths:
  url = 'https://'
  url += domain
  url += path
  urls.append(url)

「リスト内包表記でシンプルにできませんか?」と返すと、彼らをいら立たせることになります。なぜなら、使ったことのないものについて20分かけて調べなければならなくなるからです。

次のようなコメントを受け取った方が、彼らははるかに喜ぶでしょう。

次のようにリスト内包表記でシンプルにすることを検討してみてください。

urls = ['https://' + domain + path for path in paths]

このテクニックは1行の例に限りません。私はしばしば自分でコードのブランチを作成し、大きな関数を分割したり追加のエッジケースをカバーする単体テストを追加したりといった、大規模な概念実証を作成者に示すことがあります。

このテクニックは、明確で異論のない改善のために取っておきましょう。上記のリスト内包表記の例では、コード行数が83%削減されることに異議を唱える開発者はほとんどいないでしょう。対照的に、あなた自身の好みに基づいて「より良い」とする変更(例えばスタイルの変更)を示すために長々とした例を書けば、コード例は気前が良いどころか押し付けがましく見えてしまいます。

1回のレビューラウンドあたり、コード例は2〜3件に抑えましょう。作成者のチェンジリスト全体をあなたが書き始めてしまえば、彼らには自分でコードを書く能力がないと思っているというシグナルを送ることになります。

「you」は使わない

これは奇妙に聞こえるかもしれませんが、最後まで聞いてください。コードレビューでは決して「you」という言葉を使わないでください。

レビューで下す決定は、誰がアイデアを出したかではなく、何がコードをより良くするかに基づくべきです。チームメイトはチェンジリストに多大な労力を注ぎ、自分の仕事に誇りを持っているはずです。自分の仕事への批判を聞いたときの自然な反応は、防御的になり、保護しようとすることです。

チームメイトの防御本能を刺激するリスクを最小限に抑えるようにフィードバックの言葉を選んでください。あなたが批判しているのはコードであってコーダーではないことを明確にしましょう。作成者がコメントの中に「you」を見つけると、注意がコードから離れて自分自身に向いてしまいます。そうなると、あなたの批判を個人的に受け取ってしまうリスクが高まります。

次の何気ないコメントを考えてみてください。

あなたは‘successfully’の綴りを間違えました。

作成者はその指摘をまったく異なる2通りに解釈できます。

  • 解釈1: やあ、相棒! ‘successfully’の綴りを間違えているよ。でも君が賢いことに変わりはない! きっと単なるタイプミスだよね。
  • 解釈2: ‘successfully’の綴りを間違えてるぞ、間抜け。

これを「you」を省いた指摘と比べてみてください。

sucessfully -> successfully

後者の指摘は単なる修正であり、作成者に対する評価ではありません。

幸いなことに、フィードバックを書き換えて「you」を避けるのは簡単です。

選択肢1:「you」を「we」に置き換える

この変数をより説明的な名前、例えばseconds_remainingのような名前に変更してもらえますか?

は次のようになります。

この変数をより説明的な名前、例えばseconds_remainingのような名前に変更できませんか?

「we」を使うことで、コードに対するチームの共同責任が強調されます。作成者は別の会社に移るかもしれませんし、あなたもそうかもしれませんが、このコードを所有するチームは何らかの形で残り続けます。明らかに作成者自身が行うことを期待しているのに「we」と言うのは馬鹿げて聞こえるかもしれませんが、馬鹿げている方が非難めいているよりはましです。

ソファを運ぶ漫画

選択肢2:文から主語を取り除く

「you」を避ける別の方法は、文から主語を省略した略式の表現を使うことです。

より説明的な名前、例えばseconds_remainingのような名前への変更を提案します。

受動態を使っても同様の効果を得られます。私はテクニカルライティングでは普段、受動態を疫病のように避けていますが、「you」を回避するための有用な書き方になり得ます。

この変数はより説明的な名前、例えばseconds_remainingのような名前に変更されるべきです。

さらに別の選択肢として、「what about…」や「how about…」で始まる疑問文の形にすることもできます。

この変数をより説明的な名前、例えばseconds_remainingのような名前に変更するのはどうでしょうか?

フィードバックは命令ではなく依頼として伝える

コードレビューは、議論が個人攻撃へと脱線するリスクが高いため、通常のコミュニケーション以上に機転と配慮を要します。レビューではレビュアーがより丁寧になることを期待するでしょうが、奇妙なことに私は逆の方向に進むのを目にしてきました。たいていの人は同僚に向かって「そのホチキスを取って、ついでにソーダを買ってきて」などとは言いません。しかし、「このクラスを別ファイルに移動しろ」といった同様に高圧的な命令の形でフィードバックを書くレビュアーを、私は何人も見てきました。

フィードバックにおいては、うっとうしいほど優しくするくらいがちょうどいいと考えてください。指摘は命令ではなく、依頼や提案として組み立てましょう。

同じ指摘を2つの異なる形で表現したものを比べてみてください。

命令として表現されたフィードバック依頼として表現されたフィードバック
Fooクラスを別ファイルに移動してください。Fooクラスを別ファイルに移動できませんか?

人は自分の仕事をコントロールできていると感じたいものです。作成者に依頼をすることで、彼らに自律性の感覚を与えることができます。

依頼は、作成者が丁寧に反論しやすくするという利点もあります。彼らの選択にはもっともな理由があるのかもしれません。フィードバックを命令として書けば、作成者からの反論は反抗のように聞こえてしまいます。フィードバックを依頼や質問として書けば、作成者は単にあなたに答えればよいのです。

レビュアーが最初の指摘をどのように表現するかで、会話がどれだけ攻撃的に見えるかを比べてみてください。

命令として表現されたフィードバック(攻撃的)依頼として表現されたフィードバック(協力的)
レビュアー: Fooクラスを別ファイルに移動してください。
作成者: それはしたくありません。そうするとBarクラスから離れてしまいます。クライアントはほぼ常にこの2つを一緒に使うことになります。
レビュアー: Fooクラスを別ファイルに移動できませんか?
作成者: 移動はできますが、そうするとBarクラスから離れてしまい、クライアントは一般的にこの2つのクラスを一緒に使うことになります。どう思いますか?

指摘を命令ではなく依頼として書くと、会話がどれだけ礼儀正しくなるか、自分の主張を証明するために架空の対話を作り上げるとお分かりでしょう。

指摘は意見ではなく原則に結びつける

作成者に指摘をするときは、提案する変更とその理由の両方を説明してください。「このクラスは2つに分割すべきです」と言う代わりに、「現在、このクラスはファイルのダウンロードとパースの両方を担っています。単一責任の原則に従い、ダウンローダークラスとパースクラスに分割すべきです」と言う方が良いでしょう。

指摘を原則に基づいて行うことで、議論を建設的な形で組み立てることができます。「クラスの公開インターフェースを最小化するために、この関数はprivateにすべきです」といった具体的な理由を挙げれば、作成者は単に「いや、私は自分のやり方が好きなんです」とは返せません。いや、返すことはできますが、あなたが変更がどのように目標を満たすかを示したのに対し、彼らは単に好みを述べただけなので、馬鹿げて見えるでしょう。

ソフトウェア開発は芸術でもあり科学でもあります。コードの何が悪いのかを確立された原則の観点から常に正確に言語化できるわけではありません。コードが単に見苦しかったり直感的でなかったりして、なぜそうなのかを特定するのが難しいこともあります。そうした場合でも、説明できることは説明しますが、客観性を保ってください。「私はこれを理解するのが難しいと感じました」と言えば、少なくとも客観的な陳述になります。一方、「これは分かりにくい」というのは価値判断であり、すべての人にとって真実とは限りません。

可能な限り、リンクの形で裏付けとなる証拠を示してください。チームのスタイルガイドの該当箇所へのリンクが最も良い証拠となります。言語やライブラリのドキュメントにリンクすることもできます。高く評価されたStackOverflowの回答も有効ですが、権威あるドキュメントから離れれば離れるほど、証拠としての信頼性は弱まります。

後編

この記事を気に入っていただけたなら、レビューを醜い衝突なく成功裏に終わらせることに焦点を当てた本記事の後編もぜひご覧ください。そこでは以下のテクニックを取り上げています。

  • 巨大すぎるコードレビューへの対処、
  • 褒める機会を見極める、
  • レビューのスコープを尊重する、そして
  • 行き詰まりを緩和する。

人間らしくコードレビューを行う方法(後編)


編集:Samantha Mason。イラスト:Loraine Yow。@global4gに、この記事の初期ドラフトに貴重なフィードバックをくれたことに感謝します。

この記事は「muse-spark-1.2-contributor」を使用して翻訳されました。

コメント