2018.02.20

コードレビューは差分以外を読め！

shimo

ちょっと煽りっぽいタイトルをつけてしまった下條です。ごめんなさい。
最近はてなブックマークに上がっていた記事

そろそろコードレビューそのものの必要性について考えるときがきているのかもしれない

を読んで、弊社内でのコードレビューが効率的に機能しているのか、よりよいコードレビューやペアプログラミングのやり方がないか考える良い機会となりました。

どうも「diffに対して行単位でコメントをつける」というUIの方式は表面的でnitpickなレビューを誘発しているようにおもえてなりません。

これには私も同感する部分があります。

昔話になりますが、以前の会社では修正したソースコードのファイル全体を紙に印刷して対面レビューを実施するということを実施していました。その後現在の会社に入ってGitHubやBitBucketなどでのプルリクエスト (PR) レビューになった際に若干の違和感を感じた記憶があります。というのもGitHubやBitBucketなどのPRインタフェースは、diffとその周囲わずかのみを見るだけのインタフェースになっているためです。実際、紙に物理的にプリントするかどうかは重要な問題ではないのですが、コード全体を見ることで思わぬ場所への影響だったり修正が不足している箇所が見つかったりすることが多くあります。つまり修正の十分性を判断する材料が多いということです。しかしGitHubなどのPRでの限定的な箇所のdiffを見るだけでは修正の十分性の判断が難しいことがあります。当時感じた違和感はそこにあったのだと思います。もちろんいずれにせよ全ソースコードを毎回見ることはできないので程度の問題になるかとは思いますが。

ただし、

そもそもGitHubのあのdiffだけをみてコードレビューをするというやり方は間違っていると僕はおもいます。それが通用するのはよっぽど小さな修正や機能追加までです。コードの修正箇所よりも、その影響範囲が大きいことはざらにあります。

これについては間違っているとまでは言えないと思っています。当然ながら影響範囲がdiffの範囲外に及ぶことは多々あるのですが、diffの範囲外の部分をレビューアが前提として認識していればdiffを見てのレビューも機能すると思います。

私の大学院時代に指導教官から
論文は行間を読め
と常々言われており、最初はなんて寝ぼけたことを言ってるんだと思っていましたが、実際そういう側面は大きくあります。実験手法の詳細だったり、大事なことが実は敢えて書かれていない場合も多いです。
ちょっと強引ですが、見えない部分を見るという意味で、コードレビューについても似たようなことが言えるのかもしれません。diffをバカ正直に見るだけでは見えてこないものがあります。その背景にある膨大な修正されていない箇所が重要なのです。
そういう意味では
コードレビューは差分以外を読め
というのが類似の格言になるのかもしれません。

ちょっと話が逸れましたが、つまり、GitHubなどの限局的なdiffでの比較インタフェースには以下のような前提が含まれていると感じています。

レビューアが、コードや機能の全体 (少なくとも概要) を把握しており、修正内容の必要十分性を判断できる。

こういった前提のもとではPRを利用したコードレビューも比較的うまくいくのではないでしょうか。
逆に上記前提が成り立たない場合はGitHubのPRの非同期なコードレビューは上手く機能せず、先に引用したブログのように、コーディングスタイルなどの表面的なレビューを多く誘発することになります。 (むしろそういう部分以外に指摘をすることが難しい。)

したがって、