多くのプロジェクトでは、コードレビューという文化があり、それぞれ独自の文化をもって活動が行われています。今回は、コードレビューを円滑に行うためのコツについて考えてみることにします。
目的の明確化
何故コードレビューが必要なのか？
コードレビューを適切に行うには、なぜコードレビューが必要なのか、という理由を紐解く必要があります。そうでなければ、闇雲に「ただコードを眺めているだけ」という状態にもなりかねません。
コードレビューを行う理由は様々です。例としては、
- 知識の共有（属人化の防止）
- ロジックの齟齬、認識違いの防止
- 好ましくないコード（読みづらいコード）の防止
- 価値観・文化（カルチャー）の醸成
などが挙げられます。もちろん、プロジェクトによってコードレビューをする理由は様々ですが、「慣習だから」「常識だから」という前に、まずは理由を確認しましょう。理由のないコードレビューは、それ自体を避けるべきです。
目的に合わせたコードレビューを心がける
目的によって、適したコードレビューのやり方は大きく異なります。ロジックを重視するのか、コードフォーマットを気にするのか、あるいは単に属人化防止のために複数人でコードを読むためなのか。
コードレビューはコストのかかる作業ですから、出来るだけ無駄な作業は省きたいものです。たとえば、auto formatterがCIで回るのに、「ここはスペースひとつ空けましょう」などとコードの体裁を気にする必要はありませんよね。
そのコードレビューで何を保証すべきなのかを明確にしておかなければなりません。
レビューする側の心得
誠意を持って接する
コードは、書いた人が必ずいます。どんなに品質の低いコードであっても、書いた人が誰であっても、誠意を持ったレビューが必要になります。
「コードを憎んで、人を憎まず」とは言いますが、受け手としては分かっていても割と傷つくものです。読んでいる側が気分を害するような、あまり攻撃的なコメントは書かないようにしたいものです。
これ、なんでこういう書き方したんですか？普通に読みづらいですよね？早く直して下さい。
このような書き方はしないようにしたいですね。
宗教は捨てる
CUIのエディタがvimかemacsかでよく議論になるように、コードの書き方にも宗派が存在します。しかし、そんなものは敢えて提起したところで何も生み出しません。
const CONSTANT = "ConstantValue";
isConstantValue(value){
return value === CONSTANT;
}
こんなコードに対して、
条件文では定数は必ず左側に書いて下さい！
というコメントは酷いです。理由も書いてないですし、「いや、それってあなたの好みでしょ」としか思いません。こんなことを繰り返していると、レビュアーの信用も無くなってしまいます。
例外的に、全員が納得したコーディング規約に書いてあるなら、それを指摘するのは間違ったことではないかもしれませんが、たとえ気になったとしても、不毛な指摘はしないようにしましょう。
客観的に見る
コードは自由度が高いだけに、どうしても主観的に見てしまいがちです。たとえば、「これ、読みづらいなあ」と思ったとしても、それはその人だけの感想にすぎないかもしれません。
こちらの記事でも記載したとおり、時代の流れによっても「読みやすさ」の基準は変わります。それほどまでに不安定な根拠をもってしてレビューを行うべきではありません。
その場合は、なぜ読みづらいのか？を客観的視点で確認してみるべきです。「理由は無いけどなんとなく読みづらい」というのは根拠が弱すぎます。その指摘を受けた側も一体どうしていいかわからないでしょう。
先程のコードを例に取ってみます。
const CONSTANT = "ConstantValue";
isConstantValue(){
return value === CONSTANT;
}
うーん、returnに直接条件文を書くのは読みづらいなあ。if文を追加したほうが読みやすくなるよ！
const CONSTANT = "ConstantValue";
isConstantValue() {
if (value === CONSTANT) {
return true;
}
return false;
}
この例は、非常に意見が割れそうですよね。つまり、結局のところ個人の好みの域を出ません。
コードの読みやすさをはじめとした、チーム全体の価値観は、全員が納得した上で形成されるべきです。
「君の書いたコードよりこちらが提案したコードの方が明らかに読みやすいだろう！」というのは傲慢が過ぎるわけです。価値観や読みやすさなんてものは人それぞれが持っています。それより、より読みやすいコードを書くためにはどうすればいいか？読みやすいコードとは一体なんなのか？を念頭に、チームの価値観を形成する活動を行いましょう。
例えば、リーダブルコードという名著がありますが、それの読書会をしてディスカッションを行うことでも、ある程度の価値観を形成できます。そういった「基準」を持っていれば、「これ、あの本に書いてあったように、こうしたほうが読みやすいよ」といったコメントをつけることが出来ます。
テストを行わない
コードレビューは、「コードが正しく動いているか」を確かめる場ではありません。ローカルにコードをcheckoutして動作をチェックしながら綿密にレビューを行う人もいますが、そこでコードの正常性を確かめるのは、あまり好ましくない文化です。
なぜなら、テストはテストとして明確に行われるべきだからです。コードレビューでは、「テストコードが誤っていないか」を綿密にチェックするに留め、実際にローカルで動かしてテストするなんてことはしません。テストケースが足りないなら、追加をリクエストします。
もし、あなたのチームがテストコードを書く、あるいはテストを行う文化が無い場合は、テストを行う文化を醸成してください。
同様に、UIのレビューを同時に行う場合もありますが、これはスクリーンショットなどを添付すれば事足りるように思います。他、ボタンの動きやアニメーションをレビューすることはありますが、それはもはや「コードレビュー」ではありません。別の場を儲けてレビューするのがよいでしょう。
あえて絶対的に禁止するわけではありませんが、コストを掛けるならもっと良い文化を作る方向に掛けたいですよね。
レビューされる側の心得
相手に悪意は無いと知る
レビューの場では、たいてい自分の書いたコードを否定されることになります。しかし、気に病むことはありません。コードは否定されても、あなたを否定されているわけではないことを知りましょう。
文字のやり取りでは、表情や会話が伴わない分、冷たい言い方をされているように感じることがありますが、そのようなことはありません。
とはいえ、指摘箇所が多すぎると落ち込む気持ちはわかりますが、全てはコード品質を高めるため、と思って気にしないようにしましょう。レビュアーもそれほど気にしてはいません。
闇雲にすべてを受け入れない
もちろん、正当なレビューは素直に受け入れなくてはなりませんが、どうしても納得できないレビューコメントについては、議論をすべきです。レビュアーと直接話すのがベストですが、プルリクエストなどの場で議論しても構いません。レビュアーも時には間違えることもあります。
人間関係や上下関係などで、議論を行いづらいこともあるかもしれません。しかし、いかなるときでも遠慮をしていてはコード品質の向上には繋がりません。闇雲に、違和感を抱えたまま、言われたことをすべて言われたままに修正していくなら、レビュアーが直接修正したほうが早いです。
レビューはコード品質を押し上げる貴重な機会ですから、有効に活用していきましょう。
また、こういったやり取りの中で、チーム全体のコードに対する価値観が醸成されます。コードの読みやすさに対する認識が揃ってくれば、殆どそのままapproveされるようになることでしょう。
経緯や変更理由をしっかり書く
コード的に分かりづらいところはコメントを添えるのが一番ですが、プルリクエスト上では、なぜそういう変更をしたかを具体的に書いたほうがレビューがスムーズです。
変更理由が無いと、それを確認するコスト、負担をレビュアーに強いてしまうこともあります。事前に実装方針等が話し合われているなら問題ないかもしれませんが、タスクを進めて行く上で、実装方法を変更しなければならなくなった、ということもあります。
また、人は忘れる生き物ですから、以前話し合われた内容であったとしても、過度にコストが掛からない程度に最低限だけでも書いておくとより親切です。
まとめ
コードレビューは貴重なコミュニケーションの場です。ただ流すのではなく、積極的にコード品質の向上をチーム全体で意識することによって、より実りのある時間となるはずです。
今回の話に限らず、学びの機会は逃さないようにしていきたいですね。