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

2017-11-09

Michael Lynch

人間らしくコードレビューする方法（後編）

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

これは、コードレビューでうまくコミュニケーションを取り、落とし穴を避ける方法についての記事の後半です。ここでは、醜い衝突を避けつつ、コードレビューをうまく収束させるためのテクニックに焦点を当てます。

土台となる考え方は前編で解説したので、まずはそちらから読むことをおすすめします。急いでいる方のために要約すると、優れたコードレビュアーはバグを見つけるだけでなく、チームメイトの成長を助ける思慮深いフィードバックを提供します。

最悪のコードレビュー

私の人生で最悪のコードレビューは、仮にマロリーと呼ぶことにした、ある元チームメイトのものでした。彼女は私が入社する数年前からその会社にいましたが、最近私のチームに異動してきたばかりでした。

レビュー

マロリーがレビューのために最初のチェンジリストを送ってきたとき、コードは少々粗削りでした。彼女はそれまでPythonを書いたことがなく、しかも私がメンテナンスしていた扱いづらいレガシーシステムの上に構築していたのです。

私は見つけた問題を忠実にすべて記録しました。全部で59件です。当時読んでいたレビューに関する文献によれば、私は素晴らしい仕事をしたことになります。なんとたくさんのミスを見つけたのだから。つまり、優れたレビュアーに違いない、と。

数日後、マロリーは更新されたチェンジリストと、私の指摘への返答を送ってきました。タイポや変数名の変更といった簡単な問題は修正されていました。しかし、不正な入力に対して未定義の挙動を示すことや、ある関数で制御フローが6層にもネストしているといった、より高レベルな問題には対応しようとしませんでした。代わりに、そうした問題を修正するのは工数に見合わないと、素っ気なく説明するだけでした。

腹を立て、苛立った私は、新たな指摘を送り返しました。口調はプロフェッショナルではありましたが、徐々に皮肉めいたものになりつつありました。「不正な入力に対して未定義の挙動をなぜ許容したいのか、説明してもらえますか？」といった具合です。想像がつくと思いますが、マロリーの返答はさらに頑ななものになりました。

苦いサイクル

それは火曜日、一週間後のことでした。マロリーと私は、同じレビューでいまだに行ったり来たりを続けていました。前の晩に最新の指摘を送っていたのですが、彼女が帰宅するまでわざと送信を控えました。彼女がそれを読む瞬間に同じ部屋にいたくなかったからです。

午前中ずっと、胃の底に重く沈むような嫌な予感を抱え、次のレビューのやり取りを恐れていました。昼食から戻ると、マロリーは席を外していましたが、新しい変更を送ってきていました。彼女もまた、私が返答を読むところを見られたくなかったのでしょう。

彼女の返答を一つ読むごとに怒りがこみ上げ、胸の鼓動が早くなっていきました。私はすぐにキーボードを叩き、反論を書き始めました。彼女が私の提案した変更を反映していないことや、承認すべき理由を示していないことを指摘しながら。

私たちはこのやり取りを毎日、3週間にわたって繰り返しました。コードはほとんど変わりませんでした。

介入

このサイクルを断ち切ってくれたのは、最も経験豊富なチームメイトであるボブでした。彼は長期休暇から戻ると、私たちが険悪な雰囲気でコードレビューのコメントを投げ合っているのを見て驚きました。彼はすぐに、それが膠着状態であることを見抜きました。そして、レビューを引き継ぎたいと申し出、私たちは二人とも同意しました。

ボブはレビューを始めるにあたり、マロリーに新しいチェンジリストを作成するよう頼みました。これまでほとんど争いにならなかった2つの小さなライブラリを、それぞれ30〜50行程度で切り出すように、という依頼でした。マロリーがその通りにすると、ボブは即座にそれらを承認しました。

その後、ボブは約200行までスリムになったメインのチェンジリストに戻りました。そこでいくつか小さな提案をし、マロリーはそれらに対応しました。そして、ボブはそのチェンジリストを承認したのです。

ボブのレビュー全体は、わずか2日で完了しました。

コミュニケーションが重要

お気づきかもしれませんが、この衝突は本質的にはコードの問題ではありませんでした。コードには確かに正当な問題がありましたが、それは円滑にコミュニケーションが取れるチームメイト同士であれば、明らかに解決可能なものでした。

それは不快な経験でしたが、振り返ってみれば得るものがあったと感じています。この経験がきっかけで、私はレビューへのアプローチを見直し、改善すべき点を見つけることができたからです。

以下では、同様の望ましくない結果になるリスクを減らすためのテクニックを紹介します。後ほどマロリーの件に戻り、なぜ私の本来のアプローチが的外れで、ボブのアプローチが静かに見事だったのかを説明します。

テクニック

コードを1〜2段階グレードアップさせることを目指す

チームメイトは理論上、コードを改善するあらゆる機会を歓迎したいと思っているかもしれませんが、その忍耐力には限りがあります。あなたが次々と新しい素晴らしい磨き方を思いついて承認を保留し続ければ、相手はすぐに苛立ちを覚えるでしょう。

私は内心でコードをAからFまでの成績で評価しています。D評価から始まるチェンジリストを受け取ったときは、それをCやB-まで引き上げる手助けをすることを目指します。完璧ではありませんが、十分に良いレベルです。

理論上は、DをA+まで引き上げることも可能です。しかし、おそらく8回以上のレビューが必要になるでしょう。終わる頃には、相手はあなたのことを嫌い、二度とあなたにコードを送りたくないと思うようになるはずです。

「C評価のコードを受け入れれば、結局コードベース全体がC評価になってしまうのでは？」と思うかもしれません。しかし幸いなことに、そうはなりません。私の経験では、チームメイトがDからCへ成長する手助けをすると、次に送られてくるチェンジリストは最初からC評価で始まります。数ヶ月のうちに、彼らが送ってくるレビューはB評価から始まり、レビューを終える頃にはA評価になるようになります。

Fは、機能的に誤っているか、あるいはあまりにも複雑で正しさに自信が持てないコードのために取っておきます。数回のレビューを経てもコードがFのままである場合にのみ、承認を保留すべきです。詳しくは下の膠着状態への対処のセクションを参照してください。

繰り返されるパターンへのフィードバックは絞る

作者のミスのいくつかが同じパターンに当てはまることに気づいたら、すべての箇所をいちいち指摘しないでください。同じコメントを25回書くのに時間を費やしたい人はいませんし、作者も25件の重複した指摘を読みたいとは思っていません。

パターンの例として2、3件を指摘するのは構いません。それ以上になる場合は、個々の箇所ではなく、パターン自体を修正するよう作者に依頼してください。

レビューのスコープを尊重する

レビュアーがチェンジリスト内のコードの近くにある何かを見つけて、作者に修正を求めるというアンチパターンがよく見られます。作者がそれに従うと、レビュアーは通常、コードは良くなったものの整合性が取れていないことに気づき、さらにいくつかの軽微な修正が必要だと感じます。そして、さらにもう少し、と繰り返すうちに、当初はスコープが狭かったチェンジリストが、無関係な変更を大量に含むものへと膨れ上がってしまいます。

お腹を空かせた小さなねずみがあなたの家の玄関に現れたら、あなたはクッキーをあげたくなるかもしれません。そしてクッキーをあげたら、彼は牛乳を求めるでしょう。牛乳で口ひげができていないか鏡を見たくなり、次に髪を切るためのハサミを求めるでしょう……
-Laura Joffe Numeroff, If You Give a Mouse a Cookie

経験則として、チェンジリストが触れていない行はスコープ外です。

例えば次のようなケースです。

たとえコードベースに残るマジックナンバーやひどい変数名に悩まされ、一晩中眠れなくなるとしても、それはスコープ外です。たとえ作者がその近くの行を書いた本人であったとしても、やはりスコープ外です。もしそれがあまりにもひどい場合は、バグを登録するか自分で修正を送ってください。ただし、このレビューで作者に押し付けないでください。

例外は、チェンジリストが実際にはその行に触れていなくても、周囲のコードに影響を与える場合です。例えば次のようなケースです。

この場合は、作者が関数名をValidateAndSerializeから単なるSerializeに変更する必要があることを指摘してください。彼らは関数シグネチャを含む行自体には触れていませんが、それでもその行を不正確なものにしてしまったのです。

自分からの指摘が少なく、スコープ外で簡単に修正できる箇所に気づいた場合は、私はこのルールを緩めに適用します。その際は、作者がその指摘を無視しても構わないことを明確に伝えます。

大きなレビューを分割する機会を探す

400行を超えるチェンジリストを受け取った場合は、作者にそれを小さな単位に分割するよう促してください。この上限を大きく超えるほど、より強く分割を求めてください。私は個人的に、1,000行を超えるチェンジリストのレビューは断っています。

作者は、チェンジリストの分割が面倒な作業であるため、不満を漏らすかもしれません。分割の論理的な境界を特定してあげることで、負担を軽くしてあげましょう。最も簡単なケースは、チェンジリストが複数のファイルに独立して触れている場合です。その場合は、ファイルを小さなグループに分けてチェンジリストを分割すればよいだけです。より難しいケースでは、最も低い抽象度にある関数やクラスを見つけてください。それらを別のチェンジリストに移すよう作者に依頼し、最初のチェンジリストがマージされた後に残りのコードに戻るようにします。

コードの品質が低い場合は、強く分割を要請してください。質の低いコードをレビューする難しさは、サイズに比例して指数関数的に増大します。600行のひどいコードを1つレビューするよりも、雑ではあっても300行のチェンジリストを2つ監査する方がはるかにましです。

心からの称賛を贈る

ほとんどのレビュアーはコードの悪い点だけに注目しますが、レビューは良い行動を強化する貴重な機会でもあります。

例えば、ドキュメントを書くのが苦手な作者のレビューをしていて、明確で簡潔な関数のコメントを見つけたとしましょう。その出来栄えを伝えてあげてください。失敗したときだけ指摘するのではなく、うまくできたときに伝えることで、相手はより早く成長します。

称賛を贈るのに、特別な目的は必要ありません。チェンジリストの中で嬉しくなるようなものを見つけたら、私はその都度作者に伝えています。

「このAPIは知りませんでした。とても便利ですね！」
「これはエレガントな解決策ですね。自分では思いつきませんでした。」
「この関数を分割したのは素晴らしいアイデアですね。ずっとシンプルになりました。」

作者がジュニアの開発者だったり、最近チームに加わったばかりだったりする場合、レビュー中に緊張したり身構えたりしがちです。心からの褒め言葉は、あなたが意地悪な門番ではなく、支えてくれるチームメイトであることを示すことで、その緊張を和らげます。

残りの修正が些細なものであれば承認する

一部のレビュアーは、最後の1件の指摘まで修正を確認するまで承認を保留すべきだと誤解しています。これは不要なレビューのラウンドを増やし、作者とレビュアーの双方の時間を浪費します。

次のいずれかに当てはまる場合は、承認を与えてください。

指摘事項がもうない。
残りの指摘が些細な問題である。
- 例：変数名の変更、タイポの修正
残りの指摘が任意の提案である。
- 承認がそれらの対応を条件とするものではないことが分かるよう、任意であることを明示してください。

コードコメントの末尾にピリオドを付け忘れたという理由で承認を保留するレビュアーを見たことがあります。どうかそのようなことはしないでください。それは、監視しなければ単純な句読点さえ付けられないと相手を思っている、というメッセージを作者に与えてしまいます。

まだ未解決の指摘がある状態で承認を与えることには、ある程度のリスクがあります。私の見積もりでは、約5%の確率で、作者が最終ラウンドの指摘を誤解したり、完全に見落としたりします。これを防ぐために、私は承認後の作者の変更を単に確認するようにしています。稀に行き違いがあった場合は、作者にフォローアップするか、自分で修正用のチェンジリストを作成します。5%のケースで少し手間が増えることは、残りの95%に不要な手間と遅延を加えるよりましです。

膠着状態には先手を打って対処する

コードレビューにおける最悪の結果は、膠着状態です。あなたはさらなる変更なしにチェンジリストを承認することを拒否する一方で、作者はそれらの変更を拒否する状態です。

膠着状態に向かっていることを示すサインには、次のようなものがあります。

議論のトーンが緊迫したり、敵対的になってきている。
1ラウンドあたりの指摘数が減少傾向にない。
異常に多くの指摘に対して反発を受けている。

直接話す

対面かビデオチャットで会って話してください。テキストでのコミュニケーションは、会話の向こう側に生身の人間がいることを忘れさせがちです。チームメイトが頑固さや無能さから発言しているのだと想像するのはあまりにも簡単です。ミーティングは、あなたと作者の双方にとって、その思い込みを解いてくれます。

設計レビューを検討する

議論を呼ぶコードレビューは、プロセスのより早い段階での弱点を示しているかもしれません。あなたが議論していることは、設計レビューでカバーされるべきことではなかったでしょうか？そもそも設計レビューは行われたのでしょうか？

意見の相違の根本が高レベルの設計上の選択に遡るのであれば、たまたまコードレビューに参加している2人に任せるのではなく、より広いチームが意見を述べるべきです。設計レビューという形で、チーム全体に議論を開くことについて作者と話し合ってみてください。

譲歩するか、エスカレーションするか

あなたとチームメイトが膠着状態で煮詰まれば煮詰まるほど、関係へのダメージは大きくなります。別の手段を試しても行き詰まりが解消しない場合、選択肢は譲歩するかエスカレーションするかのどちらかです。

ただ変更を承認することのコストをよく考えてみてください。低品質なコードを安易に受け入れていては質の高いソフトウェアは作れませんが、一方で、あなたとチームメイトが激しく争って二度と一緒に仕事ができないほどになっても、高い品質は達成できません。もしチェンジリストを承認したら、実際どれほど悪いことになるでしょうか？それは重要なデータを破壊しかねないコードでしょうか？それとも、せいぜいジョブが失敗して開発者がデバッグすれば済むようなバックグラウンドプロセスでしょうか？後者に近いのであれば、チームメイトと良好な関係を保ち続けるためにも、単に譲歩することを検討してください。

譲歩が選択肢にならない場合は、議論をチームのマネージャーやテックリードにエスカレーションすることについて作者と話し合ってください。別のレビュアーに再アサインすることを提案してもよいでしょう。エスカレーションの結果が自分に不利なものだったとしても、その決定を受け入れて先に進んでください。戦い続ければ、状況を長引かせるだけでなく、あなたのプロ意識を疑われることになります。

膠着状態からの立ち直り

こじれたレビューでの口論は、コードそのものよりも、関わった人々の間の関係に起因することが多いものです。膠着状態やそれに近い状態に陥った場合、根底にある対立に対処しなければ、このパターンは繰り返されます。

状況についてマネージャーと話し合う。
- チーム内に衝突があるなら、マネージャーはそれを知っておくべきです。もしかすると作者が単に扱いにくい人なのかもしれません。あるいは、あなた自身が気づかない形で状況に拍車をかけているのかもしれません。良いマネージャーなら、二人がこれらの問題に対処するのを助けてくれます。
お互いに距離を置く。
- 可能であれば、事態が落ち着くまで数週間はお互いにコードレビューを送るのを避けてください。
衝突解決について学ぶ。
- 私はCrucial Conversationsという本が役に立ちました。その助言は当たり前に聞こえるかもしれませんが、口論の最中ではないときに自分の衝突へのアプローチを分析することには大きな価値があります。

最悪のコードレビュー：再訪

マロリーとのコードレビューを覚えているでしょうか？なぜ私のレビューは3週間にわたる皮肉まみれの泥沼になったのに、ボブのレビューは2日で軽やかに終わったのでしょうか？

私が間違ったこと

これはマロリーにとってチームでの最初のレビューでした。私は彼女が裁かれていると感じたり、身構えたりするかもしれないということに配慮できませんでした。大量の指摘に不意打ちされたと感じさせないよう、まずは高レベルのコメントだけから始めるべきでした。

私の仕事は彼女の作業を妨げることではなく、前進を助けることなのだと示すために、もっと手を尽くすべきでした。コード例を示したり、彼女のチェンジリストの良い点を指摘したりできたはずです。

私は自分のエゴをレビューに持ち込んでしまいました。私はこの古いシステムを立て直すために1年間を費やしてきました。そこに突然現れた新参者がシステムをいじり回し、私の懸念を真剣に取り合おうともしない。それを侮辱と受け取りましたが、その態度は逆効果でした。私がすべてのレビューで心がけている客観的な姿勢を保つべきでした。

最後に、私は膠着状態を長引きさせすぎました。数ラウンドを経た時点で、私たちが有意義な進展をしていないことは明らかだったはずです。より深い対立に対処するために直接会って話すとか、マネージャーにエスカレーションするといった抜本的な対策を講じるべきでした。

ボブが正しく行ったこと

ボブの最初の動きであるレビューの分割は非常に効果的でした。3週間もの間、苦痛を伴いながら停滞していたレビューを思い出してください。突然、2つのコードがマージされたのです。これは前向きな勢いを生み出したため、マロリーとボブの両方にとって良い気分をもたらしました。残りの部分にはまだ問題がありましたが、より小さく、管理しやすいチェンジリストになったのです。

ボブはレビューを完璧になるまで締め上げようとはしませんでした。彼はおそらく、私が声高に叫んでいたのと同じ問題に気づいていたでしょう。しかし、マロリーが今後もチームに在籍することを理解していました。短期的な柔軟性が、長期的にマロリーの品質向上を助けるための布石となったのです。

結論

この記事の前半を公開した後、私が推奨したコミュニケーションスタイルに対して、何人かの読者から異議が寄せられました。見下したように感じたという人もいれば、あまりにも遠回しで誤解を招く恐れがあると心配する人もいました。

そうしたフィードバックはもっともであり、想定されたことでもあります。ある人にとっては素っ気ないレビューコメントが無愛想で失礼に感じられる一方で、別の人にとっては同じコメントが簡潔で効率的だと評価されることもあります。

コードをレビューする際には、何に焦点を当てるか、フィードバックをどう伝えるか、いつ承認するかなど、多くの選択を迫られます。重要なのは私の選択肢を選ぶことではありません。選択肢が存在するということを認識することです。

誰も完璧なレビューのレシピを手渡してはくれません。最も効果的なテクニックは、コードの作者の性格、あなたとその人との関係、そしてチームの文化によって異なります。コードレビューの結果について批判的に考えることで、あなたのアプローチを磨いてください。緊張に直面したときは、一歩引いてなぜそれが起きたのかを考えてみましょう。自分のレビューの質に目を向けてください。もしコードを自分の品質基準まで引き上げられないと感じるなら、レビュープロセスのどの側面が妨げになっているのか、そしてそれにどう対処できるかを考えてみてください。

幸運を祈ります。そして、あなたのコードレビューが人間らしいものでありますように。

参考文献

“How to Make your Code Reviewer Fall in Love with You”は本記事の姉妹編です。レビュアーではなく作者の立場から、コードレビューを改善する方法について解説しています。
PuTTY SSHクライアントの作者であるSimon Tathamによる“Code review antipatterns”では、レビュアーとして避けるべき落とし穴が役立つリストとしてまとめられています。

本記事はSamantha Masonが編集を担当しました。イラストはLoraine Yowによるものです。本稿の初期ドラフトに貴重なフィードバックを提供してくれた@global4gに感謝します。