コードレビューで気をつけていること

Chromium Code Review Advent Calendar 2017の25日目の記事です。参入障壁が低かったのでなんとなく書いてみました。興味のある方は充実の本家Chromium Browser Advent Calendar 2017も参照下さい

はじめに

オープンソースのウェブブラウザ Chromiumでそこそこ長く開発をしてるので、自分や周りの人がコードレビューで心がけていることを書いてみました。良いコードとは何かという話はまた別の長い議論になるのでここではとりあげません。

基本的に、コードレビューはコミュニケーションと思っており、うまくやることで開発効率をあげたりコードベースをより良くしたりできる可能性があると思ってるので、そんなことを書こうかと思います。なお、目指しているだけで、必ずしもいつもできているわけではないです

以下、唐突にである調で。

レビューレイテンシを減らす

レビューを待ってる間、コードを書いてる人はそのコードの開発をある程度止めないといけない。なのでレイテンシはなるべく一定以上にしない。時差がない人のレビューは特に（逆に時差がある人のレビューは相手が起きてない時間なら少しゆっくりやる）。ただし頻度をあげすぎると自分の生産性に影響するので注意。それでもレビューを早めるとチーム全体の生産性はあがりやすい。

逆にレビュワー１人遅いだけでプロジェクト進行が進まなくなるのはざらなので、プロジェクトのコストを見積もるときもレビューレイテンシは必ずリスクに入れる。

レビューが終わってなくてもコメントを送る

レイテンシを減らすといっても、一定の質を保とうとすると一日で終わらないことも多い。また、大きな変更でその日のうちに詳細なレビューをするのが大変な場合もある。こういうときは、少しだけでも見て「残りは明日見る、ごめん」と言い足して気になったところや先に見た分のレビューコメントだけでも先に送ってしまう。

後からさらにひっくり返すコメントをつけなければならないこともあるが、相手にレビュー状況がわかるので、レビューが返ってくるまでの時間を予測しやすくなる（→その間にどの作業をやるかなどの見積もりがしやすくなる）。あと、何度か質問しながら繰り返してレビューした方がこっちの理解度もあがる。

ちなみに、パッチを書く場合も、大きな変更だったらテストや細かいところを調整する前にざっくり書いたコードをレビューに出して、「まだテスト書いてないけど全体の方向性どう思う？」みたいに意見を早めに求めるのも良くやる・見るし、有効なテクニックだと思う。

時差にあわせてレビュー頻度やコメントを調整

時差が大きい人のレビューはレイテンシが大きいのが前提なので、コメントもなるべく「もしこうだったらこう、でもそっちだったらこうして」のように複数の場合へのコメントを全部書いておく。逆に自分がレビューを頼むときも、「コードではこう書いたけどこういう選択肢もあると思う、その場合◯◯はこうなる…」みたいな他のオプションをあらかじめ提示して、合意が取れそうなデザインをなるべく少ないやり取りでさぐる。

……と言いつつ、時差があるとやり取りが長引くのは避けられないのでそれはあきらめる。違う作業を並行させてやって全体のスループットをあげるのが最も効果的。

わからないことは質問→わかったことはなるべく共有

意外と書いてる方もわかってないことがあるので、「そもそもなんでこうしてるんだっけ？」みたいな質問は割と有用。これをするときは、どれくらい知ってるコードかにもよるけど、「この辺のコード詳しくないので当たり前のこと聞いてたらごめんね」みたいな一言をなるべく添えてる。あと、後からコードを読んだりレビューをしたりする人が同じ質問をしないで済むように、それでわかったことについてはコード中のコメントやバグの説明になるべく書いてもらう。

なんかイヤだなと思ったらがんばって言語化

どこか良くわからないけどなんかぼんやり嫌だなというときがある。こういう感じがあったら自分が思う良いコードのイメージとどこかがあってないことなので、なるべくがんばって言語化して解消する。間違ってはないからまあいいかと思って通してしまうこともあるが、後で後悔することも多かった（うまく言語化して指摘できなかった自分の能力不足を悔やんだり）。こういう場合、レビューの最初はデザイン的なコメントや質問になることが多いので、それだけを書いて細かいコードレベルのレビューは後回しにする。質問を積み重ねてやっと問題が見えてくることもある。

欠陥の指摘だけではなくなるべく代替案を提示

相手のコードやそのデザインがどうも気になる場合、欠陥の指摘だけを書くのではなく自分が良いと思う代替案を出し、元の案と代替案との長短をなるべくはっきり提示する。複数人がどんどん代替案を出してより良いコードになっていくようなレビューもよく見る。指摘や質問ばかりだとなかなかいいコードに落ちていかずやり取りだけが長引くこともあり、お互いイライラする。

レビューコメントの意図を書く

割と些末なコメントでも、例えば後から読む人や将来の自分たちのための可読性のためとか、こうするとここの変更がしやすくなるからとか、理由があるときは必ずそれも書く。その理由により良くマッチするさらなる代替案を向こうが提示してくれる可能性もあがる。

主観的なコメントは決定者をはっきりさせる

スタイルの好みなど、主観的なコメントや意見がわかれてるものについては、そのことを明記して、最終的には相手に選んでもらう。もしくは、その辺のコードのオーナーになってる人がいれば、「最終的には彼にまかせる」みたいな風に決定者を決める。（コンポーネントごとに多少はポリシが違うので、コンポーネント内での一貫性を保つ方に倒すほうが混乱が少ない→無駄に生産性を下げない）

どのレビュワーに何を見てもらいたいかはっきり書く

複数のレビュワーに頼む場合、誰にどのファイルを（特に）見てもらいたいかはっきり書く。また、なるべく主要なロジック部分のコードのオーナー（ステークホルダー）に一番最初にレビューを頼み、それが終わってからそれ以外のレビュワーを追加する。（ステークホルダーが入れたくない変更だった場合、レビューが無駄になることもあるので）

TODOを活用し、無駄にブロックしあう依存を作らない

それまでのデザインの仮定と違う実装になってきたとき、綺麗なコード変更をするためには大きなデザイン変更が必要になることもある。割とよくある。それでそれを直すためにはこっちも直さないとだしまずこれ議論しないとだし…みたくなって深い泥沼が見えてくることも良くある。

こういう場合、どれだけそのパッチを入れたいかにもよるけど、それ自体はなるべく副作用の少ない小さい変更にして入れてしまい、デザインの議論は別に並行して続けることも検討する。また、可能であれば細かい変更に分解して少しずつ入れる。こういう場合、個々のパッチが入った状態でも常にできるだけ矛盾が大きくならないようにする。

ただし必ずTODOコメントはつけてもらい、必要であればバグを立てたり議論をMLではじめたりはパッチを入れる前にやってもらう。なお、TODOは必ずしもその人がなおさなくてもよく、誰か別の人がやってもよい。書いても僕は後で直せないよ！て言われても書いてもらう。直したいコードであるということとどう直したいのかを他の人と共有できるようにしておくのが重要。

コードの変更、コードの改良、デザイン議論のバランス

上に関係するけど、度重なる開発でぐっちゃりになってしまったコードというのは Chromium みたいにそこそこメンテされているコードでも良くあり、それを全部直すまで新しい開発をしたくない気持ちになることもある。コードレビューはその辺のジレンマがダイレクトに発露する場であり、なかなか興味深い議論が日々無数に繰り返されている。全体の開発フェーズや影響力の強い人の旗振りなどによって機能追加だけとかコード改良だけとかを優先させることもあるけど、レビューやMLの議論を元にみんなで淡々と進めてくものもたくさんある。いろいろ考え方はあるけど、私は基本的には「コードの変更」「コードの改良」「デザイン議論」のうちどれか１つだけを長くやってその他を止めてしまったりそれに依存させてしまうのは割けた方がいいと思っている。あるチームがどれかだけをやることになったとしても、コードベース全体では忘れずにあちこち耕しながら前進させていく。規模の大きいコードベースは大体どこもそんなものかと思う。

終わりに

あまりまとまってませんがどこんな感じですかね。他にもいっぱいあると思いますが "Code review best practice" みたいな良い文書はたくさん落ちてますのでこの辺で。

Happy reviewing!