とあるWebアプリケーションのパッケージベンダーにいる者です。
自分はよくレビュワーとしてコードレビューをしているのですが、周囲の他のレビュワーに話を聞いて
「時間が全然足らない」
「うまく指摘できない」
という悩みをよく聞きます。
今回自分が普段思っているそれらの課題について対策をまとめたいと思います。
限られた時間内でコードレビューするには
1.コードレビューは単体テストと考える
コードレビューで見る範囲はできるだけ絞る、ということです。
要求仕様に対する実装が正しいかどうかまで見るとちょっと過剰で、
時間が足りなくなってしまいます。
要件定義や設計までさかのぼることはせず、
書かれているコードの妥当性だけを見ていきます。
ただし修正範囲のコードについてはすべてしっかり見ます。
ホワイトボックステストでC1カバレッジを通すのと同じように、
分岐もしっかり見ていきます。
要件定義通りになっているかは総合テストで、
設計通りになっているかは結合テストで確認すればよいです。
2.レビューで"見ない"視点を決める
レビュー観点はどうするか、という点です。
観点は決めておかないとレビューする人もされる人も終わりが見えず、
時間切れで中途半端に打ち切られてしまいます。
「見る視点」を決めるより「見ない視点」を決めるのは、
「見ない視点」は別の方法で確認する計画が立てやすいからです。
たとえば「今回はパフォーマンス観点では見ない」と定めておけば、
それらは後で性能テストでカバーすれば良いことが明確になります。
3.黙々と見る
レビュワーと製造担当者が面談みたいな形でコードレビューする場合があるのですが、
顔合わせはせずにデスクで黙々と見ていくのがよいです。
ただし不慣れな人が書いたコードなど、書き方の甘さや意図の見えにくさがあるようであれば
顔合わせてレビューするほうが教育的効果が期待できます。
具体的にどうレビューするのか
古い記事ですが、IPAのセキュアプログラミング講座の中で以下の段階分けが提唱されています。
下読み どこに何があるかの把握
成熟度点検 ある程度の品質水準に達しているか否かの評価
機能点検 バグ、すなわち機能面の不具合の検出
脆弱性点検 セキュリティ脆弱性の検出
これをしっかりやるとやや過剰気味ですので、アレンジしていきます。
下読み
これについては以下の理由からサラっと流しましょう。
- レビュワー本人も同じプロジェクトメンバーであれば
そのシステムのアーキテクチャーをある程度把握しているはず。 - どこに何があるかは、Diffで出てくる。
自分はプロジェクトメンバー外ですが、パッケージベンダーに所属しており、
同じアーキテクチャー上に実装されていることがほとんどですので、この工程を省略しています。
成熟度点検
- ソースコードが読みやすいかどうか。
ここはあまりウェイトを置いていません。
レビュワー本人の過去の経験をもとに次のような点を主観で判断していきます。
- メソッド内のコード量が多くないか
- ネストが深くないか
- 冗長ではないか
- 意味がわかるか（実装の意図がつかめるか）
普段からリーダブルコードなどの書籍を読んでおくといいかもしれません。
機能点検
- 機能によって必要な実装がされているか
- 機能によらず必要な実装がされているか
前者は、たとえば「買い物かごに入った商品を表示する」という機能があった時
合計金額の計算はDecimal型を使っているかとか、
端数は四捨五入／切り捨て／切り上げになっているかとか、
そういった視点で見ていきます。ただし単体レベルを超えないように。
後者は、ループ中の変数がちゃんと初期化しているかとか
例外が発生したときでもロールバックしているかとか、
使っているメソッドやステートメント、記法が問題ないかを見ていきます。
ここは成熟度点検とも重なってきますが、こちらに分けました。
脆弱性点検
- 社内ルールに則っているか
セキュリティ脆弱を見つける方法としてのソースレビューは骨が折れます。
一般的なセキュリティ脆弱はツールなどを使って見つけていくことにしておき、
そういったツールでは見つけづらいコーディング規約や独自アーキテクチャーのルールに従っているかを見ます。
ツールはWebサイトをブラックボックス的に叩くもののほか、
ソースコードの静的解析をするホワイトボックス的なものもあります。(IBM AppScan Sourceなど)
今回「限られた時間で」コードレビューするための1つの対策として自分の取り組みを紹介しました。
時間が十分に確保できるのであれば良いのですが、パッケージベンダーやSIerは納期に間に合わせるために
どうしても削らなけばならない状況が発生しますので、この対策が現場の一助になれば幸いです。