前提

現在の僕の参加しているプロジェクトはこんな感じ

• Rails プロジェクト( AngularJS 使ったりしている)
• Git 使ってる( Pull Request ベースの開発で以下が merge 条件)
  • 2 人以上に approve される
  • テストが通ること(継続的インテグレーションの実施)
• 静的コード解析は導入している( Rubocop, jshint, pre-commit など )
• テストのカバレッジは計測していない(月一くらいで測ってるらしいんだけど、だからどうっていう話はない)
• プロダクトコードは既に結構ひどいところがある
  • 元々がひどかった
  • 全ての開発者が良いコードを書くわけではない
• コードレビューは PR 上でコメント突っ込む形で行われる(主に英語、日本人同士でちょっと複雑な話するときは日本語)

こんな感じの状況でやってるコードレビューです。まぁ参考程度に。

視点

• 静的コード解析でチェックしてもらえる部分は無視する*1

Rails だと if statement での assignment とかそんな話は基本的に無視出来る。 JavaScript なら equal じゃなくて strict equal を強制したりとかね。
静的コード解析をプロジェクトの最初から導入していて、最初からかなり strict な設定にしている場合、結構楽だと思うけど途中から導入した場合まだ設定を入れてなかったりオフにしてたりするのがあって人力でチェックしないといけないものもあるので、出来るだけどんどん進めて行きたい感じはある。
ただ、静的コード解析で例えば length 140 まで良いからと言っても、読んでいて長いなって思ったらどうにかしてって頼むときもある。

• 明らかに意味のないコードはバッサバッサ指摘する

なんで if statement で boolean 値と比較すんの、とか。一見間違いではないし動くけど、それ意味なくない?みたいなのとかも指摘する obj.present? とかは obj だけで nil 判定出来るじゃんとか*2。

• 言語の文化に沿った書き方をできているか

num == 0 とかって書いてたら num.zero? でいいよね、とか、 is_foo は foo? て書いたほうが良くない、とかたまによくある。

• 直感的に理解出来るコードか

よくあるんだけど for-loop, each とかで所謂 find してると指摘する。
直感に反する系だと && と || がめっちゃ連なってるのに括弧がないとかも指摘対象(いちいち優先順位気にして読めない)かなぁと。

• 悪いコードのコピペを止める

既存のコードでダメコードがわりとよくあって、それをコピペしたり移動させたりといったことをしていたら、指摘の対象になります。悪いコードの統一性を保つより、少しずつ良いコードにしていきたい感じはある。

• パフォーマンスの問題を気にする

これはあまり僕は気にすることがないけど、よく話が合わないときがある。だいたいここは気にするよりも可読性の方が優位だって説得するけど( Pure JavaScript 大好き狂がいて、 underscore.js を嫌がる傾向がある)。
あ、けど Ruby で 2 重にイテレータ書いているとことかは気にする。この辺参考になる。
Writing Fast Ruby // Speaker Deck

• テストコードが何をテストしているのか

たまに意味のないテストコードがあるので、そういうときは指摘。もう少しここは成長余地ある。

• プログラミング的な誤りよりはその issue に対するアプローチが正しいかをチェックすることを気にかける

ついプログラミング的なことにばかり気を取られるけど、設計やアプローチが正しいかも合わせてチェックする必要がある。

コードレビューのメリット

• ジュニアエンジニアの成長機会がある
• エンジニア同士の意識を合わせる機会になる
• 単純なバグを防ぐワンチャンある
• コード品質がちょっと上がる

結構僕はレビューしてガンガンコメントするから、レビューオジサンみたいになってしまってる。結構レビュー重ねるごとに、まぁ言うことないかなーみたいな PR 増えてきて大変良い。あとはちょっと品質あがる。

コードレビューのデメリット

• first approve を押したがらない
• イジメみたいになる
• 地味にストレスたまったりする
• 時間かかる

僕がレビューオジサン業やってて、結構コメント入れるから何人かのチームメンバーは最初に approve を押したがらない。つまり、ツッコミどころがある PR を approve していると思われるのが嫌なんですね。僕も嫌ですけど。だからこの辺、他のチームメンバーが萎縮しちゃって良くないなと思うところ。
あと何度レビューしてもなかなか伝わらない人もいるもんで、毎回同じ指摘せざるを得なくてイジメみたいになってしまう。つらい。指摘する方もつらい。
ストレス、たまります。こうなってるの多分ダメなケースなんだけど、改善方法分からなくて困ってる。なんでこの人はまたこんなクソコードを書くの…とか、コピペ止めろって何回言ったら分かるの…とか、って状態になると辛いですよね。

コードレビューでの心掛け

• キレない/怒らない
• 褒める
• 改善案を提示する
• 人格を批判(攻撃)しない

いや、結構大事です。大事ですというか、心掛けてやらないとたまに本当に叫びたくなる*3。褒めるっていうのは良い仕事したね！って認めてあげることで個々人のモチベーションにも繋がりますし、気持よく仕事していくためにも大事だと思います。最近は LGTM.in/g とか試したり、顔文字を入れていいね！とか書いてます。
あと、こうしたら?とかこういう考え方はどう?とか短いコードをよく書いて提示してあげます。それは考えたけどダメだったんだよ、というやり取りが生まれたりしてお互いに納得行く感じになります。人格を批判(攻撃)しないのは大事です。先入観なくコードだけをみて、なんでこう書いたの?とかいいコードだねっていう話を出来るようになったら大人です。大人になるのは難しいですが、ダメなコードをダメっていうのとダメなコードを書くからお前はダメなんだっていうのは意味が全然違うので、そこに気をつけるだけでいいのです。まぁ現実問題として、クオリティの低いコードを確実に毎回書いてくる人というのはいるもんだけど、そのへんを底上げしていく努力はしなきゃいけないし、そのためのコードレビューだよなーと心を落ち着けています。

コードレビュー文化の前にやった方がいいこと

• 何が綺麗なコードか
• 何を重視するか
• 責任意識を持ってもらう

何が綺麗なのかというのを論理的に説明できないと結構辛いですね。好みの問題だよねーになっちゃうとあれーみたいな。ガード節使ったほうが読みやすいよねーとか、ネスト浅くしようねーとかも、保守性が高いとか低いとかで出来るだけお互いが考えてケースバイケースで考えてくれるのが一番なんだけど、なかなかそうもうまくいかなくってうーん。
あとパフォーマンスを取るべきか可読性/保守性を取るか、トレードオフになる関係のものをどっち優先する?みたいな話。よく JavaScript 大好き台湾ガールとここでモメてた。チーム内での基準が統一出来てなくて、 underscore.js 使ってよっていうとパフォーマンスがーって言われるみたいな。どう見ても 2 重ループ書いててそこの意図が取りにくいって感じなのにパフォーマンスがーって言われて、最終的には可読性優先でいきましょうって落ち着いたけどそういうとことかちゃんと決めないとモメる。てか経験上つらいので、この辺は明確に基準を決めた方が良さそう。
それからこれ一番大事だけど、自分の触ってるコードは自分だけのものじゃなくてチームの資産であって、同じように他の人が書いたり修正しているものもその人が書いたコードだからその人のものというわけじゃなくて自分たちのチームの資産だから、お互いにちゃんとレビューして品質を上げていこうねっていう意識づくり。これ言わなくても分かってくれてるのが理想だけど、現実はそうじゃないのでがんばりましょう。

まとめというか余談

コードレビューやってるとチームメンバーで「この人こういうの弱いな」とか「この人ちょっと古いコード書くな」とかそういう癖みたいなのが分かってくるので、そのへんは面白さがある。あと、自分じゃ思いつかないような解法持ってきたりして素直に驚くときとかあって良い面も結構多いです。

まぁただ、よくコードレビューのある文化を良いなと言う人たちはいますが、実際にコードレビューのある文化で働いてみてこれはこれでしんどいなって思ったりします。コードレビューを羨む人たちの多くは自分がレビュイーとして、レビューをして欲しいからレビュー文化を羨むことが多いのだと思いますが、現実問題自分がレビュイーになれない環境では多少つらい思いをすると思います。レビュイーになれないというのは、例えばコードレビューを良くしてくれる人だというポジションについてしまうと「ああ、あの人が書いたコードならそう間違いもないから大丈夫なんだろう」という風になってしまい、あまりレビューをしてもらえなくなります(簡単に approve を押されてしまう)。あとは普段レビューしていて指摘するのは自分自身の考えや思想とかを元になっているわけで、自分自身はその考え方に基づいて 100% 確実に書けるわけです。ということは普段自分からレビューを受けて成長している人たちからしたら、そのコードが「模範」となってしまい指摘する余地がないように見えるのですね。

それでも、レビューをする価値はあるし、コードレビューを否定するつもりはないのですが、もう少しどうにかしたいなーと思う今日このごろ。

理想のコードレビューはお互いの思想や考え方をぶつけて話し合うとか、いいコードって何よっていう話をもっと具体的に掘り下げていくことが出来るようなものなんだろうけど、そこまで持っていけるだけのメンバーがいないと正直つらいのかなと。コアレビュワーがいてその人たちに見てもらうレビュイーたちは確かに成長機会あるけど、コアレビュワーは成長しないとかそういうのはあんまり良くない。お互いがお互いにレビュワーであれる、そういう健全な状況作りを目指したいですねって最近よく思います。

と、だらだら書いたところで長くなってしまったのでこの辺で締めときます。