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

Michael Lynch

人間らしくコードレビューする方法(前編)

最近、コードレビューのベストプラクティスに関する記事をいくつか読んでいます。気づいたのは、そうした記事がレビューのほぼすべての要素を犠牲にして、バグ探しだけに焦点を当てていることです。見つけた問題を建設的かつプロフェッショナルな方法で伝えること?どうでもいい!とにかくバグをすべて見つければ、あとは自然とうまくいくだろう、というわけです。

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

電子書籍の表紙

この画期的な電子書籍では、パートナーの欠点を最大限に見つけ出すための実証済みのテクニックを伝授します。本書では、以下の内容は扱いません。

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

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

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

では、なぜコードレビューについてはそのように語られるのでしょうか。

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

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

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

コードレビューとは何か

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

コードレビューの流れ

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

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

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

レビューは、レビュアーが変更を承認(approve)したときに終了します。これは一般的にLGTM(Looks Good To Me=問題なし)と呼ばれます。

なぜ難しいのか

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

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

- Philip Greenspun、ArsDigita共同創業者、『Founders at Work』より抜粋

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

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

テクニック

  1. 退屈な作業はコンピューターに任せる
  2. スタイル論争はスタイルガイドで決着させる
  3. すぐにレビューを始める
  4. 高い視点から始めて徐々に詳細へ
  5. コード例は惜しみなく示す
  6. 「you(あなた)」と言わない
  7. フィードバックは依頼として伝え、命令にしない
  8. 指摘は意見ではなく原則に結びつける

退屈な作業はコンピューターに任せる

会議やメールなどの中断の合間で、コードに集中できる時間は限られています。精神的なスタミナはさらに貴重です。チームメイトのコードを読む作業は認知的負荷が高く、高度な集中力を要します。コンピューターにできる作業に、こうした貴重なリソースを浪費しないでください。しかもコンピューターは、その作業をもっと上手にこなせます。

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

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

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

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

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

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

自動化は作成者にとってもメリットがあります。些細なミスを数時間後ではなく数秒で発見できるようになります。即時のフィードバックは学習しやすく、修正コストも安くなります。関連する文脈を作成者がまだ覚えているからです。さらに、自分のくだらないミスについて指摘されるとしても、あなたではなくコンピューターから聞く方が、プライドははるかに傷つきません。

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

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

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

典型的なスタイル論争

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

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

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

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

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

選択肢2:自分のスタイルガイドを少しずつ作る

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

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

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

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

すぐにレビューを始める

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

コードレビューのリレーレース

チームメイトがチェンジリストを送ってきた場合、おそらくあなたのレビューが終わるまで他の作業がブロックされている状態です。理論上は、ソース管理システムによって作成者はブランチを切り、作業を続けてからレビューでの変更を新しいブランチにフォワードマージできます。しかし実際には、それを効率的にできる開発者は4人程度しかいません。他の全員は、三方向の差分を解くのに非常に時間がかかり、レビューを待つ間に進めた作業が帳消しになってしまいます。

すぐにレビューを始めると、好循環が生まれます。レビューにかかる時間は、作成者のチェンジリストのサイズと複雑さだけで決まるようになります。これにより、作成者は小さく範囲を絞ったチェンジリストを送る動機づけが生まれます。それらはあなたにとってレビューしやすく快適なものなので、より速くレビューでき、 cycleは続きます。

チームメイトが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]

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

このテクニックは、明確で異論の余地のない改善にのみ使ってください。上のリスト内包表記の例では、コード行数が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氏に感謝します。

原文は Michael Lynch により に公開されました。

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