この記事はコードレビューの際に落とし穴にはまらずに、どのようにうまくコミュニケーションをとるか、に関する記事の後半部分です。今回はひどい衝突を避け成功裏にレビューを終わらせるテクニックに焦点をあてます。
パート1で基礎的なことを述べましたので、そこから読み始めることをお勧めします。我慢ができない方のために、簡単にお伝えしますと次の通りです: 優れたコードレビュアーはバグを発見するだけではなく、チームメイトがより良くなるためによく吟味してフィードバックします。
私の最悪のコードレビュー
私の人生で最悪のコードレビューは以前のチームメイト(ここではマロリーとします)とのやりとりです。彼女は私が会社に入る数年前から会社に在籍し、ごく最近になって私のチームに異動しました。
レビュー
マロリーが初めて変更リストを作成し私にレビューを依頼してくれたとき、コードは少し荒削りでした。彼女にとってPythonは初めてで、その上彼女は私が維持管理していた格好の悪いレガシーなシステムを元にコードを書いていました。
私は律儀に気がついた全ての問題を記載し、これは全部で59にもなりました。私が読んだレビューに関する文献によると、私はとても素晴らしい仕事をやりとげました。本当にとてもたくさんの誤りを見つけたのです。つまり、私は良いレビュアーに違いありません。
2, 3日後、マロリーは変更リストを修正し、私のコメントに対する回答をしてくれました。彼女は単純な問題に関しては修正していました。タイポ、変数名の修正、などです。しかし、彼女は高位の問題、すなわち不正な入力に対する挙動が定義されていなかったり、ある関数内の制御フローが6段階ネストされている、といったことに対応することを拒んでいました。そのかわりに彼女は拒絶的にこれらの問題は時間をかけて修正する価値がないことを説明していました。
私は怒り、そして苛立って、新しいラウンドでコメントをつけました。コメントは専門的ではありましたが問題解決に取組むものではない(passive-aggressive)ものとなっていました: 「不正な入力に対して挙動を定義しないのはなぜなのか」。ご想像の通り、マロリーの返信は一層意地のはったものとなりました。
負のサイクル
私とマロリーが同じレビューのやり取りをまだしていた火曜日のことでした。私は夕方に新しいコメントを書いていました。私は彼女がそのレビューを読んでいる時に同じ部屋にいたくなかったので、その日は意図的に彼女が帰るまでそのままにしました。
午前中ずっと、次のレビューラウンドがとても怖かったので悪いことが起きそうな感覚をお腹の底に感じてました。昼食から戻ったら、マロリーは不在でしたが新しい変更をリストをくれていたことに気が付きました。おそらく、彼女は私がその回答を読んでいるときに近くにいたくなかったのだとおもいます。
彼女の回答ごとに私の怒りはどんどん増長し私の心はズキズキし始めました。私はすぐに反発心を持ちキーボードを叩き始め、彼女が私の提案した変更も実施するでもなく、その正当性について私に承認を求めてもいないことを指摘しました。
我々はこのルーチンを3週間にわたって毎日実施しました。コードはほとんど変わりませんでした。
中断
ありがたいことに、我々の一番年上のチームメイトであるボブはこの循環を打開してくれました。彼は長い休暇から戻り、私達がレビューのコメントを投げつけあい行ったり来たりしているのに気が付き注意してくれました。彼はすぐにどのような状況なのか認識しました: 膠着状態です。彼はレビューを引き継ぐよう要請し我々はお互いにそれに合意しました。
ボブはレビューを始めるにあたってマロリーに新しい変更リストを作成するよう要請しました。我々が決して争うことのなかった2つの小さなライブラリを分割し、それぞれおよそ30から50行に分割するように、です。マロリーがこれを終えると、ボブはすぐにこれを承認しました。
それからボブは主な変更リストに戻り、これは200行のコードにスリム化されていました。ボブはマロニーが取り組んだ問題に2、3の小さな提案をしました。そして、彼は変更リストを承認しました。
ボブのレビューは全体で2日で完了しました。
コミュニケーションの問題
おそらくみなさんはこの衝突は本当はコードに関するものではないと推察されることでしょう。本物の問題もありましたが、これは効果的にコミュニケーションできるチームメイトであれば、明らかに解決できたでしょう。
これは嫌な経験でしたが、今になって思えば感謝すべき経験です。これにより自分のレビューのアプローチを再評価し、改善点を見つけられました。
次に、同じように望ましくない結果になるリスクを低減するためのいくつかのテクニックをご紹介します。後程マロリーの件に戻り、そこでなぜ当初の私のアプローチが悪い方向に向かい、ボブのアプローチがとても素晴らしかったかについて説明します。
テクニック
- 9. コードを1段階か2段階向上させることを目標にする
- 10. 同じ指摘を繰り返さないようにする
- 11. レビューのスコープを順守する
- 12. 大きなレビューを分割する機会をさがす
- 13. 心の底からの賞賛を
- 14. 残った修正が軽微なものであれば承認する
- 15. 膠着状態に対してはプロアクティブに対応する
訳注: No. 1-8は本記事の前半記事の通番です。
コードを1段階か2段階向上させることを目標にする
論理上はチームメイトはコードを改善するためにすべての機会を探し求めているはずですが、彼らの忍耐にも限界があります。彼らの変更リストを改善する新しくよりよい方法をあなたが考え続け何ラウンドも承認せずにいると、彼らの苛立ちはすぐに募ります。
私はコードについて個人的にAからFまでの評価を考えます。もしDから始まる変更リストを受け取ったならば、オーサーがCまたはB-にもっていけるように手助けするよう試みます。完璧ではなく、十分なレベル目指します。
理論上は、DをA+にすることもできますが、これはおそらく向上させるには8ラウンドものレビューを実施することでしょう。終いには、オーサーはあなたのことを嫌いになり、あなたには2度とコードを見せたくないと思うことでょう。
次のように考えるかもしれません、「Cレベルのコードを承認したら、コードベースは最終的にCグレードになってしまうのだろうか？」幸運にも、そうではありません。チームメイトをDからCに向上するのを手助けすれば、彼らの次の変更リストはC「から始まる」ことでしょう。2, 3ヶ月のうちにBから始まるレビューを提示するようになり、こうなればレビューの最後にAにできるのです。
Fは機能的に正しくない、あるいは正しいかどうか確信を持てないくらいとても複雑な場合にとっておきます。唯一承認を保留すべきケースとして、レビュー2, 3ラウンドでFのままである場合があります。この場合は下記の膠着状態に関する章を参照してください。
同じ指摘を繰り返さないようにする
全く同じパターンの複数の誤りに気がついた場合、それぞれにコメントするのはやめましょう。同じことを25回書くのに時間を使いたくないでしょうし、オーサーは25回同じコメントを読みたいと思っていないに違いありません。
指摘は2, 3の個別の箇所に留めるのが良いでしょう。それぞれの例に対応するより、単にそのパターンを修正するようにオーサーに依頼するのが何よりです。
※レビューコメント訳
インスタンス変数名は、それがpublicのインターフェースの一部でない限りはアンダースコアで始めるべきです。
https://google.github.io/styleguide/pyguide.html#Naming
(他の箇所も同様)
レビューのスコープを順守する
私がよく見るアンチパターンとして、レビュアーが変更リストの近くのコードに関して何かしら気がつきオーサーに修正を求める、というものがあります。一度オーサーが応じると、通常レビュアーは良くはなっているものの一貫性がない事に気が付き、更に細かないくつかの変更が必要となります。そしていくつか更に...。そしてそして小さくスコープが指定されている変更リストが多数の無関係な変更を含むものに拡大されます。
玄関の近くに小さな空腹のネズミが待ちうけていれば、おそらくクッキーをあげたくなることでしょう。クッキーをあげれば、ミルクが欲しいというでしょう。そして鏡の中にミルクの口ひげが映らないように、はさみが欲しいというでしょう...
Laura Joffe Numeroff著, If You Give a Mouse a Cookie
経験則としては次のとおりです: その行が変更リストの範囲でなければ、それはスコープ外です。
ここで一例を示します:
仮にあなたが徹夜をしており、コードベースのマジックナンバーとおかしな変数名に悩まされたとしても、これはスコープ外です。オーサーがその近くの行を書いたのと同じ人であったとしても、これはスコープ外です。それがどんなにひどかったとしても、バグを記録するかあなた自身が修正を提示するかのどちらかとし、このレビューでのオーサーに対応を強いないことです。
この例外として、変更リストは実際にそれを変更してはいないものの周辺のコードに影響をあたえる場合があります。例えば次のとおりです:
このケースでは、オーサーは関数名をValidateAndSerializeからただのSerializeに変更すべきという指摘です。関数シグネチャを含む行は変更していませんが、変更により正しくなくなっています。
指摘箇所は少ないもののスコープ外の簡単な修正に気がついた場合に私はこのルールをゆるく破っています。この場合には、オーサーはコメントを無視しても良いことを明示しています。
大きなレビューを分割する機会をさがす
400行を超えるコードの変更リストを受け取った場合、オーサーにはこれを小さく分割するように促します。この制限を超えれば超えるほどそれだけ反対をとなえるのが難しくなります。私は個人的に、1000行以上の変更リストはいかなるものも受け取らないようにしています。
変更リストの分割は退屈な作業ですのでオーサーはこれに不平をこぼすことでしょう。この分割の論理的な境界を明らかにすることにより彼らの重荷を軽減します。もっとも簡単なケースとしては変更リストが複数のファイルに独立に変更を加えている場合です。この場合は、変更リストをより小さい単位のファイル群に分割するだけで良いでしょう。難しいケースでは、関数やクラスの最も抽象化レイヤーが低いものを探します。オーサーにこれらを別々の変更リストとするように依頼し、最初の変更リストがマージされてから残りのコードに戻ります。
コードの品質が低い場合は、はっきりと分割することを依頼します。悪いコードをレビューする際、その大きさに対して指数関数的にレビューが困難となります。600行の1つのひどいコードより、1組の粗雑な300行の変更リストのほうが遥かによく内容を確認できます。
心の底からの賞賛を
大多数のレビュアーはコードのどこが誤っているかのみに焦点をあてますが、レビューはすばらしいところをより促進する貴重な機会です。
例えば、あなたがレビューしておりオーサーがドキュメントを頑張って書いており明確で簡潔な関数のコメントを見つけたシーンを想像してみてください。よくやったことを伝えてあげます。出来がひどい場合にそれが改善されるのをただ待つだけではなく、その代わりにきちんと修正されたときにそれを伝えてあげれば、改善の速度が増すことでしょう。
賞賛をするために具体的なゴールを念頭におく必要はありません。変更リストに何か喜ばしいことを見つけたときはいつでもそれをオーサーに伝えてあげています:
- 「このAPIは知りませんでした。とても便利ですね！！」
- 「これは素晴らしい解決策です。考えてもみませんでした。」
- 「この関数を分割するのは素晴らしいアイデアですね。以前よりとてもシンプルになりました。」
オーサーが経験の浅い開発者であるかあるいは最近チームに加わった場合は、彼らはレビューの最中に神経質あるいは防衛的になりやすいのです。丁寧に誠意ある賛辞を送り、あなたが非情なゲートキーパーではなく助けてくれるチームメイトであることを実際に示すことで、彼らの緊張は緩和します。
残った修正が軽微なものであれば承認する
レビュアーの方々の中には、全てのコメントに対する修正を確認するまで承認をしてはならない、という誤った考えをお持ちの方がいます。これにより不要なレビューラウンドが発生し、レビュアーおよびオーサー両者の時間が無駄になります。
以下のいずれかに当てはまれば承認します。
- 追加のコメントがない場合
- 残りのコメントが軽微な問題である場合
- 例) 変数名の修正、タイポの修正
- 残りのコメントがオプショナルな提案である場合
- チームメイトがこの修正が承認の条件だと考えないようにオプショナルであることを明示しましょう。
コードへのコメントの最後のピリオドが欠けているためにレビュアーが承認を保留したのを見たことがあります。これはやめてください。これはあなたが「彼らは単なる句読点の追加すら監督されないとできない」とみなしているとオーサーに伝えることになります。
未解決のコメントがあるのに承認するのはいくらか危険があります。私の見積もりでは 〜 5%のケースで、オーサーは最後のラウンドのコメントに対して誤った解釈をするか、あるいはそれを完全に忘れるかします。これを緩和するために、私は単純に承認後のオーサーの変更をチェックしています。ミスコミュニケーションがあった場合はオーサーにフォローアップするか修正の変更リストを自分で作成するかのいずれかとします。5%のケースに対して少量の修正をするほうが、その他の95%に対して不要なコストをかけ遅延を発生させるより良いのです。
膠着状態に対してはプロアクティブに対応する
コードレビューでありうる中で一番良くない結果は膠着状態です: あなたが変更なしには変更リストの承認を拒否し、一方でオーサーがその変更を拒んでいる場合です。
膠着状態に向かっていると判断するいくつかの指針を示します:
- ディスカッション口調の緊迫度合いあるいは敵対度合いが増している
- レビュー毎のあなたのコメントが収束傾向となっていない
- 通常ではない数のコメントで押し返している
話してみる
対面で話すか、ビデオチャットをします。文字のコミュニケーションは、会話のもう片側に現実世界の人間がいることを忘れてしまうような事態を引き起こす1つの要因となります。チームメイトが意地を張ってるから、あるいはできる能力がないからそうなっているのかを容易に想像できるようになります。対面することであなたとオーサー両者にかかった魔法が解けるかもしれません。
設計のレビューを考える
議論を引き起こすコードレビューは、このコードレビュー以前の過程が不十分であることを示しているかもしれません。設計レビューの際に網羅すべきだったことについて言及しましたか？設計レビューはありましたか？
意見の不一致の根本原因が高位のレベルの設計の選択まで戻るのであれば、たまたまコードレビューをしている2人の手の内に留めるのではなく、よりチームの広範囲のメンバーが介入すべきです。設計レビューをチームの残りのメンバーに対する公開ディスカッションとして開催する中でオーサーと話し合いましょう。
譲歩するかエスカレーションする
あなたとチームメイトの膠着状態が長引けばそれだけ、関係が損なわれます。複数の選択肢で解決しないのであれば、あなたの意見を譲歩するかあるいはエスカレーションします。
変更を承認することのコストだけを見積もります。低品質のコードを不用意に承認すればソフトウェアの品質は築けませんが、あなたとチームメイトが激しくやり合い一緒に働かなくなっても高品質も達成できません。この変更リストを承認すると本当に何が良くないのでしょうか。重要なデータを破壊する可能性があるコードでしょうか？あるいはバックグラウンドプロセスでの修正で、最悪のケースでジョブが失敗し開発者がデバッグすれば良いでしょうか？もし後者に近いのであれば、チームメイトと良い関係性で働き続けられるように単に譲歩することを考えます。
譲歩できないと判断されれば、この議論をチームのマネージャーあるいは技術リーダーにエスカレーションすることをオーサーに伝えましょう。違うレビュアーを再アサインするよう提案します。エスカレーションの結果あなたの意見と異なる場合、決定を受け入れ次に進みます。やり合いが継続すると良くない状態が長引きあなたがプロフェッショナルでないように見えます。
膠着状態を脱する
入り乱れたレビューのコメントはコードについての内容ではなく関係する人々の関係に関するものになりがちです。膠着状態あるいは、膠着状態に近い状態に達した場合、内在する衝突の問題に対処しなければこの状況は継続するでしょう。
- 状況についてマネージャーと議論する。
- チーム内の衝突があれば、マネージャーはそれを把握しておく必要があります。おそらくオーサーは対処が難しいでしょう。 もしかするとあなたは自身が受け入れない状況を作るのに寄与しているかもしれません。素晴らしいマネージャはこれらの問題を解決するために両者の助けとなるでしょう。
- お互いに休憩を取る。
- 可能であれば、クールダウンするまでお互いに2, 3週間コードレビューを止めてみます。
- 衝突の解消について学ぶ。
- 「Crucial Conversations」が有用です。記されたアドバイスは常識のようですが、あなたがそのつもりはなくても衝突しているアプローチについて分析するのにとても大きな価値があります。
私の最悪のコードレビュー: 再考
マロリーとのコードレビューを覚えているでしょうか？なぜ私のレビューは3週間、問題の解決に向かわないおぞましいものだったのに、ボブのレビューは2日間でいとも簡単に終わったのでしょうか？
私の何が悪かったか
これはマロリーがチームに来て初めてのレビューでした。私は彼女が非難されていると感じたり防御的になっているであろうことを考慮しませんでした。私は彼女が大量のコメントに不意討ちされたように感じないように「4. 高位のレベルから始め詳細化する」べきでした。
私は自分のレビューが彼女の仕事を妨げるためでないことをもっと実際に示す必要がありましたが、どちらかといえばこれを前に推し進めてしまいました。私は「5. コードのサンプルを示して寛大に」し、彼女の変更リストに対して「13. 心の底からの賞賛を」送るべきでした。
私は「個人的なエゴ」をレビューに介入させてしまいました(6. 相手に直接言及しない)。私は過去数年間この古いシステムを健全にするために時間を費やしてきました。突然これをいじる新しいメンバーが現れたのですが、彼女は私の懸念を真面目に受け止めこれ以上にないほど悩んだでしょうか？私はこれを無礼と感じたのですが、その態度が逆効果でした。レビュー全てに客観的な心持ちを持ちつづけるべきでした。
最後に、私は膠着状態をあまりにも長く継続させました。2, 3ラウンドの後に、我々は有意義な前進をしていないことを明確とすべきでした。私は「大きな変更を決断し(15. 膠着状態に対してはプロアクティブに対応する)」、例えば直接話し合い深い衝突に対処したり、マネージャーにエスカレーションすべきでした。
ボブの何が良かったか
ボブが最初の第1手として「12. レビューを分割」したのはとても効果的でした。レビューが苦痛の3週間にわたって停滞していたのを思い出してみてください。突然、コードの2ピースがマージされたのです。これは前に進む動きを確立したため、マロリーとボブ両者の気分を良いものとしました。残りの部分に問題が残っていたものの、これは小さく扱いやすい変更リストとなっていました。
ボブは「レビューを完全なところまでもっていこうとはしませんでした(9. コードを1段階か2段階向上させることを目標にする)」。彼は私が叫んでいたものと同じ問題に気がついていましたが、マロリーはしばらくチームに居ることを認識していました。彼の短期的な柔軟な対応によって、マロリーが長期的に品質を向上させるのに役立つように関係を築きました。
結論
この記事の前半を公開した後、読者の何名かは私の提案したコミュニケーションスタイルに問題を提起しました。ある人達は気に入りました。その他の人は、間接的でリスクのあるミスコミュニケーションを懸念していました。
このフィードバックは理にかなっており予測されたものです。ある人は簡潔なレビューコメントをぞんざいである、あるいは無礼であると感じるかもしれません。他の人は同じコメントを簡潔で効率的と感じるかもしれません。
コードのレビューでは、あなたは多くの選択をします: 何に焦点を当てるか、フィードバックをどのように構成するか、いつ承認するか、です。私の意見を選択するかは重要ではありません。ただこのような意見があることを認識すればよいのです。
あなたに完全なレビューのレシピを手渡しできる人は誰もいません。最もうまくいくテクニックはコードのオーサーの性格や、彼らとの関係、チームの文化に依ります。コードレビューの成果について真剣に考えることによってあなたのアプローチを磨き上げましょう。レビューの品質に注意を払いましょう。あなたの基準を満たすところまでコードの品質を向上できないと感じるなら、レビュープロセス上の何が妨げになっているか、そしてどのようにそれに対処できるかを考えましょう。
人間らしいコードレビューができるよう、幸運を祈ります。
追加の文献
コードレビューの社会的な要因に十分な注意をはらっている私が知りうる唯一の著者として、Dr. Kari Wiegersがいます。彼は「Humanizing Peer Reviews」で彼の見解をよくまとめています。2002年に書かれたものですが、継続して適用可能な内容が効果的なコミュニケーションの長期的な価値を実証しています。
この記事は編集: Samantha Mason、イラスト: Loraine Yowです。この投稿の初期の草稿段階で貴重なフィードバックをくれた@global4gへ感謝します。
