コードレビューの視点 009 [コードレビューの視点]
修正後も自分で見直す
コードをレビューした際に、様々な指摘をします。そして、記録を重視する組織では、必ず指摘事項をすべて記録して議事録として発行します。一方、ペアプログラミングでは記録することより、最終的なコードの品質が高くなることが目的です。どちらも品質の高いソフトウェアを開発することがその目的の1つです。実は、議事録として発行するというのが意外とくせ者です。どういうことかと言うと、担当者は指摘事項を修正しただけで修正が完了したとして、再レビューへ持ってくることがあることです。そして、議事録にある指摘事項を1つずつ確認しようとします。
でも、実際に修正されたコードを見ると、さらに指摘すべき項目が多く見つかったりします。特に、最初のコードレビュー時における指摘は、頭の中で指摘した内容が修正された状態をイメージするだけであり、ペアプログラミングのように目の前に修正結果が見える訳ではありません。
その結果、コードレビュー後に指摘事項に従って担当者がコードを実際に修正してみると、さらに改善すべき点が見つかるはずです。しかし、残念ながらスキルレベルの低いエンジニアほど、指摘事項だけを修正して、自分ではもう一度見直すことなく再レビューへ持ってきてしまいます。
私自身の性格もあるのかもしれませんが、再レビューでも最初から目を通します。議事録に書かれた指摘事項が修正されていることを再レビューで一つ一つ確認することは好みません。特に指摘事項の数が多いほど、もともとのコードの質が低いのであり、修正後のコードをもう一度見直す必要があります。指摘事項を修正したかは、担当者が自分で行えば良いことです。むしろ、修正後のコードに何か指摘されるような問題はないかを担当者が自分自身で確認する習慣と実際に確認することが重要です。
コードレビューの視点 008 [コードレビューの視点]
エンジニアのレベルが分かる
コードをレビューすることで、コードを書いたエンジニアのレベルがある程度分かってしまいます。たとえば、以下のことが大体分かってしまいます。- 使用しているプログラミング言語をどれだけきちんと学習しているのか
- コードの読みやすさにどの程度注意を払っているのか
- 必要な基礎知識を持っているのか
- マルチスレッドに関する知識は持っているのか
- etc
エンジニアのレベルが分かってしまえば、与えた仕事の難易度が本人に適切かも分かることになります（「技術者のレベルとソフトウェア開発の難易度（9）」）。
もちろん、分かると言ってもレビューアーの方がスキルレベルが高い必要があります。
コードレビューの視点 007 [コードレビューの視点]
知識の伝達の場
コードレビューは品質を向上させるだけでなく、様々な効果があります。その1つが、知識の伝達の場だということです。特に、レビューアーとしてレベルの高い人、つまり、様々なソフトウェア開発経験を経た人がいる場合、コード上の問題点を指摘する際に、なぜ、それが問題になるのか、修正しないとどのようなことが起きたり、将来起きるのかを説明できたりします。もちろん、それだけでなく、プログラミング言語に関しても初心者が知らない落とし穴や書き方を指摘することもできます。初心者が何もかもすべて教えてもらえると期待してもらっても困りますが、ソフトウェア開発組織としては、きちんとしたレビューが行われることは、知識を伝達するという行為でもあるということを認識する必要があります。
コードレビューの視点 006 [コードレビューの視点]
説明とコードが違っていないか注意する
コードレビューでは、クラスの役割を説明してもらった後に、メソッドの処理内容を説明してもらいます。本来はその両方がきちんとjavadoc形式で記述されていれば良いのですが、先に説明を書いてからプログラミングする人は少なく、頭の中で考えてそのままプログラミングされることが多いです。そのため、コードレビューでも説明が必要となります。その説明では、本人が書いた気になっている処理を説明します。しかし、レビューアーは、その通りの処理になっているかをコードを理解しながら確認する必要があります。実際、コードレビューでは、本人が意図した通りの実装になっていないコードを目にする場面が多くあります。
特に注意しなければ、自動テストコードが書かれていたり、あるいは手作業でテスト済みであるような場合です。つまり、「テスト済み」と本人が思っているコードであれば、考えていた通りの処理になっていると書いた本人は思い込みでコードを説明する可能性が高くなります。しかし、コードが間違っていて、テストも正しい期待する動作をテストしていなかったりすることがあります。
コードレビューでは、レビューアーは説明とコードが食い違っていないかに注意を払う必要があります。
コードレビューの視点 005 [コードレビューの視点]
独り言コメントは書かない
コードレビューをしていると何の役にも立たない「独り言コメント」に遭遇することがあります。たとえば、最近見たのは、次のようなものです。// やりたくないけど仕方なくこれだけでは、何が問題でどのように対処しようとしているのかが全く述べられていないため、コメント行の後に書かれているコードを見ても、そのコードが行っている処理の理由が全く分かりませんでした。結局、説明してもらって問題が分かり、コメントを書き直すように指示しました。
15年前に見た別の例では、次のようなことが書かれていました。
/* マイクロソフトのコンパイラが馬鹿だから */この時も、コメント行の後のコードを見てもコンパイラにどのようなバグがあったのか全く分かりませんでした。ここでは、ある言語仕様に対してどのようなバグがコンパイラにあって、それを回避するためのコードであることがコメントから分かる必要があります。あるいは、ひょっとしたら、コンパイラにはバグは無くて、書いた人が何かを誤解していたのかもしれません。残念ながらその時は誰が書いたコメントであるかは分からなかったので、具体的な内容を知ることができませんでした。
会社の資産としてのソースコードに「独り言コメント」がたくさんあるとしたら、コードレビューをしない組織か、コードレビューをしてもきちんとしたレビューができない組織でしょう。
コードレビューの視点 004 [コードレビューの視点]
言語仕様を確認する
コードをレビューしていると、コードを書いた本人がきちんと言語仕様を理解していない場合があります。あるいは、プログラミング経験がない言語のコードをレビューする場合には、コードを書いた本人に言語仕様を確認してみると説明が間違っているような気がすることがあります。コードレビュー中に言語仕様に関して作成者あるいはレビューアーが確信が持てない場合には、言語仕様をその場で確認することが重要です。確認してみて、その上でコードが正しいのか誤っているのかを判断する必要があります。ただし、演算子の優先順位などは、言語仕様を確認するのではなく、（）を付けることで意図した計算順序を明確にするのが良いです。
したがって、使用するプログラミング言語に関しては、その言語仕様にいつでもアクセスできるようにしておく必要があります。
コードレビューの視点 003 [コードレビューの視点]
マルチスレッドプログラミングに注意する
Javaが登場する前は、C/C++でマルチスレッドプログラミングをしようとすると、別途POSIX Threadなどのライブラリを使用しなければなりませんでした。Javaは言語仕様そのものにマルチスレッドプログラミングの機構を持っています。しかし、マルチスレッドプログラミングというのは、同期も含めて正しく設計・実装するのは難易度が高い領域です。そのため、同期処理も含めてかなり真剣にレビューしないといけません。さらに残念ながらレビューだけでは不十分であり、ストレステストも必要になってきます。そのため、マルチスレッドプログラミングを行うソフトウェア開発では、次のことが必須となります。
- マルチスレッドプログラミングに関する基礎知識をすべてのソフトウェアエンジニアがきちんと習得していること
- 書かれたコードは、マルチスレッドプログラミングに関する豊富な知識と経験を持つ人がきちんとレビューをする
- 書かれたコードは、自動テストが実行できるようになっていて、様々なプラットフォーム（単一コアから複数コア、あるいは、マルチプロセッサー）で長時間ランニングテストを行う。さらに、同時にシステムに別の負荷（CPU、HDなどへの負荷）をかけたランニングテストも行う
ある朝、停止しているテストを私も含めて数名でデバッグしていたのですが、そのバグはコードレビューでは到底見つけられないような非常に複雑な状態の時に発生するというものでした。そして、その発生原因となったコードの修正がいつ入れられたのかを調査したら、3ヶ月前のことでした。つまり、そのバグは、3ヶ月間の膨大なランニングテストをすり抜けたことになります。
マルチスレッドプログラミングの難しさは、「1回動作したから正しいという保証もないし、100回動作したから正しいという保証もない」ことです。そのため上記の3条件がそろった開発が必須となります。上記の3条件が全くそろっていないソフトウェア開発では、品質が安定することはなく、最後にはプロジェクトがキャンセルさせられるか、あるいは、製品として無理にリリースしてもその後の障害対応に膨大な工数が必要となります。
マルチスレッドプログラミングに関する基礎知識を持たない人が書いたコードは、動作させなくても（私が）コードを見ただけで問題があるのが分かることがほとんどです。そして、基本知識もない人が書いて、かつ、まともにレビューもされていないことも分かってがっかりすることが多いです。
コードレビューの視点 002 [コードレビューの視点]
必要なエラー情報を付加する
何らかのエラーを検出した場合の対処方法としては、いくつかあります。- 公開APIが呼ばれた場合のメソッドの引数の値が不正だった場合に、
NullPointerExceptionやIllegalArgumentExceptionをスローしたりします。 - デバッグ用にデバッグ文を表示したり、ログ情報を記録したりします。
Javaでは慣例として、引数の
nullの場合には、NullPointerExceptionをスローします（『Effective Java 第2版』 「項目60 標準例外を使用する」）。その場合、Javadocにその旨をきちんと記述すると同時に、例外をスローする際にエラー情報をコンストラクタのメッセージとして渡す必要があります。たとえば、引数valueがnullであってはいけない場合には、次のようになります。
throw new NullPointerException("value is null");
引数の値が不正であった場合も同様です。たとえば、
valueが負の値ではいけない場合には、次のようになります。
if (value < 0)
throw new IllegalArgumentException("value(" + value + ") is invalid");
IllegalArgumentExceptionの引数なしコンストラクタが呼ばれていたり、あるいは単に"value is invalid"と書かれていたりすることが多いです。実際にデバッグをする段階になって、どのような値であったかを知る必要があると、「値を表示するようにコードを修正して、障害を再現させて値を確認する」という行動が必要になります。Java言語に限らず、通常のデバッグ文やログ情報にも同じことが言えます。せっかくデバッグ文やログ情報を残すようにコードを書いても、いざ、デバッグ時に必要な情報（変数やポインタの値など）が出力されていないと何の役にも立ちません。
デバッグ文やログ情報出力は、コードの可読性を下げる場合が多いです。しかし、それでも必要があると判断して記述するのであれば、デバッグに必要なエラー情報はすべて表示したり記録したりすることが重要です。コードレビューをしていると、その点を全く考慮せず、デバッグ文やログ情報出力を書いているコードを多く見かけます。
コードレビューの視点 001 [コードレビューの視点]
Copyrightを書く
1984年に社会人となって開発に従事したFuji Xerox 6060 Workstationでは、ソースコードにきちんとCopyrightを書いていたかどうかは覚えていない※のですが、それ以降のプロジェクトでは、ソースコードにはCopyrightを表記するのが当たり前の文化の中で過ごしてきました。※ たぶん書いていたと思うのですが・・・
Copyright表記そのものは、XDE（Xerox Development Environment）のエディタでは自動的に挿入・更新を行ってくれたりもしましたが、そうでない環境でも手作業でも必ず記述する習慣となっていました。そのため、私自身が書いたC++言語用コーディング規約にもCopyrightを記述することを規定していました。
企業でソフトウェアを開発する上でCopyrightを記述することは、私にとっては当たり前だとずっと思っていましたが、そうではない組織も数多く存在することに驚いています。そのため以下のことが起きていたりします。
- Copyrightの記述がないことをコードレビューで誰も指摘しないし、Copyright表記がないソースコードが組織内に多数存在する。
- Copyrightを記述すべきだという認識を開発者が持っていない。
- 開発者がCopyrightの書き方を知らない。
ソースコード上のCopyright表記はソフトウェアの実行には全く関係ないのですが、企業の資産としてソースコードにCopyright表記をしない組織文化は、ソースコードを軽視していることの現れかもしれません。
著書（PDF）
翻訳本（Kindle版）
翻訳本（PDF）
