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

技術ブログの方に書くか迷ったのですが、かなりポエムの類な文章になりそうなのでこちらに書きます。

ちょっと前にバズったこちらの記事

medium.com

に触発されました。

ちなみにコードレビューに関する話としてはまだ僕が色々と手探りだった3年前にもこんなことを書いていたようです。3年前の自分の考えに触れられるブログって面白いなという気持ちとこいつどんだけ軽率な文章書いてんだよという気持ちが合わさり甘酸っぱい気持ちが生み出されました。

hachibeechan.hateblo.jp

当時と今では日本全体の技術的トレンドも変わっていますし、そもそも僕の所属している会社も違います。今の会社ではGitHubを使っており、コードレビューが当然のフローとして組み込まれています。

そしていま改めて当時のブログを読み返したのですが、びっくりするほどコードレビューに対する僕の考えが変わっていないので、改めて文章にまとめる必要があると感じました。

コードレビューで「コードのクオリティ」をあげようとするのはコスパが悪いだけではなく危険ですらある

これです。
言い換えると、コードレビューでやるべきなのはプロダクトそのものの品質を下げるようなコードがmasterに入るのを防ぐことではないか、ということです。

チームがコードレビューするにあたってフォーカスすべき要素はなにか

なんだかんだ言って、ソースコードの品質についてで一番重要なのは「問題なく仕様を満たしており、動作する」ことです。そしてその次の段階、「コードの綺麗さ」はサービスを効率よく運営し続けるという観点（それと働いているプログラマーの精神衛生）で重要になってきます*1。
そしてコードレビューで最優先するべきなのはバグやセキュリティ上の欠陥、明らかなパフォーマンス上の懸念など、プロダクトの価値を毀損しかねない問題をチェックすることです。次は単体テストのコードに問題がないことでしょうか。

これらはそれなりに高度な知的作業であり、本気でやろうとすると大変な時間がかかります。これには本当の意味でPRのコードを理解する必要があります。
また「コードのクオリティ」という言葉は意味が曖昧ですが、クリーンさという観点でいうと、この作業を通じてそれが向上することはないでしょう。逆に、セキュリティやパフォーマンスへの対策はおおむねコードのリーダビリティを下げる傾向にあります*2。

反して、「コードを綺麗に」するためのレビューは簡単です。なぜなら各レビュワーの知識ベースでコードの表面をなぞればよく、最悪コードの意味をあまり理解しなくても簡潔に書き換えることができます。

コードレビューシステムのインターフェースはだいたいそうだとおもうのですが、どうも「diffに対して行単位でコメントをつける」というUIの方式は表面的でnitpickなレビューを誘発しているようにおもえてなりません。
そもそもGitHubのあのdiffだけをみてコードレビューをするというやり方は間違っていると僕はおもいます。それが通用するのはよっぽど小さな修正や機能追加までです。コードの修正箇所よりも、その影響範囲が大きいことはざらにあります。

コードのクオリティをあげるために一番重要な要素はなにか

僕はコードの綺麗さを軽視している人間かというとそうではなく、どちらかというとプロダクトそっちのけでずっとそっちに手を入れていたい系統の人間だとおもいます。
そういう人種として、僕の拙い経験則から述べさせてもらいますと、コードレビューで改善できるのはせいぜいアルゴリズムのシンプルさだとか、このモジュール使うとコードの行数半分になりますよだとかその程度のことです。

先の"コードレビューにおけるレビュアー側のアンチパターン"のブログで言うならば、コードのクオリティの向上において最も重要なのは「意識高い系コメントパターン」のやりとりです。
そして、これら高レイヤな視点でのコード改善がコードレビューを通じてなされることはほとんどありません。なぜなら、指摘が理解できるのであれば設計レベルで書き直しが必要なコードはあまりあがってこないはずからです。そして、コードレビューのシステムはあくまで「指摘」のためのものなので、人に何かを教えるには向いていません。

例えばアプリケーションを作る際にもっともクリティカルなものはデータの設計です。一口にデータの設計といっても、データストアの構造そのものといったマクロなものから、アルゴリズムの中に登場するようなミクロなものまで色々とありますが、重要さはどこでも変わりません。
ダメなデータ設計というのは技術的負債の中でもかなり利子が高いほうになります。つまり早めに、できればコードがマージされるまえに修正するのが良いのですが、コードレビューの段階でそれをやろうとするとかなりのコードが書き直しになります。これは人の心を折る原因になりますし、スケジュールによっては全部書き直すような修正の時間をとることはできません。

コードのクオリティ改善、知見の共有はペアプロや勉強会のほうが効率が良い

完全にタイトルまんまです。
人は出来上がった作業をやり直すことに対して心理的に強い抵抗を感じますが、途中であればそれほどでもありません。また時間的な効率でいっても出来上がってから書き直すよりも細かく修正しながら作り上げたほうが良いです。
そして一緒に相談しながら書いていく作業は思考の過程も擬似的になぞることが可能なのでより学びが多いです。というかぶっちゃけコードレビューの過程で、より効率的な書き方や便利な標準ライブラリを教えたことがありますが、たいていの人は同じことを何回指摘したところで覚えてくれません。

じゃあコードレビューはなんのためにやるのか？

色々とネガティブな指摘が多くなってしまいましたが、コードレビュー自体はやらないよりはやるほうがいいものだとおもいます。ただし、上でも散々述べたように知識の共有のために行うのはメリットよりもデメリットが目立つようにおもいます。
むしろコードレビューはメンバー内である程度以上知識や設計の方向性についての情報共有が済んだ状態で行うのが一番効率が良いかとおもいます。

まず、コードレビューで行われがちではあるけどももっと効率が良い方法があるものについて、あらためて列挙していきます。

• コードのスタイルの指摘

  言うまでもなく最も無駄な行為ですが、ガイドラインなしでは最も盛り上がりがちなものではないかとおもいます。自転車置き場の議論はいつでも盛り上がるのです。
  これはeslintなどのスタイルチェッカーツールをCIでまわし、各メンバーにはprettierやEditorConfigなどの導入を強制することで代用可能です。この際、採用するルールはGoやPythonのように公式が存在するならそれを使い、ないなら世間でデファクトになっているルールを採用しましょう。なぜならコーディングスタイルというものは99%がゴミ（オレのルール以外は読みにくいからクソ問題）だからです。広く使われているゴミを使うのが手間が少なくて済みます。
  lintやフォーマッターが存在しない言語は……気持ちでカバーしましょう。

• 実装の動作確認

  基本的には自動化されたテストがあれば十分*3でしょう。

• コードの手直し

  これはペアプロでリアルタイムに行うほうが効率が良く、心理的負担も少ないです。人間は一度完成した作業をやり直すことを嫌うからです。
  ループの中でIOアクセスをしていて動作が重い……程度のレビューならコードレビューツールで簡単に手直しできるでしょうが、そういうコミットを頻繁にやるようなメンバーがいる場合は勉強会を開いたほうがいいとおもいます。

• Gitのコミットが酷い

  Gitのログの綺麗さについてはこだわる派と気にしねーよ派がいますが、僕は割とこだわる派です。仮にgit bisectしないとしても、例えば臭いコードをみつけたときにそこをblameで辿ってみて、必要のある臭いコードなのか無意味な臭いコードなのかをコミットコメントで判断したりするからです。特に僕は新しくコードを読み始めるときはgitのlogを読んでいく派（コメントと実装を照らし合わせると色々と雰囲気を読み取れるので）なので、コメントが雑だと初期の学習がしんどくなります。
  これもペアプロでやるか、メンバーが多いようなら一度ハンズオンつきの勉強会でもやるといいとおもいます。ようやく日本でもほとんどの会社が業務でGitを使うようになったとおもいますが、add, push, checkout以外はわからないと言う人が半数を超えています。「コミットの粒度が粗いから修正して」と言って自分で直せる人は極一部でしょう。

こんな感じでしょうか。とはいえ全部を一度に置き換えるのは難しいです。formatterの導入は既存のPRと大量のコンフリクトを生み出しがちですし、単体テストを書くのは（今まで書く風習がなかった人/プロジェクトにとっては）かなりの困難をともなう作業です。
自動化できないものは仕方がないので品質の担保を人力でカバーするというのはありでしょう。

以上、悪くはないけどコードレビューでやるにはあまり効率が良くないものでした。

逆にコードレビューでしか得られない利益もあります。

• ソースコードをチームで共有している感覚が得られる

  古いブログ記事でも書きましたが、コードレビューということで各PRに目を通していると程よい共犯関係とでもいいましょうか、責任を共有している感覚がうまれて作業の分担なども柔軟に行えるようになると言う効果があります。オーバーウォッチでたとえると最初に挨拶を交わすことで各自の気分が良くなり、柔軟なピックをしてくれる確率が高まって勝てるようになるかんじです。

• 「賢い」ダブルチェックを行える

  上で、昨日の動作確認は自動テストでいいやろということを書きましたが、テストコード自体の問題や考慮漏れを自動的に検証する実用的な方法は（まだ）存在しません。
  あるいは、E2Eテストは自動化されていても、それがUX的に許容できる範囲なのかどうかは人間の感覚でしかわかりません。
  これらはコードレビューで特にやる価値のある行為でしょう。
  ペアプロでも似たような効果は得られるのですが、一緒に作業している関係から客観的な検証は困難ではないかとおもいます。

二つしか思い浮かばなかった（かなしみ）んですが、人間がチームでやっていきりを発生させている現代の環境において得られるこれ以上の利益はそうそう存在しないでしょう。

まとめ

コードレビューは多くの利益を得られる素晴らしい仕組みですが万能ではありません。むしろ多くの人間を時間的に拘束するコスト、気持ちの統制がとれていない状況で発生させる手直しによるモチベーションの低下、負担のかかったレビュイーの疲労……など、多くの問題を発生させかねない諸刃の剣でもありえます。

まずは「品質の担保」というプロジェクト進行最大の関心ごとについてどういう優先順位で物事を進めていくかのコンセンサスをメンバーでとり、各人の技能が許す限りの自動化をするようにしましょう。
最低でもCIでPRごとの自動チェックが走らない状況でやらないとコードレビューで発生するストレスがメンバーの許容値を超えてしまう可能性があります。尤も、何も自動化ができてない状態でコードレビューもしないノーガード戦法をとっていたら遅かれ早かれそのプロジェクトは爆死するとおもいますが……。

次に、余裕のある会社であれば勉強会メンバー間の知識ギャップを埋める施作を推奨していくといいでしょう。アプリケーションであれば、最低でも各メンバーがフレームワークレベルでの構築ベストプラクティスをきちんと理解している必要があります。そこができていない状態でコードレビューをすると疲労が発生して色々なヒビがはいったりします。
ペアプロは余裕のない会社であってもできるので機会があったらやりましょう。

界隈によっては、コードレビューはクールでイケてる人たちのおこなう超エッジな儀式でやるとどんな悪魔も倒せる弾丸……というと言い過ぎですが何かしら憧れのような存在になってしまっている、そんな時期があったかとおもいます。今は知りませんけど。

しかし僕がここ数年で学んだ通り、そして色々な人がこぼす通り、コードレビューは利益もありますが、コストやリスクも存在します。というかコードレビューは人間の自然なセンスで実施すると間違ったやり方をしてしまいます。
これから導入しようとしている人、あるいは導入したけどうまくいかなかった人は今一度「なんのためにコードレビューをおこなうのか」というのを自問自答して、メンバーの知識格差、気質の違いを調べ直してみてはどうでしょうか。本当にコストやリスクに見合った効果が得られるのかどうか、そして危ういと感じたらコードレビューをしないという判断も全然ありだと僕はおもいます。

エンジニア向け転職サイトのソート項目の一つに「コードレビューを実施しているかどうか」というのが存在していたりするくらい「イケてる」要素の一つになっているものですよね。そんな空気の中でウチはんなもんやらねーよ！！ なんてことを発表すると炎上したりするのでやりにくいかもしれません。しかし適さないチームだって存在するわけです。うまくいかない仕組みに固執して雰囲気を壊すくらいならやめてみたり、別のやり方を探してみるのもいいんじゃないかなーとおもいます。

そんなかんじです。なんか意見あったらどうぞ。

追記

WEB+DB 102号がいい感じに関連する内容だというのをご紹介いただいたので読んだ感想です。

hachibeechan.hateblo.jp

*1:実際にはこうやって分けて考えられるほど簡単なものではないのですが……汚いコードは往々にして致命的な問題を隠しています。しかし説明を簡単にするために便宜上そうしておきます

*2:早すぎる最適化云々の話はここでは扱いません

*3:言うは易し