2006年10月20日金曜日

効率の良いコードレビュー [software]

とある友人が紹介してくれた,「苦痛を伴なわない効率の良いコードレビュー」という記事.なかなか良いことが書いてある.
Effective Code Reviews Without the Pain

特に,コードレビューには,1) コードの品質を保証する,2) 開発者を教育する,という2つの目的があると前置きした上で,コードレビューに対するアプローチに言及している点がおもしろい.

1) 物事を断定するのではなく,質問を投げかけるようにすること
2) 「なぜ?」という質問を避けること
3) 褒めることを忘れないように
4) コーディングルールが確立されていること
5) 議論の対象はあくまでもコード,決してコーダー (開発者) になってはいけない
6) 解決方法は1つだけではないことを念頭に


そしてもしあなたが開発者ならば,

1) コードがあなた自身ではないことを忘れないように
2) 自分用のチェックリストを作成すること
3) コーディングルールを整備すること

とのこと.

(上記全て私なりの意訳なので,各々の説明も含めて詳細については原文を参照して頂きたい)

いずれにしても,「間違い探し」「犯人探し」や「誰が優れたプログラマーかコンテスト」ではなく,「参加者全員から何かを学ぶチャンスがあるフォーラム」といった雰囲気作りが大切なのだということ.

非常に参考になった.


ところで私が考える「効率の良いコードレビュー」は以下の通り.

まず基本的に,チームのメンバーを全員集めて対象ソースコードをファイルの頭から順番に舐めていくのは,あまり効率的ではないと思う.当然プログラムはその順番で動くわけではないし,ソフトウェアは時間概念を持って動くので,静的にコードを眺めていても絶対に抜けが発生する.いわゆる「文法チェック」に毛が生えた程度のチェックしかできず,それはそれで必要だが何人も集めて行うのは少しもったいない.

ということで,コードレビューは,「ペアレビュー」と「チームレビュー」に分ける.

ペアレビューでは,二人一組になって,レビューする側はレビューされる側のコードを基本的に全て見る.この段階では,ツールが見逃した「文法チェック」や「単純ミスがないか」,「コーディングルールに沿っているか」など,つまり「プログラミング言語として正しく書けているか」という観点でチェックを行う.レビューされる側は,レビューする側に,自分の言葉で全てのコードを説明する.レビューする側は疑問に思ったことはなんでも聞くことにする.

チームレビューでは,コードを順に追っていくようなことはせず,「抜き打ち試験方式」を取る.対象個所の仕様やシステムに精通したレビュワーが,事前に質問を10個程度用意する.例えば「ユーザーがこの操作をしている時にこのイベントが発生したらどうなる?」とか,「このデータとこのデータが同時に書き変わったらどうなる?」とか,「またその時にこの類のエラーが発生したら,どういう手順で復帰する?」みたいな.それで,例えばシーケンス図等の補助資料を使いながら (その場でホワイトボードに書いても可),それらの質問に対して全てコードベースで説明していく.他のレビュワーも,その説明に対して追加の質問をどんどん投げかける.タスクやプロセス境界とそれらのプライオリティなども考慮し,あくまでも「動いているソフト」をイメージしながら質問することが重要.コードを書いた人は,もちろんそこの個所に一番詳しいはずなので,全ての質問に対して理路整然とコードベースで回答できなければいけない.

時間が許すなら,ペアレビューは違うパートナーと3セット程度,チームレビューはメンバーからの追加質問が「それはちょっと重箱の隅をつつきすぎじゃない?」というレベルになるぐらいまで,というのが理想だ.もちろんチームレビューの方は,考慮が足りないコードや明確に説明できない場合などは,「リベンジ」として対象範囲の第2回目以降が設定される.経験的にこの方法が,一番コードの品質が上がるような気がするし,不具合が出た時に追いやすくなると思う.


ところで上記引用記事の冒頭に書いてある,「開発者は他人のコードを指摘することで,いかに自分が優れているか誇示しようとする」というのは,どこの開発現場でも見られる光景のような気がして,思わず苦笑してしまった.


2 コメント:

匿名 さんのコメント...

まさに今仕事でコードレビューの進め方を考えていたので参考になりました。
チームレビューとペアレビューを分けるのはまったく同感です。
ただ、ペアレビューの進め方がかなりポイントになりそうですね。コンパイラが見つけてくれない問題というのは、コーディングルールが徹底されていてかつその該当モジュールのアルゴリズムが分かっていることが前提にしないと、有意義なものにならない可能性があります。

僕のチームの場合は、チームレビューを教育のため、ペアレビューをコード品質を保つため、と明確に分けています。
ペアレビューの前提として、レビュアーが該当モジュールの副担当であることにしています。モジュールの外部仕様、内部構成が分かっている上でコードレビューを進めてもらう、と。つまりペアレビューの目的をユニットテストのカバレッジを稼ぐのと同じとし、レビュアーが副担当としてやっていけるように仕立て上げる活動のうちの一環にしてしまうのですね。
ただ、このようなやり方はいきなり他人から渡された数万行レベルの外部仕様書のないモジュールの「レビュー」にはむきませんが。

そして時間の都合上、「教育」のためのチームレビューがなかなかできないのですが…。

Toshiya Hasegawa さんのコメント...

> ただ、ペアレビューの進め方がかなりポイントになりそうですね。
まさにそうなんですよ.
コードレビューに限った話ではないけど,開発プロセスっていくらでも工夫できる気もするし,凝りすぎても逆効果だろうし.

ただ,今回の引用記事を読んで一番感銘を受けたのは,「メンバーが『やらされてる感』を感じるようではダメ」ってことなんです.

…で,引用記事の中にもいろいろアドバイス書いてありますが,一番はやっぱり「リーダーに技術力があること」なのかな,と.

以前も この辺り で書いたんですけど,「名選手,名監督にあらず」とも,「名監督は名選手」でなければいけないと.

技術が分かっていないリーダーが「コードレビューは大切だから実践しよう」なんて言っても全然説得力ないですもんね.

コードレビューに関して言うと,コミュニケーションがよく取れた,チームワークの良いチームで行う程効果が高いと思うので,チーム運営を重視すると共にコードレビューも「やり続ける」ことが大切なんでしょうね.