- コードレビューは良いアイデアのように思えますよね?
- コードレビューによって、2人の開発者がコードを見ながら問題を発見し、プロジェクトの発展過程への理解を共有する機会が生まれます
- レビュアーは作者のコードを詳しく見ながら有用なテクニックを学んだり、作者に有用なテクニックを教える機会を見つけたりできます
- しかし、これは「ライトサイド」のコードレビュアーが使うやり方です。コードレビューをコード改善と開発者たちの集団的な技術向上のために使うのです
- コードレビューは、まったく別の目的のための強力な道具にもなりえます。レビュアーが「ダークサイド」に転向すれば、コード改善を妨げたり遅らせたりするさまざまな方法を使えます
- パッチ作者を苦しめたり、完全にくじけさせたりするなど、ほかの個人的な目的を追求することもできます
- 最近「ダークサイド」に転向したレビュアーなら、まだすべての可能性を考え尽くしていないかもしれません
- そこでこの記事では、ダークサイドのコードレビュアー向けのアンチパターン一覧を示します
[アンチパターン]
The Death of a Thousand Round Trips
- コードを読みながら些細なことを見つけるたびにレビューコメントで指摘し、その時点で読むのをやめます
- 開発者はまじめにその些細な点を直し、修正版のパッチを提出します
- レビュアーはその版を読み始め、また別の些細なことを見つけます。最初のレビューで言えたはずですが、そうしませんでした。その些細な点を指摘して、また読むのをやめます
- これを開発者が希望を失うまで繰り返します
The Ransom Note
- このパッチは開発者にとってとても重要らしいです
- しかしレビュアーにとってはそれほど重要ではありません。したがってレビュアーは権力のある立場にいます
- 開発者が望む変更を、レビュアーにとって重要な追加作業が完了するまで人質に取れます
- 追加作業は実際には同じコミットに含まれる必要はありませんが、レビュアーにとって重要な作業です
The Double Team
- 1つのパッチ、2人のレビュアー
- 一方のレビュアーが変更を要求するたびに、開発者は素直に変更します。すると今度はもう一方のレビュアーが文句を言う番になります
- レビュアーたちは交互に、互いに矛盾する要求を出します
- ただし、常に開発者に向けてコメントします。レビュースレッドで互いに直接議論するのは避けます
- 開発者が諦めるまで、レビュアーたちが開発者を何度前後に振り回せるかを見るのです
The Guessing Game
- 問題があり、その問題を解決する方法はいくつもあります
- 開発者は1つの解決策を選び、パッチを提出します
- レビュアーは、その特定の解決策を、問題解決とは無関係な理由で批判します
- 曖昧な設計原則への違反や、将来の開発計画との非互換性などを理由にします
- 批判を十分に曖昧にすると、開発者は既存のパッチをどう修正すればその批判に対応できるのかわからなくなります
- 開発者としては、元の問題を解決するために、まったく別の解決策を選んで実装し直すのが最善になります
- するとレビュアーは再び、同じように役に立たないやり方でそれを却下します
The Priority Inversion
- 最初のコードレビューでは、些細で簡単なことを指摘します
- 開発者がそれらを修正するのを待ってから、重大な問題を提起します
- パッチのかなりの部分を完全に書き直さなければならない根本的な問題があります。これは、すでに行った些細な修正作業の多くを捨てることを意味します
- 誰かに多くの作業をさせて、それを捨てさせることほど、「あなたの作業は望まれておらず、あなたの時間は大切ではない」というメッセージをよく伝えるものはありません
- これだけでも、開発者が諦めるには十分かもしれません
The Late-Breaking Design Review
- 数週間または数か月にわたって、多数の別個のパッチで非常に複雑な作業が進められてきました
- レビュアーはその作業全体の設計に賛成していませんが、元の議論には参加していなかったか、議論で負けた側でした
- ところが今、レビュアーにはその作業の途中にある些細だが重要な部分（たとえば多くの開発者を塞いでいるバグへの簡単な修正）をレビューしてほしいという依頼が来ます。これはレビュアーにとって好機です
- レビュアーは、ここまで行われてきた作業全体の設計について正当化を求めます
- 開発者がその作業全体の一部分しか担当しておらず、答えを知らないとしても、それはレビュアーの問題ではありません。レビュアーが満足するまで「承認」ボタンを押しません
The Catch-22
- 1つの大きなパッチだと読むのが難しすぎます。Self-Containedな下位の断片に分割するよう求めます
- 逆に小さなパッチが多いと、その一部はそれ自体では意味がありません。再びまとめるよう求めます
- ある種のトレードオフが特定のケースで関係ありそうに見えるなら、それを利用して不満を言う理由を見つけられます
- たとえばコードがわかりやすく書かれていれば性能が悪いはずで、よく最適化されていれば読みにくく保守しにくいはずです
The Flip Flop
- コードの同じ部分に、人々が一般的に行う種類の変更があります
- レビュアーは以前、こうした変更を何度もレビューしたことがあります
- また別の変更が入ってきます。作者は過去の変更を詳しく調べ、既存のパターンを注意深く踏襲し、この領域に詳しそうなレビュアーを選びます
- レビュアーは、以前はまったく問題にしなかった変更の一側面に突然異議を唱えます。既存のパターンに従うだけでは十分ではありません
- 作者が以前の同種の変更を示しながら、レビュアーの一貫性のなさを指摘したらどうなるでしょうか?
- レビュアーは「その通り。それも変更されるべきだ」と言います
- レビュアーは既存の事例をすべて変更すると自ら申し出ないよう注意すべきです。運がよければ、開発者がそれを既存事例を自分で変更しろという指示だと解釈し、レビュアーは多くの労力を節約できます
しかし真面目な話 ...
- ここまでの話は冗談でした。レビュアーがわざとこんな悪い振る舞いをしていると示唆したいわけでもありません
- 説明の大半は、レビュアーが実際にしていることの誇張か、あるいは苛立ったパッチ投稿者の感じ方の誇張です
- 実際にはこういうことをしてはいけません!
- ラウンドトリップを最小限に抑えるよう努め、些細なことをつつく前に（必要なら）重要な書き直しを求め、パッチを批判するときは、どのような版なら受け入れられるのか建設的な提案をすべきです
- コードベース全体について意見があるなら、1人の開発者のパッチをレビューして揚げ足を取るのではなく、すべての開発者と全体的な議論をすべきです
- うっかりこうしたことをしてしまったなら、自覚すべきです。うっかり開発者の人生をより難しくしてしまったと認識し、謝罪し、もっと助けになれるよう努めるべきです
- 根本的な問題は権限の濫用です
- ある開発者が別の開発者のパッチのコードレビュアーになると、一時的な権限が生まれます。レビュアーには、そのパッチがコミットされるのを止められる権限があります
- 権限には責任が伴います。意図された目的のために、必要な分だけ権限を使うべきです
- ほとんどのアンチパターン（または風刺している穏当な行動）は権限の濫用です。レビュアーがパッチ改善とは無関係、あるいは逆行する個人的なアジェンダを追求するために、開発者に対する一時的な権限をてことして使っているのです
- 個人的なアジェンダはアンチパターンごとにさまざまです。無関係な作業、個人的なスタイルの好み、怠惰、変化への抵抗、パッチ投稿者への個人的な嫌悪などがあります
- パッチ投稿者が過去にコードレビュアーとしてこうした悪い振る舞いをしていたなら、嫌悪が正当化されることもあるかもしれません。だからこそコードレビュアーの権限は慎重に使うべきです
- 開発者たちが互いへの報復の悪循環に陥れば、ソフトウェアプロジェクトは破滅します
Gatekeeping コードレビュー
- ここまではピア間のコードレビューに焦点を当ててきました
- コードレビュアーとパッチ投稿者は同僚であり、互いに責任を負わせる立場でも、コードベースに対する最終決定権を持つ立場でもありません
- そのため、コードレビューで得られる権限は一時的です
- ピアレビューの状況では、コードレビュアーと作者は基本的に同じ目標を持つべきです
- どんな機能を入れるか、承認前にどれだけ磨くべきか、許容可能なコーディングスタイルは何かについて深刻な意見の不一致があるなら、コードレビューの文脈外で扱うべきです
- しかし、別種のコードレビューの状況ではそうではありません。特に、プロジェクト外部の人が求められていないパッチを送ってくる場合は大きく異なります
- こうしたシナリオは一般にフリーソフトウェアで起こります
- 世界中の誰もがソースコードを修正でき、その一部の人が変更を送り返そうとするからです
- しかし、ほかの状況でも起こりえます
- プロプライエタリなコードを開発する会社の中で、あるチームの開発者が別チームのプロジェクトにパッチを送り、運を天に任せることがあります
- こうした状況では、パッチ受領者がそもそもその変更を望んでいなかったり、実装方法にまったく同意していなかったりして、パッチをまったく受け入れない可能性がはるかに高くなります
- この場合、それは同僚レビュアーとして与えられた一時的な権限の濫用ではなく、プロジェクト責任者としての恒久的な権限を正当に行使しているのです
- 自分のソフトウェアプロジェクトの方向性は自分で決めます
- コードレビュアーがこのような「ゲートキーピング」の役割を果たすとき、パッチが既存の一般的な設計原則や要件に違反しているという理由でパッチを批判することが、常に間違いというわけではありません
- もしかすると要件に合致する形でその問題を解決する方法がわからないのかもしれません
- 実際、それが難しい部分であり、自分がまだ同じ変更をしていない唯一の理由かもしれません
- しかし、ゲートキーピングの文脈でも、説明なしに「The Guessing Game」を適用するのは無礼です
- 私は通常、開発者が難しい部分を見落としていたことを説明しようと努め、自分でも答えがわからないならそう言うようにしています
- もちろん、「The Death of a Thousand Round Trips」のような消極的で攻撃的な妨害には弁解の余地がありません
- 本当にそのパッチをコードにまったく入れたくなく、その決定を下す正当な権限があるゲートキーピング役なら、投稿者がこれ以上時間を無駄にしないよう、言葉で説明できます
Disclaimer
- 私は何年ものあいだ、自分が関わった（両方の立場での）コードレビュー、ほかの人たちの間で観察したコードレビュー、会話で聞いただけのコードレビューから、この記事のためのメモを集めてきました
- したがって、最近私のコードをレビューした特定の誰かを狙ったものではありません
13件のコメント
意外と誇張ではないかもしれない....
これは本当に私が実際に経験したことですが、開発者を辞めかけました。本当に立ち直るのが大変でした。
読んでいてPTSDになりかけた
この話題のためのメモをこれまでうまく集めてきたようですね！！
読むだけでも精神的虐待レベルです...
要するに、最後の一文が核心ってことですよね？www.....
うわ……本当にイライラしましたね;;
こういうのは、SI + SM をやっているところに行けば、日本でもよく見かけます。よく言う「縄張り意識」ですね。意地の悪い人間が自分の既得権益を守るために、いろいろなことをするものです。
邪悪な方法がたくさんありますね。
長期的に見れば、合理的な理由もなくああいう振る舞いをする人は、1) 早い段階で開発者の人脈から排除されるか、2) 性格が悪くても能力がずば抜けていて大きな役割を担っており、排除しにくいので、誰かがアダプター役としてコミュニケーションの窓口になってつながりを保っているからその状態が維持されているだけで、その仲介役が何らかの理由でいなくなれば、そう遠くないうちにすぐ排除される。いくら抜群に優秀でも、結局は人が集まって何かをしてこそ金も動くし、金が動いてこそ人も動く。だから人とうまくやれない人は集団から排除され、淘汰されるものだ。
普通、集団の中で性格が悪いのに長く生き残った人は、自分がまるでドラマ『シャーロック』みたいな高機能ソシオパスのような特別な何かだから生き残れたのだと勘違いしがちだが、利用価値があるから周囲がただじっと我慢して使っているだけで、利用価値がなくなれば「一緒にいて不愉快だったし、もう二度と会うまい」という関係になる。カンバーバッチ主演の『シャーロック』も、私たち外野から見れば魅力的なソシオパスに見えるだけで、周囲にシャーロックを見捨てず大切にしてくれる人がいなければ、ただそれだけで何のストーリーにもならない。
性格が悪いのに長く生き残ってきた人は、自分がまるでドラマのシャーロックのような高機能ソシオパス的なすごい何かだから生き残れたのだと勘違いしがちだが、実際には利用価値があるから周囲がただぐっと我慢して使っているだけで、その利用価値がなくなれば「一緒にいて不快だったし、もう二度と会わないでおこう」という関係になる、ということですね。 ==> ものすごく刺さる文句です。覚えておかないと。
職場いじめ、あるいはパワハラを思い出しますね
ユーモアだとは言うけれど、読んでいるとイライラが一気にこみ上げる…。