コードレビューの視点 006

説明とコードが違っていないか注意する

コードレビューでは、クラスの役割を説明してもらった後に、メソッドの処理内容を説明してもらいます。本来はその両方がきちんとjavadoc形式で記述されていれば良いのですが、先に説明を書いてからプログラミングする人は少なく、頭の中で考えてそのままプログラミングされることが多いです。そのため、コードレビューでも説明が必要となります。

その説明では、本人が書いた気になっている処理を説明します。しかし、レビューアーは、その通りの処理になっているかをコードを理解しながら確認する必要があります。実際、コードレビューでは、本人が意図した通りの実装になっていないコードを目にする場面が多くあります。

特に注意しなければ、自動テストコードが書かれていたり、あるいは手作業でテスト済みであるような場合です。つまり、「テスト済み」と本人が思っているコードであれば、考えていた通りの処理になっていると書いた本人は思い込みでコードを説明する可能性が高くなります。しかし、コードが間違っていて、テストも正しい期待する動作をテストしていなかったりすることがあります。

コードレビューでは、レビューアーは説明とコードが食い違っていないかに注意を払う必要があります。

書籍『継続的デリバリー』

このブログでも以前紹介しましたが、2011年Jolt Excellence Awardを受賞した『Continuous Delivery: Reliable Software Releases through Build, Test, and Deployment Automation』の翻訳本が出版されています。

継続的デリバリー 信頼できるソフトウェアリリースのためのビルド・テスト・デプロイメントの自動化

• 作者: David Farley
• 出版社/メーカー: アスキー・メディアワークス
• 発売日: 2012/03/14
• メディア: 大型本

本のキャッチコピーの「いつまで手動でデプロイしているんですか？」がいいですね。お勧めの書籍です。

David Holmes氏来日予定（JavaOne Japan 2012）

『プログラミング言語Java』の第3版から著者として名前を連ねていて、第4版では改訂部分の多くを執筆したDavid Holmes氏がJavaOne Tokyo 2012のスピーカーとして来日します。

プログラミング言語Java (The Java Series)

• 作者: ケン・アーノルド
• 出版社/メーカー: ピアソンエデュケーション
• 発売日: 2007/04
• メディア: 単行本

今回は、Jigsawに関するセッションとLambdaに関するセッションの2つで話をする予定だそうです。David Holmes氏と会うのは2008年のサンフランシスコでのJavaOne以来です。

コードレビューの視点 005

独り言コメントは書かない

コードレビューをしていると何の役にも立たない「独り言コメント」に遭遇することがあります。たとえば、最近見たのは、次のようなものです。

// やりたくないけど仕方なく

これだけでは、何が問題でどのように対処しようとしているのかが全く述べられていないため、コメント行の後に書かれているコードを見ても、そのコードが行っている処理の理由が全く分かりませんでした。結局、説明してもらって問題が分かり、コメントを書き直すように指示しました。

15年前に見た別の例では、次のようなことが書かれていました。

/* マイクロソフトのコンパイラが馬鹿だから */

この時も、コメント行の後のコードを見てもコンパイラにどのようなバグがあったのか全く分かりませんでした。ここでは、ある言語仕様に対してどのようなバグがコンパイラにあって、それを回避するためのコードであることがコメントから分かる必要があります。あるいは、ひょっとしたら、コンパイラにはバグは無くて、書いた人が何かを誤解していたのかもしれません。残念ながらその時は誰が書いたコメントであるかは分からなかったので、具体的な内容を知ることができませんでした。

会社の資産としてのソースコードに「独り言コメント」がたくさんあるとしたら、コードレビューをしない組織か、コードレビューをしてもきちんとしたレビューができない組織でしょう。

プログラミング言語Go関連の書籍 （2）

Go Version 1のRC1がリリースされましたが、すでにVerison 1対応の書籍が出版されたようです。

The Way to Go: A Thorough Introduction to the Go Programming Language

• 作者: Ivo Balbaert
• 出版社/メーカー: iUniverse
• 発売日: 2012/03/07
• メディア: ペーパーバック

電子書籍版は、diesel eBook storeでなんと$3.99で購入できます。

http://www.diesel-ebooks.com/item/9781469769165/Balbaert-Ivo-The-Way-to-Go-A-Thorough-Introduction-to-the-Go-Programming-Language/1.html

コードレビューの視点 004

言語仕様を確認する

コードをレビューしていると、コードを書いた本人がきちんと言語仕様を理解していない場合があります。あるいは、プログラミング経験がない言語のコードをレビューする場合には、コードを書いた本人に言語仕様を確認してみると説明が間違っているような気がすることがあります。

コードレビュー中に言語仕様に関して作成者あるいはレビューアーが確信が持てない場合には、言語仕様をその場で確認することが重要です。確認してみて、その上でコードが正しいのか誤っているのかを判断する必要があります。ただし、演算子の優先順位などは、言語仕様を確認するのではなく、（）を付けることで意図した計算順序を明確にするのが良いです。

したがって、使用するプログラミング言語に関しては、その言語仕様にいつでもアクセスできるようにしておく必要があります。

コードレビューの視点 003

マルチスレッドプログラミングに注意する

Javaが登場する前は、C/C++でマルチスレッドプログラミングをしようとすると、別途POSIX Threadなどのライブラリを使用しなければなりませんでした。Javaは言語仕様そのものにマルチスレッドプログラミングの機構を持っています。しかし、マルチスレッドプログラミングというのは、同期も含めて正しく設計・実装するのは難易度が高い領域です。

そのため、同期処理も含めてかなり真剣にレビューしないといけません。さらに残念ながらレビューだけでは不十分であり、ストレステストも必要になってきます。そのため、マルチスレッドプログラミングを行うソフトウェア開発では、次のことが必須となります。

• マルチスレッドプログラミングに関する基礎知識をすべてのソフトウェアエンジニアがきちんと習得していること
• 書かれたコードは、マルチスレッドプログラミングに関する豊富な知識と経験を持つ人がきちんとレビューをする
• 書かれたコードは、自動テストが実行できるようになっていて、様々なプラットフォーム（単一コアから複数コア、あるいは、マルチプロセッサー）で長時間ランニングテストを行う。さらに、同時にシステムに別の負荷（CPU、HDなどへの負荷）をかけたランニングテストも行う

多数のプロセスが協調し、個々のプロセス内では多数のスレッドが動作するという大規模なマルチスレッドプログラミングを行うプロジェクトに従事したことがあります。その時は、何十台という様々なPCで夜間にランニングテストを繰り返していました。そして、朝出社してテストが停止していると最優先でデバッグを行います。

ある朝、停止しているテストを私も含めて数名でデバッグしていたのですが、そのバグはコードレビューでは到底見つけられないような非常に複雑な状態の時に発生するというものでした。そして、その発生原因となったコードの修正がいつ入れられたのかを調査したら、3ヶ月前のことでした。つまり、そのバグは、3ヶ月間の膨大なランニングテストをすり抜けたことになります。

マルチスレッドプログラミングの難しさは、「1回動作したから正しいという保証もないし、100回動作したから正しいという保証もない」ことです。そのため上記の3条件がそろった開発が必須となります。上記の3条件が全くそろっていないソフトウェア開発では、品質が安定することはなく、最後にはプロジェクトがキャンセルさせられるか、あるいは、製品として無理にリリースしてもその後の障害対応に膨大な工数が必要となります。

マルチスレッドプログラミングに関する基礎知識を持たない人が書いたコードは、動作させなくても（私が）コードを見ただけで問題があるのが分かることがほとんどです。そして、基本知識もない人が書いて、かつ、まともにレビューもされていないことも分かってがっかりすることが多いです。

朝型力

プログラマー”まだまだ”現役続行 (技評SE選書)

• 作者: 柴田 芳樹
• 出版社/メーカー: 技術評論社
• 発売日: 2010/09/04
• メディア: 単行本（ソフトカバー）

拙著の第7章「朝型力」からの抜粋です。

人は見ていないようで見ている

　20代、特に新卒新人として配属されたときは、誰よりも早く出社する習慣が必要です。ぎりぎりにしか出社しないようであれば、自分で時間管理ができないと見なされてもおかしくありません。そのような人に限って、朝の会議の時間ぎりぎりに会議室に入ってきたりします。
　当人は、「ぎりぎり間に合った！」とほっとしているかもしれませんが、そのようなことを繰り返していると、時間にルーズな新人だと見なされるだけです。
　人は見ていないようで見ているもので、誰々はいつも時間を守って余裕を持っているが、誰々は時間管理がだらしないと、評価はすぐに職場中に広がります。

今、読み返しみると「誰よりも」は余分かと思います。

ちょっとぐらいの早起きでは元にもどる

　朝、余裕を持って出社するには、やはり早起きすることです。その結果、ぎりぎりに会社に駆け込むこともないでしょうし、遅刻することもなくなります。
　しかし、早起きを動機づけるには、早く起きた分の時間を何かの活動に充てることです。
　ほんの10分ぐらい早起きして、ちょっとだけ余裕を持ってコアタイム開始時間より少し前に出社しようとしても、必ず元の状態に戻って、いつもぎりぎりとなります。したがって、思い切って１時間とか２時間早く起きることです。そして、その時間で読書なり、英語の学習といったものを行うことです。

章のまとめとして、次のように述べています。

貴重な朝の時間を有効に使う

　朝型の生活パターンを勧めている書籍は多く出ていますが、夜型を勧めている本はほとんどありません。 　一度しかお会いしたことがありませんが、『朝2時起きで、なんでもできる!』の著者である枝廣淳子さんは、朝の時間に集中して行うことで、さまざまな活動をされています。
　朝の１時間は、夜の２時間に匹敵するといわれます。早起きして、コンピュータやソフトウェアに関する学習をするのもいいでしょうし、情報処理試験などの資格を取得するための学習をするのもいいでしょう。
技術者として早朝に学習しようと夜に学習しようとどちらでも良いではないか、という意見の人もいるでしょう。自分の効率が良い時間帯で学習をすれば良いのは確かです。ただし、それには、社会人として時間にルーズにならないという条件付きです。
　会社に遅刻する、半日休暇を取る、朝の自主参加の勉強会に遅刻したり欠席したりする、などの理由として「夜遅くまで勉強していて朝起きられませんでした」は理由にはなりません。つまり、勉強しているから、時間にルーズでも良いでしょうというのは成り立たないのです。そして、技術的には優れていても、この時間にルーズという一点だけで全体評価は下がってしまいます。
　朝型として継続した学習を勧める理由の一つは、朝寝坊をして朝の時間に関してルーズになることを防いでくれることです。当然、早朝に学習をするためには、実際に家を出る２、３時間前に起きて学習することになります。その結果として、上記のような時間を守れないような行動を減らすことができる訳です。
　重要なことは、継続した学習よりも「時間を守る」方の優先度が高いことです。これは、技術者としてではなく社会人としてです。
　とにかく、若いうちは、この朝の貴重な時間を有効に活用する習慣を身につけることをお勧めします。そして、時間にルーズにならないことです。

面白いことに、毎朝遅刻すれすれに出社する人は、フレックス勤務制度であるか否かには関係ありませんし、住んでいる場所と職場の距離とも関係ないように思えます。

私自身が会社で開催する勉強会は、朝の勤務時間前にしか開催しません。そのため、もともと朝の時間にルーズな人が参加を申し込んでも、ほとんどの人が数回で出席しなくなります。本を読み終える最終回まで参加を継続する人は、その後の勉強会にも引き続き参加する人が多いです。

いきものがかり コイスルオトメのPV

コイスルオトメ

• アーティスト: いきものがかり
• 出版社/メーカー: エピックレコードジャパン
• 発売日: 2006/10/18
• メディア: CD

いきものがかりの「コイスルオトメ」のプロモーションビデオを見たのですが、私が住んでいる「こどもの国」近辺で撮影されていました。

http://www.sonymusic.co.jp/?70003712_ESCL-2885&70003712_ESCL-2885_01VFL
（全編ではなく一部だけですが）

PVに出てくる大きな駐車場は、こどもの国線のこどもの国駅のホームの前の駐車場です。週末の天気が良い日は、この駐車場が家族連れの車でいつも満車になります。全編のプロモーションビデオを見ると、奈良小学校付近から長津田駅の手前までにかけて撮影されているようです。

コードレビューの視点 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言語に限らず、通常のデバッグ文やログ情報にも同じことが言えます。せっかくデバッグ文やログ情報を残すようにコードを書いても、いざ、デバッグ時に必要な情報（変数やポインタの値など）が出力されていないと何の役にも立ちません。

デバッグ文やログ情報出力は、コードの可読性を下げる場合が多いです。しかし、それでも必要があると判断して記述するのであれば、デバッグに必要なエラー情報はすべて表示したり記録したりすることが重要です。コードレビューをしていると、その点を全く考慮せず、デバッグ文やログ情報出力を書いているコードを多く見かけます。