By
—

コードレビューの進め方

ビジネスバンクグループ CTO の @shoutakenaka です。

ALL-IN開発チームでは全員で相互レビューをしていて、コードを書いたら必ず別のエンジニアにレビューしてもらうようにしています。

必ずレビューされるとなれば書き方に気をつけるようになるし、レビューすることで他のメンバーが書いたイケてる書き方を盗めたり、仕様や書き方について疑問に思ったことをコーディング直後のホットなうちに担当のメンバーに確認できたりするので、メンバー間のコミュニケーションの促進や長期的なレベルの底上げに役立っています。

一方で、アサインされているタスクをこなしつつレビューをしないといけないので、コーディングだけするのに比べるとそれなりに大変。
各自うまく工夫しながらやっています。

そんななかで、僕が普段どんな風にコードレビューを進めているかをまとめてみます。

コンテキストによってレビューを使い分ける

Keyboard

一口にコードレビューといっても、状況によってどこに重きをおいてレビューをするかというのが変わってきます。 僕の場合、

  1. 新規ジョインメンバーのコード
  2. 共通部分の変更が入っている
  3. 業務機能部分の変更のみ

の３パターンのいずれかによって少しレビューの仕方を変えています。

新規ジョインメンバーのコードのレビュー

新たにジョインしてくれてまだ Rails とか AngularJS になれてないメンバーのコードをレビューする時は、アドバイスする箇所を探すという心持ちで、時間をなんとかひねり出して細かくコードを見ています。

慣れるまでは Ruby っぽい書き方になっていなかったり、Rails 関連の各種 gem で提供されている便利機能で簡単にできることを独自実装していたり、ということがあります。

そういうコードを見かけたら、「こんな書き方ができるよ」というコメントをしていきます。

後は ActiveRecord の使い方がまずいところ (大量に DB サーバーにクエリを投げるような実装になっているところとか) がないかどうかチェックしたりしています。

共通部分の変更が入っているレビュー

共通部分 (プロジェクトフレームワーク部分) の変更が入っている場合は、そもそも共通機能として実装すべきかどうか、既存機能に影響がないかどうか、という観点で慎重にチェックします。

共通部分の変更は影響範囲が大きい、かつ一度共通機能にしてしまうと後から手を入れにくい (特にインターフェースの変更とか手間がかかる) ので、設計の観点で色々ツッコミをいれたりします。

業務機能部分の変更のみのレビュー

上の２パターンはレビュー対象全体からするとそれほどの割合ではなくて、一番多いのが業務機能の変更に対するレビューです。

メンバー全員分の全変更をレビューするとなるとそれなりの分量です。

全部頭からじっくり見ることができればベストですが、現実的にそこまではしていられません。
なので、このケースの変更をレビューする時には、スピーディにチェックしつつもコメントすべき箇所を見逃さないか、

いかに重点的にチェックすべき怪しそうなコードを見つけるか

が重要になります。

怪しい「smell」がする場所を嗅ぎ当てる

Smell

では、どうやって重点的にチェックすべき怪しそうなコード ＝ 怪しい「smell」のするコード にあたりをつけるのか。 僕がレビューをする時は、

  1. 最初にタスクを見て、何を実装すべきなのかを確認
  2. 自分ならどう実装するのかざっくりとイメージ
  3. イメージと目の前のレビュー対象のコードを見比べる

をして、違和感があるかないかをチェックしていく感じで進めていきます。

タスクの内容を確認

まずはチケットを見てタスクの内容を確認し、そもそも何をしようとしている変更なのかを確認します。

今使っている Crucible だと、レビューを出す時勝手に JIRA のチケットにリンクしてくれるので、レビューを出す側も見る側も楽チンです。

チケットを見て、それほどボリュームの大きいタスクでなければ、自分で作るならどうするのかここでイメージします。
でかいタスクだと一気に全体をイメージするのが難しいので、コードを見ながらイメージを膨らませていく感じになります。

想定外のコードを追加、修正していないか

実装イメージを浮かべたら、まずどのファイルが修正されてるのかチェックします。

ここで「このファイルをいじる必要ありそう」と自分が思ったファイル以外のものが変更されていると怪しい「smell」です。

余計なことをしているか、担当メンバーが仕様を勘違いしてるか、それとも僕の方がタスクの内容を読み間違えているのか。
いずれにしろ僕が思っているのと実際の作業の間に何か齟齬がありそうなので、これらのファイルは重点的に確認した方が良さそうリストに入れます。

コードの「形」

次は一つ一つファイルを見ていきますが、コードの細かい内容は無視して全体の「形」、つまりインデントの下り具合や１ファイルあたりの行数などだけを見ます。

ここでコードのギザギザがいい感じになっていないところ、具体的には

  • ネストが極端に深い
  • 行が極端に長い
  • １メソッドが長い
  • 文字が密集してごちゃごちゃしている感じがする
  • ギザギザが長い (= １ファイルの行数が多い)

といった箇所は怪しい「smell」がするので、重点確認リスト行きです。

名前のつけ方

直感的にわかりやすい名前がついているかどうかは、可読性、ひいては保守性に影響するので、識別子 (変数やメソッド名) に適切な名前がつけられているかざっとチェックします。

  • 名前が適当 (e.g. a, b, tmp...)
  • 一般的ではない略語を使っている
  • 連番のサフィックスがついている (e.g. num1, num2...)
  • 変数が名詞じゃない
  • メソッドが動詞から始まってない
  • 英語として不自然
  • Ruby の慣習に従った名前になっていない

といったものは怪しい「smell」を発しているので要注意です。

特に、適切な名前がつけられていない時 (どんな名前をつけたらいいか思いつかなくて苦肉の策でつけたよう名前) は、プログラムの設計に問題があって、クラス分割やメソッド分割といったリファクタリングをした方がよい可能性があるので、そういう観点で細かいチェックをします。

これらをざっとチェックすると、集中的にレビューした方がよさそうなところが大体見えてくるので、そこを中心にレビューし、後は時間が許せばそれ以外のところも目を通す、という感じで進めていきます。

まとめ

この記事では、僕が普段どんな風にコードレビューをしているのかをまとめてみました。

全てのメンバーのコードをしっかりとレビューするのが理想ですが、レビューに使える時間は限られています。
ですので、ジョインしたてのメンバーや影響範囲の大きい部分に修正が入っているコードは重点的にレビュー、そしてそれ以外の変更に関してはざっと確認して怪しそうなところを洗い出し、そこだけ集中的にレビューすることで、手持ちの時間を「レビューすべき変更」に効率的に使うようにしています。

相互レビューを続けていれば全体のレベルが徐々に底上げされていくはずなので、それを信じて今後も相互レビューは継続していきます！

エンジニア募集中！

ビジネスバンクグループではエンジニアを募集中しています。

弊社が採用しているテクノロジや開発環境に興味を持った方は、 ここから是非エントリー を！