コードレビューについて

このところ立て続けにコードレビューについて話をする機会があったので 私が経験した最高のレビュー体制を簡単にまとめておこうと思います。

• 利点 何故必要か 何が嬉しいのか
• コスト うまく回すためには何が必要か
• 細かい運営方法

はっきり言って当たり前の事しか書きません。 私も当時は当たり前のことだと思っていましたから、特に気にもしていなかったのです。 ただ見聞するところによると、これをちゃんとやっているところはとても少ないようです。 ウォールストリート系のファンドでもろくにレビューしてないとかどういうことなんでしょう。 だから時々会社が吹っ飛ぶんですね…

結局は、ああだ、こうだ各論を言っても、ちゃんとやれるのか、それ一点に尽きてしまう話なのですが…

利点

レビューを何のためにするか、それはまず第一に自分達の書いているコードに潜在するバグによる損失をできるだけ少なくすることでしょう。 型システムやフォーマルメソッドによる静的解析、自動テスト、特に QuickCheck などの網羅テストなどによる動的検査、事前仮想シミュレーションなど様々な方法がありますが、どれも一長一短がありこれさえやっていれば安心と言うことはありません。 特に金融系などの expect unexpected な世界の場合、これらの半ば機械化されたツールや手法にのみ頼ったプログラムの品質保証は、想定外の事態が発生した場合とても脆い。 もちろんこれらの方法は重要であり、コストの許す範囲で行うべきですが、やはり最終的には柔軟な発想を持つ人間によるプログラムの検査、つまりコードレビューが必要になります。 もちろんそれとてなんら完全な保証を与えるものではありません。最終的には運用時の冗長化や人間による監視が不可欠です。

レビューはバグを事前に見つけるだけでなく、お互いの書いたコードの批評を通してのプログラムの効率化や可読性向上、またメンバーのコード技能の伝達にもとても有効です。 良いコードの書き方というものはなかなか文書化しにくい、されにくいものです。 そのような文書が既に既にあればよいですが、やはりこういった職人的技能は職人間のコミュニケーションや、他の職人の仕事を見ることで盗むのが一番ではないでしょうか。 自分の書いたコードをレビューされ、欠点を指摘されることも大切ですが、 レビューのために他人のコードを読むのも、他人の技能を盗むよいチャンスです。

コスト

レビューのコストは高い、まずこのことを認識してください。 私の経験では本当に真面目なレビューを行うと仕事時間の30%はコンスタントにレビューに持っていかれました。 その間は外見的にはなんら新しいコードが作られないことになります。 つまりレビューなしにコードをひたすら書いている場合に比べ(狭い見方をすれば)70%しかアウトプットが無いことになります。 これはもし上層部がレビューに関するコストを甘く見ている場合、レビューは全く機能しないことを意味します。 誰が評価もされないようなことに時間を割くでしょう。 レビューしろと言われても適当に読んではいやりましたで終わりです。 それではレビューを行いソフトウェアの品質を上げるという目標はまず達成できません。

良いレビュー体制を築き上げるにはまずレビューにかかるコストを理解し、そしてレビューを行う利点、そしてプログラマがレビューを行うことを奨励しているということを積極的に周知する必要があります。 私の居た所では面接の時点からウチはコードレビューを重要視すると言われていましたし、入ってからすぐに、コードレビューは必要な業務であり、その際、コードを書いていないからといって会社は君の業績を軽く見たりはしない、ということを何度も言われました。

レビュー体制

VCS で行うレビュー

後に書きますがレビューは日々の業務の中の疑問や気づいた点の蓄積です。 ですからできるだけ気軽にレビューを行いそれを記録していけるツールが必要だと思います。 私が知っているところでは単に普段の開発で使っている VCS でそれを行なっていました。 つまり、コードの中にコメントの形でレビューを書き、それをコミットする。 そしてそこから派生する議論も全てコード上のコメントで行います。 (もちろん複雑な話になった場合は直接の議論を行い、合議の結果だけを記しておく、なども当然あるでしょう。) レビューと議論が終わった際にはそのコメントを消すか(いずれにせよ VCS に記録は残っています)、有用な情報だと判断されれば普通のコメントとして残されることになります。 この方法は VCS はまず皆さん使っておられるでしょうし、特別にレビューのためのツールが必要ないのが良いですね。

レビューのために bug tracker や task management system を使うのはあまり良いとは思いません。 レビューでは非常に細かい点が議論されることがあり、これが仕事のタスクの一チケットに相当するとはとても思えないからです。 例えば、この変数名は短すぎて良くわからない、といったことのために bug tracker をブラウザで開き、チケットを切る、やってられるでしょうか。 レビューは VCS に記録されていますから、コードベース上に現在どれくらいのオープンなレビューがあり、どれが緊急を要するかなどはレビューコメントに一定の書式を入れ、簡単な VCS の検索を行うことでほとんど実現可能です。

物理的にチームの人間が集まりレビューを行う、日本人が好きそうなこの体制、私は全く評価しませんし軽蔑します。 時間の無駄です。 既にそのコードを知っている人には退屈だし、そのコードを担当しない人には退屈だし、そのコードを書いた人にとっては糾弾会になる恐れもある。 小学校の時のおわりの会みたいなもんです。 シニアメンバーのジュニアいじめによるストレス解消くらいしか利点はないのではないですか。 人が集まって議論するのはもっと上のデザインでの議論などにすべきで、細かいコードの書き方などは日々のコードレビューで静かに切磋琢磨していけば良いと思います。 人々が頭を寄せ合って、一行一行だらだらと辿りながらでなければコードの個々の品質に関する議論ができないのならば、そもそもチームメンバーのスキル、つまり人選に問題があるのでしょう。

サイクル

私の経験したレビュー体制では三つのサイクル(というものでもないが)がありました。

• 日々の開発中に付け加えられていく臨機応変？なレビュー
• 開発コードから運用コードへの移行時の集中レビュー
• 日々のレビューが溜まった時の大掃除

こういうものにも何かそれぞれかっこいい名前が付いているのかもしれませんが、私は知りません。

日々の開発中のレビュー

他人のコードを利用、拡張、メンテしていると、あきらかに間違っていたり、書き方がおかしかったり、そもそもコードが理解できない場合があります。 こういった問題が自分がやっている仕事に現在形で関わってくる場合は、もちろんレビュー以前の問題です。 それを書いた人と話して問題点を解決し先に進みます。 しかしそこまで緊急を要さないが、コードを読んでいて気になった部分、というものが必ず出てくる。 そういった場合にレビューを書きます。

また、自分でコードを書いていて途中で自分のコードに自信が持てなくなった時にも自己レビューを書くことが推奨されます。 この時間違っても FIXME だけ、などという意味のないコメントは書くべきではありません。 他人にレビューを書くように自分にレビューを書きます。

リリース時の集中レビュー

上記の日々のレビューをちゃんとやった上で、さらに開発コードを実地に生産コードとして動かす際にはさらに詳細なレビューを行います。 開発のシニアメンバー(開発者の1/4位か)がレビューワーとなって、現行生産コードとリリースしようとしている開発コードの差分を全てレビューします。 各プロジェクトごとに担当レビューワーを複数(複数いないと議論になりにくい)配置し、その人達が全て差分を読み、レビューコメントがあった場合はそれを議論解決し、全員が差分についてオーケーを出して初めて生産コードに差分が移されます。 このことで生産コードは常にレビューワー達に全てコードを読まれた状態になります。 (もちろん、それが何か絶対的な保証を与えるものではありませんが) この差分のアプルーバルは確か何か小さなツールがあって自動的にメンバのアプルーバルの集計を行なっていたはずです。

私の経験した所ではパートナー(雇用側のエライ人)の技術系の人もちゃんとこのレビューは行なっていました。 これで会社としてレビューを重要視しているということをアピールできていましたね。 このようにレビューを重要視する会社ではありましたが、それでも書かれる全てのコードに対しこの集中レビューを行うことはコスト的に見合いませんでした。 そこで集中レビューはビジネスの中心の核となるコードのみとし、それ以外の周辺コードは上記の開発中レビューだけで回していたと記憶しています。

大掃除

日々の開発レビューが溜まってくるとだんだんコード上にレビューコメントが散見されるようになります。 これを放っておくのはあまりよくありません。 そこで、チームのリーダーは折を見てレビュー解消に集中せよ、という指令を出すことがありました。 この時は急ぎの仕事を除きメンバーはレビューコメントを消していく方向で仕事をします。 だいたいこの動きが出ると一週間(=5営業日)は解消を主に仕事をしていたような気がします。

レビューコメント書式

ここから先は技術的で瑣末なことです。 レビューをコメントとしてどう書くかということを紹介します。 コメントはいろんなプログラミング言語で違いますが、ここは # から始まるものとします。

レビューを書く

一番簡単なレビューの書き方は # RV <書いた人>: <コメント> です。:

# RV alice: Plz give a fancier name than `foo'!
foo = do ...

RV は誰でも何でも書くことができます。 変数名がわかりにくすぎる、ほとんどコピペされたコードが他にある、アルゴリズムがおかしいように見える、そもそも書かれているコードの詳細がわからない、コーナーケースが押さえられていない、などなど。

誰か書いた人がわかっていたり、誰か特定の人に問題を直してもらいたい場合、RV to : という書き方もしました:

# RV alice to bob: Fix this code!

RV の後にそれを書いた人のユーザー名を書きます。 これは VCS の機能(blame)だけではたまに書いた人の記録が追いにくくなることと、いちいちツールを使ってコメントを書いた人を探すのはめんどうだからです。 この名前はレビューコメント自動処理の際にも必要なタグになるので正しいユーザー名を書かなければいけません。 (確か変な書式のレビューコメントを抽出するツールがあったように思います)

自分が主に担当しているコード以外で、おかしいと思ったところはまず RV コメントをつけ相手の反応を待ちます。 明らかなスペルミスなど以外、何も言わずに勝手に直してはいけません。 相手が同意して直してくれるか、それとも合議の結果自分が直すことになるか、流れは場合によって変わってきますが一段レビューを挟むことが推奨されていました。

レビュー議論

さて RV コメントが作られました。どうしましょうか。勝手に消してはいけません! 誰かが、to で指定された人がいればその人は特に、それに対して答を書く必要があります。 レビューに対する反応は RV コメントに引き続き書きます:

# RV alice: Plz give fancier name than `foo'!
# bob: Why? `foo' sounds perfect.
# alice: It's just too anonymous.
foo = do ...

もちろん本来ならもうちょっと生産的な議論が行われるはずですが、割愛します。

レビューされたコードを直す

もしレビューコメントが付いたコードを直したり、そもそも何の問題もないと思われた場合、直した詳細とか、直さなかった理由を付け加えた上で、レビューが解決されたことを示す X を RV の前につけます:

# XRV alice: Plz give fancier name than `foo'!
# bob: Why? `foo' sounds perfect.
# alice: It's just too anonymous.
# bob: Ok, ok. Renamed `foomatic'.  <- bob はここで X をつけました
foomatic = do ...

もし二人以上の人が議論に加わった場合、議論した人の全員の同意をコメント上で取った上で X をつけます:

# XRV alice: O(N^2)? I am not sure but it should be better...
# bob: Reduced memory consumption greatly, it is still O(N^2) though...
# carl: Better algorithm in Nlog(N): http://...
# alice: Cool. I've rewritten it. Could you check?
# carl: Checked. Bob ?
# bob: Nice. Xed it.  (He added X.)

RV が XRV に成った場合、この全体の議論を終了するのは RV を始めた人です。 この議論全体を消去しても良いし(いずれにせよ VCS に議論は残っています)、もし後日にも役に立つ議論だった場合にはまとめてレビューコメントではない普通のコメントにして残しておくのもよいでしょう。

レビューコメント復活

もし XRV されても、それでもまだ納得できない場合は、 RV を始めた人、もしくは別の人でも構いません、は XRV を再びもとの RV に戻すことができます。もちろんその事を明記します:

# RV alice: Plz give fancier name than `foo'!
# bob: Why? `foo' sounds perfect.
# alice: It's just too anonymous.
# bob: Ok, ok. Renamed `foomatic'.
# alice: Sorry, `foomatic' still puzzles me. Removed X. <-- alice は X を消しました
foomatic = do ...

フューチャーワークメモとしての RV

普通は RV コメントは次のリリース時には全て解消されるべきものですが、そうではない、ここは直したり、強化すべきだがすぐにではないな、という点もコーディングではよく出てきます。そのために特別な RV コメントの書式があります。RV のように期日を付けていました:

## RV201302 alice: This code should be unified with function 'poo'

締め切りより大幅に前にこの RV を解消するのには当然何の問題もありません。解消には上で紹介したのと同じ方法 RV から XRV そしてコメント解消のステップを踏みます。

RV 管理ツール

上でも数度触れましたが、いくつかレビューのためのツールがありました。

• 頭の良い diff: これは開発から生産へのリリース時での差分を読みやすくするものです。少々のインデント修正などは無視されるようになっていました。他にも機能があったはずですが、よく覚えていませんし、使用するプログラミング言語にもよるので割愛します
• RV ã‚„ XRV をコードから探し出すツール。これはレビュー大掃除時に担当すべき人にお知らせを出すツール、などがありました。
• 勝手に RV が消されてしまうのを防ぐツール。これは簡単に思いつきますが、実際には使われていなかったような気がします。チームメンバーの良識を信じていたのか。

レビューワーに上下なし

そしてこれが最後にとても大切なことなのですが、誰もがレビューを書き、レビューに答える権利があります。上司だからといって萎縮するようなプログラマが居れば、それはよいレビュー体制とは言えません。例えば私はこんなことをやったことがあります。ここで私は alice ですね:

# RV alice: Hey, Who the hell did write this fucking code?
#           No comment and pretty badly written!!!
# david: Sorry Alice, it is another smelly pile of shit
#        of mine made years ago. Could you fix this plz?
# alice: Oops. Ok, boss

まあ日本では上下がアメリカより厳しいので難しいかもしれませんが、レビューの目的が完全にチームで理解されていれば上司部下、先輩後輩の垣根を超えたコミュニケーションは可能だと思います。 ちなみにレビューコメント例を日本語で書こうと思いましたが…難しいですね。 日本語は敬語のシステムが複雑すぎ、書く際に気を使います。 怖い上司に向けてお前のコードは読めない、という事を穏便に伝えようとする際、日本語でどう敬語を書こうかなどと考えていてはレビューの精神は半ば死んでしまう。 お互い下手な英語で生産的に罵り合う方が気が楽なのかもしれません。

結局は各人のモラルにかかっている

コードレビューにはやった証拠がありません。 読んだよと言えば読んだことになってしまいます。

最終的にはコードを読む人のモラルに、そしてそのモラルはコードレビューの価値とコストをチーム全体が理解しているかにかかっています。 チームリーダーや上司のレビューにかける鉄の意思を日々見せ続けることだけが、メンバーのモラルを維持し、レビューを成功させる秘訣だと言えるかもしれません。

だらだらと書きましたが、対して技術的に面白いことをしている訳でもなく、当たり前の事をいかにちゃんと普通にやるか、という話になってしまいました。 つまらなかったかもしれませんが…これをちゃんとやっているところがどこにある？ という話でもあります。あんまり無いみたいなんです。