スクラムではコードレビューをどうやっているか？

みなさんこんにちは。@ryuzeeです。

よく一緒に議論したり大学行ってワークショップをしたりしている原田さんが、スクラムでのコードレビューについて書かれましたので、僕も過去数年コーチとして色々な現場に行った際のことや、自分で受託開発をスクラムでやった際のことを踏まえてスクラムにおけるコードレビューのやり方を書いてみます。

レビューのやり方

早期から頻繁に

僕自身は小規模で頻度の高いインクリメンタルなレビューを好んでいます。かつて大きなSIerにいてウォーターフォール型の開発をしていた際に、パートナーさんから出てきたソースコードを見ると、コピーペーストの嵐だったり、変数名がデタラメすぎたり、インデントがぐちゃぐちゃだったり（以下思いつく全ての「えーーー」を適当に想像してください）して、かつ工程の後半にレビューしていたので、直すに直せないというような悲惨なことも経験しました。（注）ウォーターフォールが悪いわけではなく、レビューを工程の後半にやってもムダが多いということです。

ですので、

とにかくソースコードが少量のうちから頻繁に実施するのが基本思想です。問題は小さいうちに解決した方が解決コストが安いというのは常識です。

早期から頻繁にレビューすることによって、後から大量に似たようなことを修正したりすることも避けられますし、問題の複雑化を避けることが可能になります。

完成の定義に含めるが、公式の会議にはしない

原田さんの例では、チーム全員がレビューに参加していましたが、僕は必ずしもそうである必要はないかなと思っています。レビューを行うこと自体については、スクラムにおける完成の定義に含め、タスクボードにもレビューを実施するという項目を貼ります。その際は最低チームの3人以上で行うというくらいにしています。また開催の時間も決めておらず、必要な時に周りに声をかけて実施します。不安に思ったら即レビューすればいい、という感覚ですね。 XPのペアプログラミングの良いところは、常に複数の目を通る（しかも途中でペア変えます）ことによって常時レビューが行われているのに近い状態になることです。ペアプロができなくても、常に頻繁にレビューすることによって効果を得られると思います。

レビューでの観点

このあたりはあんまり特殊なことはしていないことが多いです。クラス名や変数名や関数名、データベース名、カラム名など、「名前」が適切かは重視します。名前が分かりにくソースコードは多くの場合良いソースコードではないです。名前のまずさをコメントでカバーするようなコードにならないように初期の段階から名前にはこだわります。

構造の確認としては、僕はWebアプリケーションが多いので、Model, View, Controllerのそれぞれが、本来それぞれでやるべきことをやっているのか、という点も確認ポイントです。経験上あまりレビューを行わないで進めると、Controllerが肥大化しやすい気がします。またこれとあわせて、そもそも書いたソースコードは自動テストしやすいかという点も確認します。継続的にリリースを繰り返していくためにはテストの自動化は必須です。したがって初期の段階からテストが書きやすい構造、テストの実行速度が速い構造、依存関係の少ない構造を維持することが重要になります。またレビューの観点として重複コードを避けるためのコンポーネントやビヘイビアの切り出しなんかもテーマになります。

レビューで使う道具

レビューを実施する際に気をつけるべきことは、人間にしかできないことに集中する、ということです。したがってインデントの間違いやコメントブロックの有無のチェック等定型的なチェックは機械にまかせるべきです。これらは例えば、PHP_CodeSnifferやCheckStyleのような体裁チェックツール、複雑度やコピーペーストディテクターなどが該当します。このようなツールは事前にCI（継続的インテグレーションサーバ）にプロセスとして組み込んで常時チェックする仕掛けにします。ただ常時機械チェックするだけでなく、チームとしての対応方針も決めておく必要があるのはもちろんです（例えば、警告レベルN以上は必ず対応するとか、複雑度M以上は対応するとか。技術的負債が増えないことを意識する必要があります）。

レビューで気を付けていること

原田さんも書かれていましたが、レビューは改善のために行うもので、個人を批判するためのものではありません。後ろ向きな指摘ではなく、プロダクトを作っているチームとして、どうやったらより速く品質の高いものをつくれるようになるかという改善の場です。レビューの際にはまずそのことについて全員が同意している必要があります。スクラムではチーム内での立場の上下はありませんので、特定の人が一方的に指摘するのも好ましくありません。そのあたりのファシリテーションは必要であればスクラムマスターが行えば良いでしょう。チームの成熟度が高くなれば、スクラムマスターがどうのこうのせずとも、同じ方向を向いて活動できるようになります。またレビューの結果を個人の人事評価に結びつけるような真似をしてはいけません。これはバグの作り込みの数についても同様です。評価に結びつけた時から隠蔽が始まります。