コードレビューをするときは目的を明確に


slide23

シーエイダブリュウでは、コードレビューを導入しています。Githubのpull requestの仕組みを活用して、マージするときに必ず自分以外の目を通すことにしています。

なぜコードレビューをしているのか?
コードレビューは一般的に「良い文化」とされていますが、実際にやってみるとそれなりの時間的コストを消費することになります。ただやみくもに導入してしまうとただの時間の浪費になってしまう可能性があるのです。

なのでレビュワー・レビュイーともに事前に下記の目的をしっかり理解してもらって、より価値のあるコードレビューをするように心がけています。新しいメンバーにも必ずその目的を伝えるようにしています。

コードレビューの目的

  • 常に一定の保守性を維持する
  • メンバーのスキル向上
  • 担当分以外の仕様の把握

常に一定の保守性を維持する

これが最も重要な目的としています。

「良いコード」「悪いコード」って何でしょうかね?
深い話なので、あんまり言及してしまうと、夜があけるどころか気がついたら浦島太郎のごとく老人になってしまう恐れがあるのでほどほどにします。

ぼくは「意図が明確で、見通しが良くシンプルで、他人にとって理解しやすいコード」が「良いコード」で、その逆を「悪いコード」としています。

「悪いコード」の代表例として「スパゲッティコード」というものがあるんですが、複雑に入り組んでしまって、どことどこが絡み合っているか想像もつかない状態っていうことですね。下手にいじくるとあらぬ方向にトマトソースが飛び散ったりして後始末が大変ですよ。

こんな状態に陥ってしまったら最後、他人がメンテナンスすることが恐ろしく困難になります。どれくらい困難かというと丸腰でデスピサロに挑むくらいには困難です。

それでもやらなきゃいけないときどうするか?どうにか正しい挙動にするためにスパゲッティーを更に絡ませるんですよね。こうしてスパゲッティーが量産されて行くわけです。気付いたときには時既に遅し。そこに待っているのはデスマーチしかありません。絶望の深淵です。

システムというものは規模が小さいうちは容易にシンプルに書くことが出来ますが、規模が大きくなっていくのに比例して徐々に複雑になっていきます。それを完全に防ぐことは出来ませんが、スパゲッティーが入り込む隙を減らすことなら出来ます。そのためのコードレビューです。

シーエイダブリュウでは下記のような観点でコードレビューするようにしています。

  • メソッド名・クラス名・変数名がその役割を的確に示しているかどうか
  • 必要のないコードや、使用していないメソッドがないか
  • コードがDRYかどうか(コピペコードがないかどうか)
  • コードが本来書くべき場所に書いてあるか(Fat Controller問題とかそういうの)
  • ユニットテストを通しやすいか

注意が必要なのはコードレビューをやり過ぎないこと。例えば、「ifの後に半角スペースを入れるかどうか」「長いコードの改行位置」「括弧を省略するかどうか」などなどの好みに分類されるものなどがそうです。

「俺んちじゃカレーにニンジンは入れネェンだよ。だからニンジンは取り除きやがれコノヤロー」て言うようなことはやめましょうってことです。「なんだと!じゃあ言わせてもらうが、おれんちではジャガイモは六等分だ!もっと小さくしろ!」なんて議論になったら泥沼です。あと余談ですが、ぼくはニンジンは苦手ですがカレーに入っていても構いません。おかずのダイバーシティは重要だと思っているのでむしろ歓迎すべきです。

こういうのを指摘したくなる気持ちはわかります。でも果たしてそれを修正することのメリットは、発生するコストを上回っているのか?そこに帰結しますね。その辺はコーディング規約でカバーすればいいんじゃないかと。

とはいってもあんまりコーディング規約を厳密にするようなことはせずに、ある程度柔軟に許容する方針にしています。結局コードレビューで指摘するような事態になってしまうので。

メンバーのスキル向上

Businessplan besprechen - Arbeit im Büro
シーエイダブリュウでは一つのプロジェクトを2,3人くらいでまわすことがほとんどですが、まだメンバーが固まらないうちはほぼ一人で書いていたような時期もありました。

他人のコードが介入しない分、全体の見通しが非常に良いのですが、反面自分のコードの書き方しか知らないし、知る機会も限定的になってしまっていました。それは短期的には問題にならないですが、あるとき自分を振り返ってみたときに、あまり技術力が向上していないことに気付いたのです。

コードレビューをすることで必然的に他人のコードを読むことになり、同時に読まれることになります。今までゴリゴリ書いていた処理が、実はもっとシンプルに書けたりするようなことに気付いたり、人に読まれることを意識することでよりスッキリしたコードを目指す習慣が根付きます。

これは特に新人に対しても有効で、コードの善し悪しの基準を伝える良い機会になりますし、不足がある場合はペアプロに移行してより理解を深めるようなこともしやすくなります。

リモートのエンジニアとのコミュニケーション

シーエイダブリュウでは常時2,3名くらいのリモートのエンジニアと一緒に開発していて、タスク単位で作業を割り当てる方式を全てWEB上で行っているので、低いコミュニケーションコストでスムーズに運用していけるように工夫しています。

ところが、逆にコミュニケーションの機会が少ないことで、「タスク消化要員」のようなポジションになってしまい、自社事務所とリモート要員の間で「こっち側とあっち側」みたいな見えない境界線があるような感覚を感じるときがあります。

そこで、一緒に開発しているリモートの方は非常に優秀な方が多いので、コードレビューの際に気になったところや疑問点などを質問して教えてもらったりすることで、ナレッジ共有とコミュニケーションの機会を確保する試みをしています。

この辺はまだ検証が足りないのでまた改めてブログに書きたいですね。

目的を明確に

今回はコードレビューの話でしたが、何でも「目的を明確にする」ということは大事なんじゃないかと思います。曖昧なままだと本来のメリットを得ることが出来ず、むしろ手段が目的化してしまって単に手間だけ増えるなんてことも良くありますからね。

そういやなんでデスピサロを倒さなきゃならなかったんだっけ?