WEB+DB の新しいやつがちょっと前にでてます. コードレビュー特集だそうな. 時が経つのは早い. まだ次の原稿書いてないのに…
そういえば前にコードレビューの話を書いた気がして, 見なおしたところ かきかけ だった. せっかくなので続きを書いてみることにします. といっても何書くつもりだったか覚えてないのでだらだらと.
WEB+DB PRESS の特集は, 主にこれからコードレビューを導入したい人に向けて書かれている. 幸か不幸か私はコードレビューを義務付けれたプロジェクトで働いているため, 導入には苦労していない. かわりにレビューをちょろまかせない面倒はある. ある意味でコードレビューを <やらされている>.
もちろんこの言い分は大げさだ. 必要性に異議を唱える気はない. ただ異議はさておき自分の意向とは無関係にコードレビューに参加している気分を書いた話は あまり目にしないので, そんな立場から何か書いてみたい.
なお私が仕事でやっているレビューはコミット前にパッチ単位で見るやつ. Pull request の不便バージョンだとおもってもらえばよいです.
レビューの良し悪し
コードレビューにも上手い下手, 良し悪しがある. 良し悪しの程度は定量的な指標でも議論できるし, 定質的な井戸端談義もある.
定量的な指標とは, 古いところだと 欠陥検出率 みたいのがある. 単位時間あたりにみつけたバグの数が多いほど良いレビューというわけ. もうすこしカジュアルな指標だと, たとえば 応答時間 なんてのもあるだろう. レビューを頼まれてから返事をするまでの時間が短いほど良い. そのほか平均の ターンアラウンド数 も指標になりうる. 何度も駄目出しをするレビューはよくないレビューといえる, かもしれない. すくなくともやられる側の気分は悪い.
定量的な指標は追跡しだすとキリがないものの, 目標設定に使いやすいのは利点. たとえば人事考課の際, 同僚フィードバックの１つが “レビューの反応が悪い” と指摘していたとしよう. そんなときは適当な bot を書いて自分の反応時間を記録し, たとえば <1 時間以内に反応する> なんて目標をたてることができるかもしれない.
よく知られているように, この手の定量的な指標を人事評価それ自体に使うべきでない. チートが横行してすぐに本来の意図が損なわれる. 個々人が自分のためのツールとして使うくらいに留めたい.
良し悪しには定量的な指標だけでなく, 定質的な…というか主観的な指標もある. 要するにイラッとくるレビューもあれば目から鱗のレビューもある.
たとえば細かいコーディングスタイルの間違いや好み違いを指摘されるとまあまあ気分が悪い. すぐに慣れて何も感じなくなるけど, 本来ならできるだけ機械的なチェックで済ませたいもの. WebKit は検索会社由来の cpplint.py を改造した check-webkit-style が執拗に細々した間違いをみつけてくれるが, それでもスタイルや好みの指摘はなくならない. Go 言語の gofmt ツールもスタイル自転車置き場の不毛さを訴えている. コーディングスタイルだけでなく, 関数やクラスの粒度も好みの押し付けと意義深いレビューの境界にいると個人的には思う.
一方, 明らかなバグを指摘されると大変ありがたく思う. そして指摘が少なく, いつもすぐ LGTM をくれるレビュアは, ありがたいと同時に少し不安になる.
より抽象度の高い, 設計方針に関するフィードバック. これは内容によって毒にも薬にもなる. ただ基本的にエラい人 - 変更前のコードを書いた人や該当モジュールに詳しい人の意見は無視できない. それに詳しい人の指摘は大抵的を射ている. そんな指摘は, レビュイにとって主観的に良いレビューといえるだろう.
Review Code Like a Boss
あるとき出張先の食堂でランチの席を探していると, 同じく出張中の同僚に出くわした. 大きな仕事を片付けるためこの地に長居しているらしい. 食事をしつつ, ゴシップ好きの私は尋ねた:
こっちでは B さんにレビューされてるらしいじゃないですか. B さんは気分屋って噂だけど, どう? なんか違う?”
B はコードベースの中心にあるモジュールをリードする古参の一人. エライ人の悪口で盛り上がるのは会社員の嗜みである.
同僚はこう答えた.
いやあ, やっぱり違いますね. デザインのレベルからズバッと来るので勉強になりますよ. こないだは “この機能追加でそのファイルをさわるのはおかしい. こっちのモジュールで対処すべき” って変更ファイル名のリストだけからダメだしされちゃいました.
ゴシップ的な答えを期待する私の興は削がれたものの, 貫禄ある態度に B への印象は上向いた. コードをよく知るエースやベテランほど構造上の指摘を厭わない面はある.
構造上の指摘に応じると, 時にはコードを丸々書きなおす羽目になる. レビュアとしても, 書き直しになるダメだしは若干心苦しい. また構造上の違和感を言語化したり, 正しいあり方を指摘するのは揚げ足取りよりも難しい. 単に “違和感がある” とケチをつけるのは気が引ける. そんな遠慮から, ちょっとした構造の歪みはコードレビューで見逃しがちになる. レビュー相手が身近な同僚で, 締め切りが近かったりするとなおさらだ.
コードベースへの理解が深ければ全体像の見通しが持てる. あるべき姿を提案できる. それにエラくなるほど個々の締め切りよりコードの健全さを優先するもの. だから構造上の問題をバシバシ指摘できるのだろう.
理想的には各人がコードベースの意図を正しく把握し, 実装方針もコードを書く前に議論して合意をとる方が良いのだろう. ただコードベースの大きさに対して開発ピッチが速いと理解が変化に追いつかない. またチームが大きいと, 正しい相手をつかまえて議論をするのも一苦労. 万全を期して資料を書きミーティングやメールの返事を待つより, コードを書いて送りつける方が早かったりする. たまに書き直しになるとしてもね.
小さいチームのコードレビューでそんなバッサリレビューが多いとしたら, それはチームの健康が歪みかけているシグナルだろう. そんなとき, シニアは新入りメンバーがコードの理解を深める手助けをできるかもしれない. 日々の仕事の中でデザインを議論する機会を増やしても良い.
一方でチームやコードが大きくなる中でエースプログラマが存在感をスケールするのに, バッサバッサとコードをレビューするのは 1 つのやり方に見える. 多くのコードに目を通し, その整合性を高める.
広い範囲のコードをレビューすれば, より大きな構造上の問題に気づくこともできる. 不穏な気配を嗅ぎつけ, 必要に応じチームに警鈴を鳴らし, 根深い問題を正す大きなリファクタリングを推し進める. おそらく B はそのように働いている. B にかぎらず, 大きなプロジェクトはそんなプログラマが支えている…気がする. 手厚いプロセスがコードを守ってくれることもあるだろうけどね.
Review Code Like a Pointy Haired Boss
話がそれた.
問題領域やコードをよくわかってるプログラマがレビューすると意義深いフィードバックにつながる. 逆にいうと, 問題領域やコードをよくわかっていないプログラマによるレビューは(主観的に)ぱっとしない.
なんでそんな人がレビューをするのか. いくつか事情がある. まず, 望ましいレビュアが捕まらないパターン. 長期休暇中かもしれないし, デスマ中かもしれない. 単につれないだけかもしれない. そもそも <望ましいレビュア> なんてのがいないこともある. もうプロジェクトに参加していなかったり, コードを書いた当人が唯一の <望ましいレビュア> だったり. どれも情けない話だけれど, 大きなプロジェクトではたまに見かける光景でもある. 成人病みたいなもの.
姿を消した <望ましいレビュア> の穴は, かわりのレビュアがうめる. かわりのレビュアはかならずしもコードをよくわかっていない. だから書き手の意図を汲めるとも限らない.
代わりのレビュアにとって, 細かいことを言わずさっさと LGTM してしまうのは１つの手だ. けれど誰もがそう強気になれるわけではない. 思わぬバグを見逃してしまったら? あとから <望ましいレビュア> がやってきてケチがついたら? 不安は多い. かくして気の弱い代理レビュアは腰が引けつつレビューをする.
腰が引けたレビューはこんなふうに進む: まず, 細かい揚げ足取りが増える. 些細なスタイルのあらをつついたり, 潔癖すぎるようなリファクタリングを言い出したりする. 心のどこかで LGTM を恐れているからだ. 発言が二転三転することも多い. 細部に対する理解が浅いせいで, 自分の的外れな指摘にあとから気づき主張がぶれる.
テストを書けとうるさいのも特徴. 理解の甘さをテストで埋め合わせようとする. 作りの良し悪しはよくわからんけどこれだけテストすればバグはなかろう, そんな言い訳がある. きちんとテストするのは悪いことではないけれど, プロジェクト全体の水準にあわない過剰さは無駄だ.
代理レビュアは最後までコードの正しさに確信を持てない. だから既存のコードを大きく変更するのを嫌う. 問題があったときにいつでも revert できるよう, 差分が小さく綺麗なパッチを求める. 結果としてデザインの整合性が損なわれることがある. あるいは過剰なリファクタリング要求とパッチの小ささが衝突し, パッチを細かくわけろと必要以上にうるさい.
腰が引けたレビューとは, 要するに保守的で形式的なレビューだ. 理解不足をプロセスで補おうとする.
私は諸事情からぼちぼちこの手の代理レビューをする. 昼ドラの姑みたいなレビューをするのはいつも心苦しい. ただ古い大きなプロジェクトには人手不足のモジュールもある. つれないレビュアの守備範囲で仕事を進めたい時もある. そう諦めてコードを読み, レビューしている. レビュアの言動が pointy haired なのは, かならずしも性格が悪いせいじゃない…とおもう…ごめんよほんと.
Review Idioms
さて, レビュアの当たり外れがどうであれ, 会社員たるもの日々のコードレビューを避けては通れない. そして毎日数をこなすうちに段々とコツがわかってくる. 他の同僚から学ぶこともある. そんなテクニックを, 思いつく範囲でいくつか紹介してみたい.
仕事のやりかたはプロジェクトによって様々だろうから, 思い当たる節の程度は人それぞれだと思う. 他にも思いつくものがあったらみな blog とかに書くのが良いですよ.
Commit Log Petition
ひとつめ. コミットログは嘆願書だと思って書け!
コードレビュー以前, コミットログやコメントの読者像は曖昧だった. 将来の開発者, 未来の自分? 未来人の望みを思い描くのは難しい. コードレビューがあると, 少なくとも一人は明白な読者がいる. レビュアだ. 実際, 多くのレビューツールはコミットログがレビュー依頼に含まれている.
そこでコミットログを <こういう変更を しました> というかわりに <こういう変更を したいんですけどよいかしら?> とお願いする手紙だと考えてみる. するとけっこう筆が進む. (時制のつじつまは適当にあわせてね.)
お願いするにあたり, レビューで聞かれそうなことは先回りして説明しておく. 綺麗に書けなかったややこしいコードには, 事情があってグチャっとしてるごめん, みたいな言い訳をする. 大きな変更の一部ならその旨を説明する. 遅くなりそうなコードならベンチマーク結果を添付する. といった具合.
不慣れな箇所を変更したとき, 普段は付き合いがないエンジニアにレビューを頼むときは, いつもより丁寧なコミットログを書く. 隣のエンジニアと話し合いながら書いたコードを話していた当人にレビューしてもらう時なんかは, ついあっさりした説明になりがち. マメさを問われる.
Fix Reservation
Commit Log Petition (あるいは Comment Petition) を補うテクニック. あとで直す予定の修正は, 対応するバグ/チケットをファイルしてコメントからリンクしておけ!
一度の変更でバグを直しきれないとき, リファクタリングをしたいけどチェックインを分けたい時は, その意図をバグとして記録する. そして該当微妙コードのコメントにバグへのリンクを書いておく.
これは FIXME や TODO の本気度高めバージョンだと思えば良い. 問題があるのはわかってる, あとで直すから今はチェックインさせろ. ほら, ちゃんとバグも登録したからさ. そう訴える. FIXME や TODO と書いて見逃してもらえる間柄のレビュアが相手なら, ならわざわざチケットを予約する必要はない. 相手との関係を見定めて使い分けたい.
Rubber-stamp / TBR
ものすごく自明な修正, たとえば特定の処理系でだけ出力される警告や互換性エラーを直すなんてのは, 本当はレビューせずチェックインすればいい. ただレビューされた記録がないとツールがチェックインさせてくれないことがあるし, そもそも <自明な修正> の線引きは曖昧だ.
そこで自明な修正をチェックインしたいときは, 手近なレビュアに “パッチにハンコ(Rubber-stamp)押してくれない?” と頼む. ハンコは <自明な変更への形式的なレビュー> をあらわす. IRC や IM で “Can anyone rubberstamp http://wkb.ug/99955 ?” なんて頼むと, ヒマなレビュアがハンコレビューをしてくれる.
自明な変更に対しては, レビューを頼む側だけでなくレビュア自身も “ハンコおしときました” と言うことがある. Mozilla 系列の文化では LGTM のことを r=me という. これをもじって, ハンコおしたぜという意味の rs=me というジャーゴンがある.
自明な変更にたいするイディオムには, このほかに TBR, あとでレビューしといてね (To Be Reviewed) なんてのもある.
これもたぶん Mozilla 由来で, Chromium でもよくみかける.
コミットログに tbr=morrita などとかいておくと, ツールがレビューのないコミットを見逃してくれる.
TBR や rs=me には官僚的なレビュー社会を生き抜く現代人の知恵を見た気がする.
Ping
待てど暮らせどレビュアーから反応がないときは, レビューツール経由で通知を送ろう. Ping はそのときの決まり文句. メールを読む相手の動体視力にひっかかりやすいよう, メッセージにはレビュアの名前を混ぜておくとよい. 例: “@morrita ping?”
Github の PR でもたまにやってる人がいますね.
Reset Thread
指摘を受けては修正するレビューのラウンドを何度も重ねると, レビューツールのスレッドがどんどん長くなり荒廃の色が深まる. 紛糾した言い合いを沈静化しているうちにスレッドの参加者も無闇に増えて, 議論を続けるのが億劫になる. 長いスレッドは人々の意欲を削ぐ.
そんなときは一旦レビューをキャンセルし, 新たにレビューを申し込み直そう. 長いスレッドが目に入らなくなると, 新鮮な気分でパッチを眺められる. レビュアも LGTM に耐えうる健やかな心を取り戻す.
関係者に失礼がないよう, 元のスレッドには新しいレビューの所在を報告すること. 議論を蒸し返される心配はすくない. 序盤で言いがかりをつけたきた通りすがりのレビュア達も, この頃には大抵みな飽きている.
Time Limit
望ましいレビュアがつかまらず, 代理レビューを頼まれた時の小技.
まず自分のわかる範囲でレビューを済ませ, それから “三日間反応がないようなら問題なしってことでコミットしますね” と宣言する. (日数はプロジェクトの慣習にあわせてください.) 義理を通しておきたい <望ましいレビュア> に宣言が届くよう, CC(Bugzilla, Rietveld) や @mention(Github PR) でお知らせしておく.
WebKit のように組織をまたぐプロジェクトだと, コードをレビューしてやるヒマはないけど特段ケチをつけたいわけでもない, という距離感はよくある. 逆に小さなチームで Time Limit 宣言が必要だとしたら, そのプロジェクトはどこか不健康かもしれない.
Side Channel Attack
反応がないレビュアーも困るけれど, 意見が多すぎるのも困る. 一番困るのは, 二人のレビュアが相反する主張をはじめたとき. お前らどっちでもいいからさっさと決めてくれよ…という気分になる.
そういうときはどうするの? ある同僚に尋ねたら, 決着がつくのを待つかメールか IM で連絡して説得する, との答えだった. なるほど. たしかにおおっぴらな説得工作は不恰好なこともあるからねえ.
Nitpick / LGTM with Nits
コーディングスタイルやタイポなど, 細々したところにケチをつけるときにレビュアが使う単語. みじかく Nit ともいう. “細々した指摘で悪いけどちょちょっと直しちゃってくださいね” と若干へりくだったニュアンスを出すのに使う. 例: “Nit: Return here.”
細かいところを直しては欲しいけど, それを確認するために余分なラウンドを重ねるのは面倒くさい. とんなとき, “細かいところを直したらチェックインしておくれ” という意味で LGTM with nits と言うこともよくある. レビュアはコメントと同時にツールの approval フラグをセットする. レビュイは細かい修正をした上で, 次のレビューを待たずチェックインする.
Embrace Nits
ある友人に WEB+DB PRESS を見せたら, “コードレビューとか何様のつもりだってんですか!” と憤っていた. 仕事のプロジェクトはさておき, ちいさなオープンソースプロジェクトにとってパッチや pull request は貴重な応援. コミュニティー形成への小さな一歩でもある. そんな寄稿をむげなくレビューで蹴るなんて尊大だ, というわけ.
この指摘は一理ある. 概ね問題のないパッチに細かい揚げ足とりをして寄稿者の手間を増やす道理はない. 相手の時間だって限られている. そんなときはお礼を言って PR をマージし, そのあと気になるところをささっと直せば良い. Github にホストされているような小さなプロジェクトでは, マージした PR をあとからメンテナが直す様子を目にする.
私も直されたことがある. 変なコードですまなかったと思いはしても不愉快ではない. プレコミットレビューに慣らされると忘れがちな感覚ではあった.
Small Change
基本にかえって, パッチは小さく!
１つのパッチ, コミットは意味のある範囲でなるべく小さくする. これはバージョン管理の基本だけれど, コードレビューのある社会では一段と有難味が増す. パッチは小さいほどレビューしやすい. なので小さいパッチほどレビュアの反応が早い.
Refactoring Trap
小さいパッチは, リファクタリングと相性が悪い欠点がある. リファクタリングは大抵パッチの差分を太らせ, レビュアの認知的負担を増す. この話は前にも少し書いた. このときは Gerrit や PR のようにブランチをレビューできるツールが解決になると思っていた.
最近は他のアプローチにも気づいた. それは, あえて全然リファクタリングしてないパッチを提出し, レビュアに “ここリファクタリングした方がよくない?” と言わせる方法. 指摘をうけてリファクタリングしたパッチは大きくなる. でもリファクタリングを言い出したのはレビュアだから問題ない. レビュアをハメろ! 誘い出せ!
実際, レビューをすすめるうちにレビュアがリファクタリングづいてしまいパッチがどんどん肥大化していく光景はよく目にする. レビュアが同じタイムゾーンにいて短いターンアラウンドでレビューできるなら, ペアプロミングのつもりで粗いコードを磨き上げるのも悪くなかろう. 普通にペアプロすればいい気もするけどね…
WIP
大きな変更を効率良くレビューしてもらう方法. 変更は作業中(Work In Progress)のうちからレビュアに見せておけ!
大きな変更は, 未完成の早いうちから人目につく場所においておこう. ただしチェックイン/マージできる状態にない書きかけであることを明示しよう. Bugzilla ならレビュー要求を出さずにパッチをアップロードする. Rietveld は… コメントに WIP とでも書いておけばいいんでないかな. 必要に応じて Reset Thread してもいい.
変更が興味深いものなら, 関係者は WIP のパッチをざっと眺め, 設計方針などに意見をくれるだろう. そうすれば間違った方向に深入りする前に路線変更できる. 手戻りが少なくて済む. WIP パッチに細かい指摘はいらない. デザインへのコメントに留め, チクチクやるのは準備ができてから. WIP イディオムにはそんな含意がある.
最近みかけた Bootstrap 3 の PR は, 大規模な WIP ブランチを運用する例として面白い. 開発者の @mbo は外野の意見を聞いたり聞かなかったりしながら PR 中のブランチにコミットを続けている. スレッドが賑やかで楽しそう. パッチ単位レビューにはできないスタイルが羨ましい.
Extract Ownership
がっつり書き換えたいコードはまず別クラスに切り出せ!
慣れないモジュール, 反応の悪いレビュアの守備範囲にあるファイル, 多くの開発者が目にするコアなクラス, そんな面倒なコードをいじらないといけないことがある. しかもちょっとしたバグ修正じゃなくて, 結構な規模の機能追加. その仕事がおわるまでずっと冷たい反応や厳しい視線に晒され続けるのか. そう思うと胃が痛む.
そんなときはまず新しいクラスやモジュールを作ろう. 件の衆目集まるクラスから新しいクラスを Extract Class し, Extract Method やら Move Method やらで 変更したいコードをそのクラスに集める. それから本来の機能追加を始めよう.
新しいクラスやモジュールは独立したファイルに収める. 最初のリファクタリングさえ乗り切れば, つづく変更は大半が新しいファイルでおこる. そして聞きなれない名前の新しいファイルをいじっても, 多くの開発者は気に留めない. 興味を持っているメンバー同士レビューして, さっさと開発をすすめることができる.
エンジニアの多くは, ある変更に含まれるファイル名からレビューに口を挟むかどうかを判断する. コードレビューのツールもファイル名にもとづいて変更の通知を送る機能を持つものがある. (WebKit の WatchList, Phabricator の Herald など.) ファイルはレビュアの <関心の単位> だと言える. リファクタリングによって, 自分の関心をより大きな関心から切り離すことができる.
新しいクラスを切り出す副作用として, 自分がそのクラスの持ち主だと識別されるようになる. レビュー依頼も切り出し前のファイルを見ていたエンジニアではなく, 新しいファイルを抜き出した側にやってきたりする. リファクタリングによって持ち場が生まれる.
誰かの目の前からコードを持ち去って隠すのにこの方法を使うべきでない. 一方で, 背中に感じる過剰な視線は God Class のようなまずいコードの <臭い> かもしれない. ある extraction の望ましさはレビューを通じて議論しよう. 不穏な変更が放つ臭いをレビュアは見逃さないだろう.
私もやりすぎて “そのわけわからんクラスは何なんだよ” と怒られたことがある. このときはたぶん, 私の臆病さがデザインを歪めていた.
Code Review as a ‘worse’
思いつく範囲のイディオムを列挙したみた. こんなしょうもない小細工をやりくりしつつ, 私達はコードレビューな日々を暮らしている. レビューめんどくせえな, と言いたくなる瞬間が時々あるのもわかるでしょ. そんな面倒はプロジェクトの歪みを知らせるシグナルかもしれない. けれどその原因をすぐ正せるとも限らないから, 対症療法としてこうした処世術には意味があると思う.
ところで, この pre commit/merge, patch/branch 単位のウェブベースなコードレビュー - 仮に Mondrian 風レビュー とでも呼んでおこう - は, このごろ幅をきかせている気がする. WEB+DB PRESS の記事もだいたいこのスタイルのバリエーションを想定しているように見える.
世の中には, たとえば inspection のようによりフォーマルなレビューがある. 他にもソフトウェア開発の成果物をレビューする様々の方法が提案されている. 場合によっては Mondrian 風レビューより効果が高いものもあるだろう. もともと差分中心のレビューには, 全体の整合性を見落としがちなど欠点もあるからね.
それでも世の中で Mondrian 風レビューが幅をきかせはじめているとしたら, 私は実際そんな気がしているんだけど, そこには何か理由があるのだろうか.
少し前に ”What ‘Worse is Better vs The Right Thing’ is really about” という記事を読んで以来, Mondrian レビューは “worse is better” の worse なのかもしれないと思うようになった. 色々欠点があり, 必ずしもいちばん効率的ではない. けれど始めやすく続けやすい. UNIX や C が持っていた感染力を, Mondrian 風レビューは備えていないか.
感染力を裏付ける敷居の低さの 1 つは, コミットやマージといったバージョン管理ツールのライフサイクルにうまく便乗した収まりの良さから来ていると思う. コミットやマージはソフトウェア開発の <リズム> だ. Mondrian 風レビューはそのリズムに乗っている. 凝ったレビュー手法の変拍子を学ばなくていい. Github の PR は マージ UI の一部にレビュー機能をつけ, リズムの乱れを一段と小さく絞った.
タイミングだけでなく, レビューするコードの大きさにも同じ事がいえる. チェックインやブランチは, 日々コードを書いているプログラマにとって馴染みのある粒度だ. Mondrian 風レビューでは, 手に馴染む粒度のコードが肌に馴染むリズムを刻みながらやってくる. それに加え, Git など分散 SCM が普及したおかげで pre-commit のレビューを待つ負担が下がったのも普及を後押ししている.
Mondrian 風レビューの持つこうした <馴染みの良さ> は, worse is better における <実装の単純さ> に対応しているのではないか. 先の記事は, worse is better の含意を 進化的な強さ と解釈している. この説明をコードレビューの文脈にあてはめてみる. 強い意志と訓練を要する <繊細で洗練された> なレビュー様式を, ツールをインストールすれば始められる <がさつで適当な> Mondrian 風レビューが食い荒らしてく. 割とそれっぽい気がしない?
なんてのはまあ, 完全な与太話. Mondrian 風レビューの感染力を示す大した根拠はない. でもそうだったら面白いと思うので, 来年も行方を見守りたい所存でございます. 良いお年を.
画像: