コードレビューの視点 018

情報を付加しないコメントに注意する

プログラミングにおいては日本語でプログラムを書くこと、つまりクラス名、メソッド名、変数名を日本語にしてプログラミングすることはまれだと思います。しかし、コメントを日本語で書くことは普通に行われています。そうすると、次のようなコードを目にすることになります。

// モデル情報を取得する
Model m = config.getModelInfo()

これはコードを読めば分かることを説明しているコメントに過ぎないのですが、日本語で書いてあるので何らかの情報を付加しているような印象を与えます。これを英語で書いたとしたら次のようになるでしょう。

// Gets the model info
Model m = config.getModelInfo()

明らかに「同じこと」を二回書いていることになります。したがって、そもそもこのようなコメントは書かないわけです。

コメントはコードから読み取れない、あるいは分かりにくいことを説明するために書くわけです。しかし、日本語で書くことにより次のようなコメントを目にすることがあります。

• 上記のようにメソッド名が表す意味をそのまま日本語で書いている
• メソッド名が不適切に命名されて処理内容が読み取れないので、日本語のコメントで説明している
• メソッド名が不適な命名で処理内容が読み取れないけれど、そのメソッド名がそのまま日本語に訳されてコメントとして書かれている

コードを読みやすく理解しやすいように書き、その上で読み手に付加情報を与えるためにコメントを書くのです。付加情報のない無意味なコメントが書かれていないか、メソッド名などを適切に変更すればコメントが不要になるのではないかという視点でコードをレビューする必要があります。

コードレビューの視点 017

防御的にプログラミングされているかに注意する

防御的プログラミングに関しては、過去に書いているこちらを参照してください。

書いたコードが正しく呼び出される限り、防御的プログラミングのためのコードは余分なコードのように思えてしまうかもしれません。しかし、様々なモジュールを結合してソフトウェアを作り上げていくと、常に正しく呼び出される保障はありません。

16年ぐらい前の話ですが、当時、ある若いエンジニアのコードをレビューした時に、引数の値の異常値を検査していなかったので、そのことを指摘したことがあります。その時の彼の回答は、「そのような異常値は、仕様書に書かれている正しい値の範囲外であり、そのような値が渡ってくることはありません」でした。

それに対して、「仕様書に書かれた通りの範囲の値が渡されるのは、すべてのデバッグが終わった時なので、検査をするコードを入れなさい」と返事したような気がします。

実際、防御的プログラミングは、それを行ったことによる効率的な障害調査を経験してみないと、必要性を実感できないものです。そのため、強制的に防御的プログラミングをさせる必要があるかもしれません。

ソフトウェアの品質を高める１つの方法として、コードレビューでは、防御的プログラミングを行っているかに注意を払う必要があります。

コードレビューの視点 016

ボクシングに注意する

Java言語に限った話ですが、不注意なボクシング/アンボクシングに注意する必要があります。

Javaが1.4の頃からプログラミングしている人であれば、自動ボクシングが言語仕様として存在しない頃からJavaを使用していわけですから、Booleanクラスとboolean型の違いを分かっていると思います。しかし、Java SE 5.0以降からJavaでプログラミングを始めた人は違いが分かっていない人が増えてきていると思います。

5.0は、2004年9月にリリースされていますので、すでに10年以上が経過しています。そのため、基本データ型（byte, short, char, int, long, float, double)とラッパークラス（Byte、Short、Character、Integer、Long、Float、Double）の違いを分からないままプログラミングしている人がいるのではないでしょうか。そのため、コードレビューなどで、以下のようなフィールド宣言を見かけたりします。

Boolean flag = false;

この場合、どうして「booleanではなく、Booleanなのですか？」という質問に対して、質問の意図を理解できないようであれば、ボクシングを理解していないと思って間違いないでしょう。

不注意なボクシングは、使用するメモリ量の増加や性能低下になってしまいます。その仕組みについては、初心者でもしっかりと理解しておくことが必要です。ボクシングに関しては、拙著『Java 2 Standard Edition 5.0 Tiger』の第2章「ボクシング/アンボクシング」を参考にしてもらうとよいかと思います。

コードレビューの視点 015

意図しない継承に注意する

Java言語でクラスを設計する場合に、継承されることを想定していなくても、実際には継承可能なコードを書いてしまうことが多いです。たとえば、次のコードを考えてみてください。

public class Point {
    private final int x;
    private final int y;

    public Point(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public int getX() { return x; }
    public int getY() { return y; }
}

このようなクラス宣言をした場合には、このクラスの使用方法は、1つではありません。単純にインスタンスを生成するだけでなく、クラスを継承して別のクラスを作成するという使用方法や、さらに、メソッドをオーバーライドして使用するなどができてしまいます。

ところが、コードレビューで、このようなコードを見かけたときに、「継承を前提とした設計なのですか？」と聞くと、「継承されることは想定していません」という返事が返ってくることがあります。Java言語では、継承できないように書くには、さらにクラスをfinal宣言するとかprivateのコンストラクタだけにするとかの手間をかけないといけないため、意図せずに継承可能なクラスを書いてしまうことが多いです。

しかし、継承される前提の設計でなければ、そもそも継承できないように書くべきです。コードレビューでは、その点を注意しなければなりません。特に、公開APIとなるようなクラスでは、なおさら注意する必要があります。

コードレビューの視点 014

可視性に注意する

プログラミング言語の多くは、モジュール化を行うための何らかの言語仕様上の仕組みを持っています。たとえば、Java言語やGo言語であればパッケージ（package）をまとまりとして扱います。APIの観点で言えば、パッケージ外に公開されるのか、パッケージ内に隠蔽されるのかという違いとなります。

Java言語では、何らかのIDEを使用している人がほとんどだと思います。その結果、新たなクラスを作成する場合、デフォルトではpublicと宣言されたクラスが生成されたりします。

コードをレビューしてよく指摘するのは、「このクラスはパッケージ外から使用されることを想定しているのですか」ということです。つまり、「クラスをpublicと宣言しているのですから、外部から使用するという設計ですよね」という質問になります。

しかし、「パッケージ外から使用されることは想定していません」と回答されることがあります。パッケージ外からの使用を想定していないのであれば、そもそも、使用できないように最初から書いておくべきなのです。

可視性に注意することは、クラス宣言だけでなく、メソッド宣言にも適用されます。public宣言されたクラスだからといって、すべてのメソッドがpublicというクラスは少ないはずです。

コードレビューでは、設計上の意図がきちんと反映されているかを確認する必要があります。特に、その意図の記述が使用しているプログラミング言語により可能ならば、きちんとその言語の機能を使用すべきです。その一つが、可視性を正しく記述することです。

コードレビューの目的

ソフトウェア開発で行うコードレビューは、1つの目的ではなく、様々な目的を持ちます。きちんと機能が実装されているかを確認することは、目的の1つです。しかし、それだけではないわけです。

他の目的の1つとして、作成したソフトウェアエンジニアのスキルレベルを知って、どこが弱いのかを把握し、指導する場でもあるわけです。弱い部分には色々な側面があります。その1つとして、第3者にとって分かりやすく書かれているかも含まれます。

ソフトウェア開発では、ソースコードを作成するだけでなく、様々な成果物があり、それらもレビューされます。たとえば、様々な文書もそうです。書かれた文書が、書いた本人にしか理解できないし、誤解を招くような日本語で書かれているといったことは、レビューを通して指摘し、修正させることを繰り返すことが必要です。その結果、本人のレベルが向上して、誰が読んでも誤解することなく分かりやすく書けるようになるわけです。

もし、担当者が書いたものを何もレビューしなければ、担当者のスキルレベルが低い場合には、役に立たないがらくたの文書が作成されるだけであり、担当者は何年たっても質の低い文書を書き続けるかもしれません。

「コードレビュー」と言っても、行っていないソフトウェア開発組織もあるでしょうし、きちんとしたレビューを受けたり、指導をしたこともなく、ソフトウェア開発をしなくなった管理職もいると思います。その結果、「コードレビュー」をすることに関しては、文書をレビューすることと比べて、時間がかかり過ぎるとか無駄だと思う人がいてもおかしくはありません。

コードレビューには、様々な目的が含まれていますが、ソフトウェア開発組織にとって重要なのは、色々な意味で技術的負債とならないコードにすることと、ソフトウェアエンジニアのスキルレベルを長期的に上げるためということです。

コードレビューの視点 013

IDEが生成したコメントをそのままにしない

EclipseやNetBeansを使用して、新規にクラスを生成しようとすると、以下のようなコメント行が挿入されたりします。

/*
 * To change this license header, choose License Headers in Project Properties.
 * To change this template file, choose Tools | Templates
 * and open the template in the editor.
 */

あるいは次のようなコメント行だったりします。

// TODO code application logic here

初心者なら、IDEが生成したこのようなコメントを放置するかもしれません。そして、コードレビューで指摘されて削除したり、修正したりすることになるわけです。

初心者でない場合には、より悪い状況です。まず、開発経験が何年あっても、細部に注意を払えないエンジニアだと私は判断します。そして、そのような人がレビューアーとしてレビューしたコードの品質はたかが知れているので、初心者をレビューする立場になっても、きちんと指導できないと判断します。さらに、そのようなコメントを削除したり修正しなさいという指示は、その開発者への信頼口座からの引き落としになります（「信頼残高へマイナスである指示」）。

コードレビューの視点 012

言語固有の命名規約に従う

『Effective Java 第2版』の「項目56 一般に受け入れている命名規約を守る」は、Java言語に関する命名規約が説明されています。

昔は、1つの言語だけでプログラミングをすることが多かったです。しかし、2つ以上の言語を使用して開発をすることが普通になってきています。複数の言語間を行ったり来たりする際には、使用している言語で一般に受け入れている命名規約に従うことに注意しなければなりません。

複数言語を使用しなくても、新たな言語を学んでプログラミングする際には、慣れ親しんだ言語の命名規約を一度忘れて、その新たな言語の命名規約を学ぶ必要があります。

コードをレビューしていると、言語固有の規約に従っていないコードに出会ったり、無頓着な開発者に出会ったりします。

コードレビューの視点 011

英語の綴りには注意する

普段、英語を読むことがほとんどない人が書くプログラムでは、やたらと英単語の綴りの間違いが多いことがあります。

分かりやすい例で言えば、Java研修でGUI課題として「デジタル時計」を作成してもらうのですが、「DegitalClock」というクラス名で作成する人が必ず2、3割はいます。業務で開発してもらっているソースコードのレビューでも英単語の綴り間違いを指摘することが多いです。まだ、それでも綴りが間違っているだけなら良いのですが、それ以前に、意図を表す適切な名前になっていないこともあります。

コメントを除いて、日本語やローマ字でプログラミングすることはまれだと思います。英単語の綴りに自信がない時は、辞書で確認するように心がけてください。

コードレビューの視点 010

コメントアウトされたコードは削除する

大学時代や就職した頃はソースコード管理を使用することなくソフトウェアの開発を行っていたので、不要になったコードも後で必要になるかとか、あるいは、参考になるかということで、コメントアウトしていることが多かったような気がします。

今日、普通のソフトウェア開発であればSubversionやGitなどのソースコード管理システムを使用している訳でからコメントアウトしたコードを残している理由はほとんどありません。古いバージョンを取り出してくれば削除された部分を見ることはできます。

逆に、コメントアウトして残す場合には、残している意図もコメントとして残す必要があります。しかし、何らかの意図があって残されていることはほとんどなく、コードレビューでは削除を指示することが多いです。