コードレビューの品質を上げよう

コードレビューをもっと良くしようという経験的な記事です。

コードレビューの目的

コードレビューの主たる目的は、コードベース全体が常に改善し続けていることを確認すること にあります。どんなツールもプロセスもこの目的のためにあります。

改善しているかどうかは次のような指標で測る：

原則としてレビュアーは提案された変更がコードベースを改善することがわかったらapproveします。完璧を求める必要はなく、継続的な改善になっているかが鍵です。

意見や個人の嗜好よりも技術的な事実やデータ
スタイルに関してはスタイルガイドのみが権威を持つ。スタイルガイドになければ個人の好み。スタイルはそこで一貫しているべき。もし前例がないのであればauthorを受け入れる
適切なコメントを残す。言語やフレームワーク、ソフトウェアデザインの原則を学ぶ重要な機会でもあるので、コメントを残し、知識を共有することでコードヘルスは良くなる
揉めたら実際に会うかビデオで話す。また、会話の内容は記録しておく。解決しない場合はチームでの議論にする

コードレビューでは、フォーマットやスタイルの議論に焦点が当てられ、想定通りの実装になっているか、パフォーマンスが良いか、既存実装に影響がないかなどの重要な議論が蔑ろにされがちであると言ってます。

この問題を解決するために、コードレビューで大事にされるべき観点をピラミッド形式で表現した図が公開されています。

コードレビューピラミッドは、あとで変更するコストが大きいものから小さいものまでレイヤ分けされています。各レイヤで投げかけるべき質問の例もあります。さらに、どの範囲までを自動化して、どの範囲にコードレビューの時間を使うべきかも示されています。

主に、仕様を満たした実装であるかを見ます。境界条件を確認したり、実際に動作確認を行うなどのレビューをします。

可読性（リーダビリティ）とは、つまり読みやすさのことです。一般的な可読性から開発言語に特有の書き方などのレビューです。

設計レビューと呼ぶのが適切かは分かりませんが、コードの責務が適切か、既存の実装に不都合な実装になっていないかなどをレビューします。将来的な拡張性もレビューできると良いでしょう。

コードレビューで最も重要なのは全体のデザインです。

それぞれの変更は意味がありますか？コードベースに対する変更ですか、それともライブラリの変更ですか？システムの他の部分とうまく連動しますか？その機能は今追加するべきですか？

その変更は想定通りの機能を果たしていますか？
開発者がユーザにとって良いと思っていることはなんですか？ユーザはエンドユーザであることもあれば、今後コードを使うであろう未来の開発者であることも。

開発者が十分にテストしていることが望まれますが、レビュアーとしてもエッジケースに注意し、並列処理の問題がないか調べて、ユーザの立場で考え、コードにバグがないかを調べる必要があります。

UIに変更がある場合などは、実際に変更が合っているかを確かめることもできます。どうやって確かめれば良いか分からない、あるいは簡単には再現できない場合には開発者にデモを用意してもらって確かめましょう

もうひとつ重要なのは並列プログラミングの問題です。デッドロックやレースコンディションがある場合には念入りにチェックします。しかしこうし問題は発見が難しいのでできれば避けたほうが良いでしょう

仕様を把握していて実装するときには気付きづらいですが、「あとから読んで理解できるか」、「仕様を知らない第三者が読んで理解できるか」、そして「すぐに理解できるか」を気にします。

変数や関数名が適切かです。

あまりに抽象的な命名だと、どういう目的の変数や関数なのかを見失ってしまいます。悪い場合には他の変数と重複してしまうことも。

命名する際には情報量を多く、できるだけ省略しないようにするのが良いでしょう。

再利用性を高めたり、バグを減らすことができます。

一つの関数が責務を負いすぎていないか、不適切な責務まで担っていないか、採用しているフレームワークなどと相談して適切に分割されるようにします。

一度処理してしまうと取り返しがつかないもの、メールの送信や決済などは念入りに。動作確認やテストは十分だろうか？
もし設計変更があったときに変更しやすいだろうか？

まずは基本的に攻撃的にならないように心がけましょう。コードを書いた人にはこだわりがあるかもしれないことは念頭に置いた上で、議論をします。

また指摘する内容の重さも伝えると良いと思います。必ず変更するべきなのか、提案なのか、それとも疑問なのか。自分は質問する形式にすれば角が立ちにくいと思っています。

MUST – 必ず直してほしいところ。どう直して欲しいのかも具体的に書く。理由も。メンテしづらくなったりパフォーマンスが悪かったり。
IMO – In my opinion。「自分ならこう直す」というような提案型のコメント。メリデメが両方あるような場合のものに、具体的なコメントをつけて書くと良い。
NITS – 直しても直さなくてもいい程度の修正につける。
ASK – 実装ないようがわからないとき。
GOOD – 実装が良いときに書く。気分が良いので。