人間らしくコードレビューを行う方法（後編）
これは、コードレビューでうまくコミュニケーションを取り、落とし穴を避ける方法についての記事の後編です。ここでは、醜い衝突を避けつつ、コードレビューを成功裏にまとめるためのテクニックに焦点を当てます。
前編で基礎を解説していますので、まずはそちらから読むことをおすすめします。お急ぎの方のために要約すると、優れたコードレビュアーはバグを見つけるだけでなく、チームメイトの成長を助ける思慮深いフィードバックを提供します。
私の最悪のコードレビュー
私の人生で最悪のコードレビューは、Mallory（仮名）と呼ぶことにした、以前のチームメイトとのものでした。彼女は私が入社する何年も前から会社にいましたが、最近私のチームに異動してきたばかりでした。
レビュー
Malloryから初めてレビュー依頼となるチェンジリストが届いたとき、コードは少々粗削りでした。彼女はPythonを書いたことがなく、私が保守していた扱いにくいレガシーシステムの上に構築していたのです。
私は見つけた問題をすべて律儀に記録しました。全部で59件です。当時読んでいたレビュー関連の文献によれば、私は素晴らしい仕事をしたことになります。こんなにも多くのミスを見つけたのだから、きっと優秀なレビュアーに違いない、と。
数日後、Malloryから更新されたチェンジリストと、私の指摘への返信が届きました。タイポや変数名の変更といった単純な問題は修正されていましたが、より本質的な問題、例えば不正な入力に対してコードが未定義の挙動を示すことや、ある関数で制御構造が6層にもネストしていることなどには手をつけていませんでした。代わりに、それらの問題は修正にかける工数に見合わないと、あっさり片付けられてしまったのです。
怒りとフラストレーションを覚えた私は、新たな指摘を送り返しました。口調はプロフェッショナルではありましたが、皮肉めいた響きが混じり始めていました。「不正な入力に対して未定義の挙動をさせておきたい理由を説明していただけますか？」ご想像のとおり、Malloryの返信はさらに頑ななものになりました。
苦いサイクル
一週間後の火曜日のことでした。Malloryと私は同じレビューでいまだに行ったり来たりを続けていました。前夜、私は彼女が退社するまでわざと指摘を送らずに待っていました。同じ部屋で自分のコメントを読まれるのが嫌だったからです。
朝からずっと、胃のあたりに重く沈むような嫌な予感を抱え、次のラウンドを恐れていました。昼食から戻ると、Malloryは席を外していましたが、新たな変更を送ってきていました。きっと彼女も、私に返信を読まれる場にいたくなかったのでしょう。
彼女の返信を一つ読むごとに、胸の鼓動は速まり、怒りは増していきました。私はすぐにキーボードを叩き、反論を書き始めました。彼女が私の提案した変更をしておらず、承認できるだけの正当な理由も示していないことを指摘しながら。
このやり取りを毎日、3週間にわたって繰り返しました。コードはほとんど変わりませんでした。
介入
幸いなことに、最も経験豊富なチームメイトであるBobがこのサイクルを断ち切ってくれました。長い休暇から戻ってきた彼は、私たちが険悪な雰囲気でレビューコメントを投げ合っているのを見て驚き、事態が膠着状態であることをすぐに見抜きました。彼は自分がレビューを引き継ぎたいと申し出、私たちは二人とも同意しました。
BobはまずMalloryに、それまであまり争点になっていなかった小さなライブラリ2つを、それぞれ30〜50行ほどの新たなチェンジリストとして切り出すよう依頼しました。Malloryがそうすると、Bobは即座にそれらを承認しました。
次にBobは、約200行までスリムになったメインのチェンジリストに戻りました。いくつか小さな提案をし、Malloryはそれらに対応しました。そしてBobはそのチェンジリストを承認したのです。
Bobのレビューは、わずか2日で完了しました。
コミュニケーションが重要
お気づきかもしれませんが、この衝突は本質的にはコードの問題ではありませんでした。コードに正当な問題があったことは確かですが、それらは互いにうまくコミュニケーションが取れるチームメイトであれば、明らかに解決可能なものでした。
不快な経験でしたが、今振り返れば、あってよかったと思っています。この件がきっかけで自分のレビューへの向き合い方を見直し、改善すべき点を見つけられたからです。
以下では、同様に望ましくない結果になるリスクを減らすためのテクニックを紹介します。後ほどMalloryの件に戻り、なぜ私の当初のアプローチが根本的に間違っていて、Bobのアプローチがなぜ静かに優れていたのかを説明します。
テクニック集
- コードの成績を1〜2段階上げることを目指す
- 繰り返されるパターンへの指摘は絞る
- レビューのスコープを尊重する
- 大きなレビューは分割する機会を探す
- 心からの称賛を贈る
- 残りが些細な修正なら承認する
- 行き詰まりには能動的に対処する
コードの成績を1〜2段階上げることを目指す
チームメイトは理論上、コードを改善するあらゆる機会を歓迎したいと思っているかもしれませんが、その忍耐力には限りがあります。チェンジリストを磨くための素晴らしいアイデアを次々と思いついて、何度も承認を保留にすれば、相手はすぐにうんざりしてしまうでしょう。
私は個人的に、コードをAからFまでの成績で考えるようにしています。D評価から始まったチェンジリストを受け取ったときは、CやB-まで引き上げることを目指します。完璧ではありませんが、十分に良い状態です。
理論上はDをA+まで引き上げることも可能ですが、おそらく8回以上のレビューが必要になるでしょう。その頃には、相手はあなたのことを嫌い、二度とコードを送りたくないと思うようになっているはずです。
「C評価のコードを受け入れていたら、コードベース全体がC評価になってしまうのでは？」と思うかもしれません。幸い、そうはなりません。私の経験では、チームメイトのコードをDからCに引き上げる手助けをすると、次に送られてくるチェンジリストは最初からC評価で始まります。数か月もすれば、最初からB評価のレビューが届くようになり、レビューを終える頃にはAになっているのです。
Fは、機能的に誤っているか、あまりに複雑で正しさに自信が持てないコードのために取っておきます。数回のレビューを経てもコードがFのままである場合にのみ、承認を保留すべきです。詳しくは後述する行き詰まりのセクションをご覧ください。
繰り返されるパターンへの指摘は絞る
作者のミスのいくつかが同じパターンに当てはまることに気づいたら、すべての箇所を指摘するのはやめましょう。同じコメントを25回書くのに時間を費やしたくはないですし、作者も同じ内容の指摘を25回読みたいとは思っていません。
パターンのうち2、3件を取り上げて指摘するのは問題ありません。それ以上の場合は、個々の箇所ではなく、パターン自体を修正するよう依頼しましょう。
レビューのスコープを尊重する
レビュアーが、チェンジリスト内のコードの近くにあるものを見つけて、作者に修正を求めるというアンチパターンをよく見かけます。作者がそれに応じると、レビュアーはコードは良くなったものの、一貫性がなくなったことに気づき、さらにいくつかの軽微な修正が必要になります。そしてまた少し修正が必要になり……と、スコープの狭かったはずのチェンジリストが、やがて無関係な変更を大量に含むものへと膨らんでいくのです。
お腹を空かせた小さなねずみが玄関先に現れたら、クッキーをあげたくなるかもしれません。そしてクッキーをあげれば、彼はミルクを欲しがります。ミルクで口ひげができていないか鏡を見たくなり、次は髪を切るためのはさみをねだります……
-ローラ・ジョフィ・ヌメロフ、If You Give a Mouse a Cookie
経験則として、チェンジリストが触れていない行はスコープ外です。
例を挙げます。
たとえコードベースに残るマジックナンバーやひどい変数名のせいで一晩中眠れなくなったとしても、それはスコープ外です。たとえ作者がその周辺の行を書いた本人であったとしても、やはりスコープ外です。あまりにひどい場合は、バグチケットを起票するか、自分で修正を送ってください。このレビューで作者に押し付けないようにしましょう。
例外は、チェンジリストが実際には触れていなくても、周辺のコードに影響を与えている場合です。例えば次のようなケースです。
この場合は、作者が関数名をValidateAndSerializeからSerializeへリネームする必要があることを指摘しましょう。関数のシグネチャがある行自体には触れていなくても、その行を不正確にしてしまったのですから。
私自身、指摘があまり多くなく、スコープのすぐ外側に簡単に直せる箇所を見つけたときは、このルールを緩めに適用することがあります。その場合は、無視しても構わないことを明確に伝えます。
大きなレビューは分割する機会を探す
400行を超えるチェンジリストを受け取った場合は、作者に小さく分割するよう促しましょう。この上限を大きく超えるほど、強く分割を求めてください。私は個人的に、1,000行を超えるチェンジリストのレビューはお断りしています。
作者は分割が面倒だといって不満を言うかもしれません。その負担を軽くするために、論理的な分割の境界をこちらから示してあげましょう。最も簡単なのは、チェンジリストが複数のファイルを独立して触っている場合です。その場合は、単にファイルのまとまりごとに小さなチェンジリストへ分割できます。より難しいケースでは、最も抽象度の低いレイヤーにある関数やクラスを探します。それらを別のチェンジリストに移すよう作者に依頼し、最初のチェンジリストがマージされてから残りのコードに戻りましょう。
コードの品質が低いときは、なおさら強く分割を求めてください。質の低いコードをレビューする難易度は、サイズに比例して指数関数的に増大します。600行のひどい塊1つを監査するより、雑な300行のチェンジリスト2つを監査する方がずっとましです。
心からの称賛を贈る
ほとんどのレビュアーはコードの悪い点だけに注目しますが、レビューは良い行動を強化する貴重な機会でもあります。
例えば、ドキュメントを書くのが苦手な作者のレビューで、明確で簡潔な関数のコメントを見つけたとします。うまく書けていることを伝えてあげましょう。失敗したときに減点するだけでなく、うまくいったときに伝えることで、相手はより早く成長できます。
称賛するために特定の目的を持つ必要はありません。チェンジリストの中で心から素晴らしいと思ったことがあれば、いつでも作者に伝えています。
- 「このAPIは知りませんでした。とても便利ですね！」
- 「これはエレガントな解決策ですね。自分では思いつきませんでした。」
- 「この関数を分割したのは素晴らしいアイデアですね。ずっとシンプルになりました。」
作者がジュニアの開発者だったり、最近チームに加わったばかりだったりすると、レビュー中に緊張したり防衛的になったりしがちです。心からの褒め言葉は、あなたが厳しい門番ではなく、支えてくれるチームメイトであることを示し、その緊張を和らげてくれます。
残りが些細な修正なら承認する
残りの指摘がすべて修正されるのを確認するまで承認を保留すべきだ、と誤解しているレビュアーがいます。これでは不要なレビューのラウンドが増え、作者とレビュアーの双方の時間を無駄にします。
次のいずれかに当てはまる場合は、承認を与えましょう。
- これ以上指摘がない。
- 残りの指摘が些細な問題である。
- 例：変数名の変更、タイポの修正
- 残りの指摘が任意の提案である。
- これらは任意であることを明示し、承認がそれらにかかっているとチームメイトに誤解させないようにしましょう。
コードコメントの末尾にピリオドが抜けていたという理由で承認を保留するレビュアーを見たことがあります。どうかそうしないでください。監督なしでは句読点すら打てないと思われている、というメッセージを作者に与えてしまいます。
まだ未解決の指摘がある状態で承認することには、ある程度のリスクがあります。私の見積もりでは、約5%の確率で、作者が最終ラウンドの指摘を誤解したり、見落としたりします。これを防ぐために、私は承認後の作者の変更を簡単に確認しています。まれに齟齬があった場合は、作者にフォローアップするか、自分で修正用のチェンジリストを作成します。残り95%のケースに不要な手間と遅延を加えるより、5%のケースで少し手間が増える方がましです。
行き詰まりには能動的に対処する
コードレビューにおける最悪の結末は、行き詰まりです。あなたがさらなる変更なしにチェンジリストを承認することを拒否し、一方で作者も変更を拒否する状態です。
行き詰まりに向かっているサインには、次のようなものがあります。
- 議論の口調が緊迫したり、敵対的になってきている。
- レビューのラウンドごとの指摘数が減少傾向にない。
- 異常に多くの指摘に対して反発を受けている。
直接話し合う
直接会うか、ビデオチャットで話しましょう。テキストでのやり取りは、会話の相手が生身の人間であることを忘れさせがちです。相手が頑固さや無能さからそうしているのだと想像してしまいやすくなります。顔を合わせれば、あなたと作者の双方でその思い込みを解くことができます。
設計レビューを検討する
論争を呼ぶコードレビューは、プロセスのより早い段階での弱点を示しているかもしれません。そもそも設計レビューでカバーしておくべきことを議論していませんか？そもそも設計レビューは行われましたか？
意見の相違の根本がハイレベルな設計判断に遡るのであれば、コードレビューに参加している2人だけで抱え込まず、より広いチームに意見を求めるべきです。作者と話し合い、設計レビューという形で議論をチーム全体に開くことを検討しましょう。
譲歩するか、エスカレーションする
チームメイトと行き詰まったまま長く燻り続けるほど、関係へのダメージは大きくなります。別の手段を試しても膠着状態から抜け出せない場合、選択肢は譲歩するかエスカレーションするかのいずれかです。
ただ承認してしまうことのコストを天秤にかけてみましょう。低品質なコードを安易に受け入れていては良いソフトウェアは作れませんが、チームメイトと激しく対立して一緒に働けなくなってしまっては、高い品質も達成できません。チェンジリストを承認したら、実際どれほどひどいことになるでしょうか？重要なデータを破壊しかねないコードでしょうか？それとも最悪の場合でもジョブが失敗して開発者がデバッグすれば済むようなバックグラウンドプロセスでしょうか？後者に近いのであれば、チームメイトと良好な関係を保ち続けるためにも、単純に譲歩することを検討しましょう。
譲歩が選択肢にならない場合は、チームのマネージャーやテックリードへ議論をエスカレーションすることについて作者と話し合いましょう。別のレビュアーに再アサインすることを提案してもよいでしょう。エスカレーションの結果が自分に不利なものであったとしても、その決定を受け入れて次に進みましょう。戦い続ければ、悪い状況を長引かせ、あなたの印象を損ねるだけです。
行き詰まりからの立ち直り
泥沼化したレビューでの言い争いは、コードそのものよりも、関わった人同士の関係に起因することが多いものです。行き詰まりやそれに近い状態に陥ったなら、根本的な対立に対処しなければ、このパターンは繰り返されます。
- マネージャーに状況を相談する。
- チーム内に衝突があるなら、マネージャーはそれを知っておくべきです。もしかすると作者が単に扱いにくい人なのかもしれません。あるいは、あなた自身が気づかない形で状況を悪化させているのかもしれません。良いマネージャーなら、二人がこれらの問題に対処するのを助けてくれるでしょう。
- お互いに距離を置く。
- 可能であれば、事態が落ち着くまで数週間はお互いにコードレビューを送り合うのを避けましょう。
- 衝突解決について学ぶ。
- 私はCrucial Conversationsという本が役に立ちました。そのアドバイスは当たり前に聞こえるかもしれませんが、議論の渦中にないときに自分の衝突への向き合い方を分析することには大きな価値があります。
私の最悪のコードレビュー：再訪
Malloryとのコードレビューを覚えていますか？なぜ私のレビューは3週間にもわたる皮肉まみれの泥沼と化し、Bobのレビューは2日で爽やかに終わったのでしょうか？
私が間違っていたこと
これはMalloryにとってチームでの最初のレビューでした。彼女が評価されていると感じたり、防衛的になったりするかもしれないことを、私は考慮していませんでした。大量の指摘で圧倒しないよう、まずはハイレベルなコメントだけに絞るべきでした。
私の仕事は彼女の仕事を妨げることではなく、前進を助けることなのだという姿勢をもっと示すべきでした。コード例を提供したり、良い点を指摘したりできたはずです。
私はエゴをレビューに持ち込んでしまいました。この一年、古いシステムを立て直すために心を砕いてきたのに、突然現れた新メンバーがシステムをいじり、私の懸念を真剣に取り合おうとしない。それを侮辱と受け取ってしまったのです。しかし、そのような態度は逆効果でした。普段のレビューで心がけているような、客観的なマインドセットを保つべきでした。
最後に、行き詰まりを長引きさせすぎました。数回のラウンドを経ても有意義な進展がないことは明らかだったはずです。思い切った変化を起こすべきでした。例えば直接会ってより深い対立に対処したり、マネージャーにエスカレーションしたりといったことです。
Bobが正しかったこと
レビューを分割するというBobの最初の一手は非常に効果的でした。思い出してください、レビューは3週間にわたって停滞していたのです。そこへ突然、2つのコードがマージされたのです。これによりMalloryもBobも前進しているという実感を得られました。残りの塊にもまだ問題はありましたが、より小さく、管理しやすいチェンジリストになったのです。
Bobはレビューを完璧になるまで締め上げようとはしませんでした。私が声を荒げて指摘していたのと同じ問題に彼も気づいていたでしょうが、Malloryがこれから長くチームにいることを理解していたのです。短期的に柔軟に対応することで、長期的にMalloryの品質向上を助ける体制を整えたのです。
おわりに
この記事の前編を公開した後、推奨したコミュニケーションスタイルについて、読者からいくつか異論が寄せられました。見下しているように感じる、という意見もあれば、あまりに婉曲的で誤解を招く恐れがある、という心配もありました。
そのフィードバックはもっともで、予想どおりのものでもあります。ある人にとっては簡潔なレビューコメントが無愛想で失礼に感じられ、別の人にとっては同じコメントが簡潔で効率的だと評価されることもあります。
コードをレビューする際には、どこに焦点を当てるか、フィードバックをどう伝えるか、いつ承認するかなど、多くの選択を迫られます。重要なのは、私の選択肢を選ぶことではありません。選択肢が存在するのだということを認識することです。
完璧なレビューのレシピを誰かが手渡してくれることはありません。最もうまくいく手法は、作者の性格や、相手との関係性、チームの文化によって異なります。コードレビューの結果について批判的に考えることで、自分なりのアプローチを磨いていきましょう。緊張が生じたときは、一歩引いてなぜそうなったのかを振り返ってみてください。レビューの質にも目を向けてください。コードを自分の品質基準まで引き上げられないと感じたら、レビュープロセスのどの部分が妨げになっているのか、どう対処できるかを考えてみましょう。
健闘を祈ります。そして、あなたのコードレビューが人間らしいものでありますように。
参考文献
- 「How to Make your Code Reviewer Fall in Love with You」は、本記事を補完する記事で、レビュアーではなく作者の立場からコードレビューを改善する方法を解説しています。
- PuTTY SSHクライアントの作者であるSimon Tatham氏による「Code review antipatterns」では、レビュアーとして避けるべき落とし穴の有益なリストがまとめられています。
この記事はSamantha Mason氏が編集を担当しました。イラストはLoraine Yow氏によるものです。@global4g氏に、初期のドラフトに貴重なフィードバックをいただいたことに感謝します。