レベル: 初級 Mark Roulo (mark.roulo@javaworld.com), コラムニスト, JavaWorld
2000年 3月 01日
コードの検査は、バグを除去する上で最も強力な手段の 1 つです。コードの検査が有益である理由の 1 つに、テスト時に見過ごされたエラーが発見されることが多いということがあります。検閲者が特定の間違いを探すようにするなら、コードの検査の効果はさらに高まります。テストするよりコードを読んだ方が見つけやすい間違いの場合は、特にそう言えます。ここでは、コードを検査することによって容易に見つけることのできるプログラミング上の間違いを何種類か取り上げます。
「あなたの隣人の目の中にあるごみには気付くのに、あなた自身の目の中にある丸太に気付かないのはなぜですか」
-- マタイによる福音書 7 章 3 節
他人が冒した間違いは、自分が冒した間違いよりも目に付きやすいものです。作家と編集者、運動選手とコーチの関係を見ても、他人の意見が価値あるものだと分かるでしょう。したがって、批評に逆らうのでなく、むしろ、自分のプログラミングの結果に含まれている欠点を見つける面で、他人の能力を活用すべきです。
正式なコードの検査は、コードの質を改善するために利用できる、最も強力な手法の 1 つです。コードの検査、つまりコードにバグがないか注意深く調べることは、コードのテストを補うものとなります。なぜなら、コードを検査することによって、テストで見つかるのとは別の種類の間違いが見つかることがあるからです。
検査する人が、バグを大ざっぱに探すのではなく、特定のエラーを注意深く探すなら、コードの検査の効果はさらに高まります。ここでは、わたしが現在携わっている Java コード・ベースでしばしば目にする一般的な間違いを 11 種類紹介します。以下の項目を、ご自分のコード検査用のチェックリストに含めておくなら、同じ種類の典型的な間違いを繰り返さないで済むでしょう。
一般的な間違い #1: 不必要に String をコピーすること
テストで検出しにくい間違いの 1 つは、不変オブジェクトのコピーを不必要に作成するというものです。不変オブジェクトは変更することができないため、これをコピーする必要はありません。不変オブジェクトのうち最も一般的に使用されるのは、String です。String の内容を変更する必要がある場合は、代わりにStringBuffer を使用しなければなりません。
以下の方法でString をコピーした場合、確かに機能はします。
String s = new String ("Text here"); |
しかし、上記のコードは必要以上に遅く、複雑です。これを次のように書くこともできます。
String temp = "Text here";
String s = new String (temp); |
しかし、このように書き直しても、不要なString が余分に含まれてしまいます。次のように単純に書く方が優れています。
警告: これは、コードに対してプリプロセッサーまたはポストプロセッサーを実行する場合 (たとえば、多くの ODMG-93 データベースの場合) にはあてはまらないことがあります。プリプロセッサーまたはポストプロセッサーはコードを変更することがあるので、String オブジェクトに見えるものが、実際にはString オブジェクトでないことがあり得ます。
一般的な間違い #2: 戻されたオブジェクトを複製し損なうこと
カプセル化は、オブジェクト指向プログラミングの主な概念の 1 つです。残念ながら Java では、プライベート・データへの参照を戻すことによって、カプセル化を破壊してしまうということが容易に起こり得ます。
その点を、以下のクラスで示します。
import java.awt.Dimension;
/**
* Example class. The x and y values should never
* be negative.
*/
public class Example
{
private Dimension d = new Dimension (0, 0);
public Example ()
{
}
/**
* Set height and width. Both height and width must be nonnegative
* or an exception is thrown.
*/
public synchronized void setValues (int height, int width)
throws IllegalArgumentException
{
if (height < 0 || width < 0)
throw new IllegalArgumentException();
d.height = height;
d.width = width;
}
public synchronized Dimension getValues()
{
// Ooops! Breaks encapsulation
return d;
}
} |
Example クラスでは、内部に保管される height 値と width 値が決して負の値にならないものとしています。setValues() メソッドを使用して負の値を設定しようとすると、例外エラーが起こって失敗することになります。残念ながらgetValues() は、d のコピーではなく、d への参照を戻すので、次のような間違ったコードを書いてしまう恐れがあります。
Example ex = new Example();
Dimension d = ex.getValues();
d.height = -5;
d.width = -10; |
これでは、Example オブジェクトに負の値が含まれてしまいます。getValues() メソッドの呼び出し元が、戻されるDimension オブジェクトの height 値と width 値を設定しない場合、テストではこの種のバグが見つからない恐れがあります。残念ながら、クライアント・コードの中には、戻されるDimension オブジェクト内の値を変更してしまうものがあると予想されます。結果として生じるバグを探し出すのは、退屈で時間のかかる作業です。マルチスレッド環境の場合は特にそう言えます。
もっと良い方法は、getValues() メソッドがコピーを戻すようにすることです。
public synchronized Dimension getValues()
{
return new Dimension (d.x, d.y);
} |
これで、Example オブジェクトの内部の値は安全です。呼び出し元はそれらの値を必要なだけコピーして変更することができますが、setValues() メソッドを使用しないでExample オブジェクトの内部の状態を変更することはできません。
一般的な間違い #3: 必要がないのに複製すること
get メソッドが、内部データへの参照ではなく、内部データ・オブジェクトのコピーを戻すことを学んだ開発者は、次のようなコードを書くことがあります。
/**
* Example class. The value should never
* be negative.
*/
public class Example
{
private Integer i = new Integer (0);
public Example ()
{
}
/**
* Set x. x must be nonnegative
* or an exception will be thrown.
*/
public synchronized void setValues (int x)
throws IllegalArgumentException
{
if (x < 0)
throw new IllegalArgumentException();
i = new Integer (x);
}
public synchronized Integer getValue()
{
// We can't clone Integers so we make
// a copy this way.
return new Integer (i.intValue());
}
}
|
上のコードは間違いではありません。それは、最初に挙げた事例において、余分のString を作成しても間違いではなかったのと同じです。しかし、どちらの場合も、必要以上の仕事をしています。Integer オブジェクトは、String オブジェクトと同様、いったん構成したなら変更不能になります。したがって、内部Integer オブジェクトをコピーの代わりに戻しても、問題はありません。
getValue() を次のように作成することが必要です。
public synchronized Integer getValue()
{
// 'i' is immutable, so it is safe to
// return it instead of a copy.
return i;
}
|
Java プログラムには、一般的な C++ プログラムよりも多数の不変オブジェクトが含まれています。JDK に付属の不変クラスの不完全なリストには、次のものが含まれています。
-
Boolean
-
Byte
-
Character
-
Class
-
Double
-
Float
-
Integer
-
Long
-
Short
-
String
- 大部分の
Exception サブクラス
一般的な間違い #4: 配列を手でコピーすること
Java では配列を複製することができますが、開発者が以下に挙げるようなコードを書くという間違いを冒すのは珍しいことではありません。問題は、Object の提供するclone メソッドが 1 行で行うことを、以下のループは 3 行使って行っているということです。
public class Example
{
private int[] copy;
/**
* Save a copy of 'data'. 'data' cannot be null.
*/
public void saveCopy (int[] data)
{
copy = new int[data.length];
for (int i = 0; i < copy.length; ++i)
copy[i] = data[i];
}
} |
このコードは間違っていませんが、必要以上に複雑です。saveCopy のより良いインプリメンテーションは、次のとおりです。
void saveCopy (int[] data)
{
try
{
copy = (int[])data.clone();
}
catch (CloneNotSupportedException e)
{
// Can't get here.
}
} |
配列をしばしば複製するのであれば、次のようなユーティリティー・メソッドを使うと便利な場合があります。
static int[] cloneArray (int[] data)
{
try
{
return (int[])data.clone();
}
catch (CloneNotSupportedException e)
{
// Can't get here.
}
} |
すると、saveCopy は見た目にも分かりやすくなります。
void saveCopy (int[] data)
{
copy = cloneArray ( data);
} |
一般的な間違い #5: 間違ったものをコピーすること
プログラマーは、コピーを戻さなければならないことは知ってはいても、不注意で間違ったものをコピーしてしまうことがあります。以下のコードは、開発者の期待どおりには機能しません。部分コピーしか行っていないためです。
import java.awt.Dimension;
/**
* Example class. The height and width values should never
* be negative.
*/
public class Example
{
static final public int TOTAL_VALUES = 10;
private Dimension[] d = new Dimension[TOTAL_VALUES];
public Example ()
{
}
/**
* Set height and width. Both height and width must be nonnegative
* or an exception will be thrown.
*/
public synchronized void setValues (int index, int height, int
width)
throws IllegalArgumentException
{
if (height < 0 || width< 0)
throw new IllegalArgumentException();
if (d[index] == null)
d[index] = new Dimension();
d[index].height = height;
d[index].width = width;
}
public synchronized Dimension[] getValues() throws
CloneNotSupportedException
{
return (Dimension[])d.clone();
}
}
|
ここで問題なのは、getValues() メソッドが配列を複製しているものの、配列にDimension オブジェクトが含まれていないということです。呼び出し元が内部配列を変更して別のDimension オブジェクトを指すようにすることは、clone() によって妨げられていますが、呼び出し元がDimension オブジェクトの内容を変更することは妨げられていません。getValues() を改良すると次のようになります。
public synchronized Dimension[] getValues() throws
CloneNotSupportedException
{
Dimension[] copy = (Dimension[])d.clone();
for (int i = 0; i< copy.length; ++i)
{
// NOTE: Dimension isn't cloneable.
if (d[i] != null)
copy[i] = new Dimension (d[i].height, d[i].width);
}
return copy;
} |
このような間違いは、int およびfloat などの アトミック・タイプの複数次元の配列でも起こります。int の 1 次元の配列は、単純に複製することができます。以下に例を示します。
public void store (int[] data) throws CloneNotSupportedException
{
this.data = (int[])data.clone(); // OK
} |
int の 2 次元の配列をコピーする場合は注意が必要です。Java には 2 次元の配列がないため、int の 2 次元の配列は、実際にはint[] タイプのオブジェクトの 1 次元の配列です。int[][] を単純に複製すると、上記の例の最初のバージョンのgetValues と同じ間違いを冒すことになるので、それを避けたいと思うことでしょう。以下のコードには、2 次元のint 配列を複製するための 正しい方法と間違った方法が示されています。
public void wrongStore (int[][] data) throws
CloneNotSupportedException
{
this.data = (int[][])data.clone(); // Not OK!
}
public void rightStore (int[][] data)
{
// OK!
this.data = (int[][])data.clone();
for (int i = 0; i< data.length; ++i)
{
if (data[i] != null)
this.data[i] = (int[])data[i].clone();
}
} |
一般的な間違い #6: new が NULL になるかどうかをテストすること
初心者の Java プログラマーは、new 操作の結果がnull になるかどうかのテストを行うことがよくあります。このテスト用コードは次のようなものです。
Integer i = new Integer (400);
if (i == null)
throw new NullPointerException(); |
このテストは間違いではありませんが、必要のないものです。if とthrow を構成している 2 行は無駄です。これらの行は、プログラムを肥大させ、処理速度を低下させているに過ぎません。
C/C++ プログラマーは、最初、このテストを行いがちです。C においてはmalloc() の結果のテストが必要であり、このテストをしないとバグが出るからです。C++ においては、new の結果をテストするのが適当な場合があります。テストすべきかどうかは、例外が使用可能かどうかに依存します (多くの C++ コンパイラーでは、例外を使用不可にすることができます。その場合、new が失敗すると、null が戻ります)。Java では、new がnull を戻すことは許されていません。許されているとしたら、おそらく JVM が破損して、テストは役に立たないでしょう。
一般的な間違い #7: .equals の代わりに == を使うこと
Java には、等価性をテストする 2 つの方法があります。== 演算子を使う方法と、すべてのオブジェクトにインプリメントされている.equals メソッドを使う方法です。アトミック・タイプ (int、float、char など) はオブジェクトではないため、== 演算子しか使えません。以下に例を示します。
int x = 4;
int y = 5;
if (x == y)
System.out.println ("Hi");
// This 'if' test won't compile.
if (x.equals (y))
System.out.println ("Hi");
|
オブジェクトの場合は注意が必要です。== 演算子は 2 つの参照が同じオブジェクトを指しているかどうかをテストしますが、equals() メソッドが実行する等価性のテストはそれほど明確なものではありません。さらに分かりにくいことに、java.lang.Object に備わっているデフォルトのequals メソッドは、単に比較の対象となる 2 つのオブジェクトが同一のものかどうかをテストする場合に == を使用します。
多くのクラスでは、もっと役に立つことを行うために、デフォルトのequals メソッドをオーバーライドしています。たとえばString クラスでは、2 つのString が同じ文字を同じ順序で含んでいるかどうかをテストします。Integer クラスでは、含まれているint 値が同じかどうかをテストするために、equals をオーバーライドしています。
ほとんどの場合、オブジェクトの等価性のテストにはequals メソッドを使用し、アトミック・タイプには== を使用することが必要です。
一般的な間違い #8: 非アトミックな操作とアトミックな操作を混同すること
Java では、32 ビット以下の読み取りまたは書き込みは、アトミックなものになることが保証されています。つまり、各アクションが 1 ステップで実行され、中断されることがないということです。したがって、そのような読み取りおよび書き込みの同期を取る必要はありません。以下のコードはスレッド・セーフです。
public class Example
{
private int value;
// More code here...
public void set (int x) // NOTE: No synchronized keyword
{
this.value = x;
}
} |
ただし、上記の保証は、読み取りと書き込みにしかあてはまりません。以下のメソッドはスレッド・セーフではありません。
public void increment ()
{
// This is effectively two or three instructions:
// 1) Read current setting of 'value'.
// 2) Increment that setting.
// 3) Write the new setting back.
++this.value;
} |
この間違いがテストの時に見つかることはおそらくありません。第一に、スレッド化のバグのテストは非常に難しく、時間もかかります。第二に、CPU によっては、このコードが単一の命令に変換され、結果として正しく機能することがあります。他の JVM でテストしないと、このバグは見つからないと考えられます。最初からコードを正しく同期する方が優れています。
public synchronized void increment ()
{
++this.value;
}
|
一般的な間違い #9: catch ブロックに終結処置コードを入れること
catch ブロックに終結処置コードを入れると、コードが次のようになることがあります。
OutputStream os = null; try { os = new OutputStream ();
// Do something with os here. os.close(); } catch (Exception e) { if (os != null)
os.close(); } |
このコードはいくつかの理由で間違っていますが、テストでは間違いが見過ごされる可能性が高いと言えます。このコードの問題点を 3 つ挙げます。
-
os.close() ステートメントが 2 個所に現れている。これは不要であるのみならず、保守上の問題を引き起こす可能性があります。
- 上記のコードは
Exception しか処理せず、Error を処理しない。try ブロックがError を生じて失敗した場合でさえ、ストリームが閉じないことになります。
-
close() が IOException をスローできる。
より良いバージョンは以下のとおりです。
OutputStream os = null;
try
{
os = new OutputStream ();
// Do something with os here.
}
finally
{
if (os != null)
os.close();
} |
これで最初の 2 つの問題は修正されました。コードに重複がなくなり、Errors が適正に処理されるようになりました。最後の問題については、良い解決方法がありません。最良の解決策は、おそらく try/catch ブロック自体にclose() を入れるというものです。
一般的な間違い #10: 不要な catch ブロックを追加すること
開発者の中には、try/catch ブロック という語を聞くと、try ブロックにはすべてcatch ブロックを関連付けなければならないと考える人がいます。C++ プログラマーは特にそうです。というのは、C++ にはfinally ブロックという概念がないからです。C++ の場合は、関連するcatch ブロックと対にするということが、try ブロックの唯一の存在理由です。
不要なcatch ブロックを追加すると、コードは以下のようになります。このコードでは、例外がキャッチされると即時に再スローされます。
try
{
// Nifty code here
}
catch (Exception e)
{
throw e;
}
finally
{
// Cleanup code here
} |
不要な catch ブロックを除去して、上記のコードを短くすることができます。
try
{
// Nifty code here
}
finally
{
// Cleanup code here
}
|
一般的な間違い #11: equals、hashCode、または clone のインプリメントに失敗すること
java.lang.Object に備わっているequals、hashCode、clone のデフォルトのインプリメンテーションは正しいものです。
残念ながら、上記のインプリメンテーションがいつも役に立つとは限りません。それを修正するため、多くのクラスでは上に挙げた 1 つまたは複数のメソッドをオーバーライドし、もっと役に立つ機能を実現しています。
残念ながら、上に挙げた 1 つまたは複数のメソッドをオーバーライドしたクラスを拡張すると、子クラスもそれらのメソッドをオーバーライドしなければならないのが普通です。コードの検査に際しては、親クラスがequals、hashCode、またはclone をインプリメントしているならば、子クラスも正しいかどうかを確かめる必要があります。equals、hashCode、およびclone の正しいインプリメンテーションを作成するのは容易ではありません。参考文献のセクションには、その方法を説明した文書へのリンクが挙げられています。
結論
上記の間違いはどれも、わたし自身がコードの検査において最低 1 回は遭遇したことのあるものです。事実、そのいくつかは、わたし自身も冒したことがある間違いです。ありがたいことに、コードの検査は簡単に行うことができます。間違いを探す方法さえ分かっていれば、それを見つけて修正するのは容易です。正式なコードの検査を行う時間がなくても、これらの間違いをコードから根絶するなら、デバッグの時間の節約となります。検査に費やす時間は、無駄になることがないのです。
参考文献
著者について  | | | Mark Roulo はJavaWorld の Java Tip のテクニカル・コーディネーターです。1989 年以来プログラミングを職業としており、Java 使用歴は alpha-3 リリースにさかのぼります。KLA-Tencor で常勤のかたわら、500KLOC 規模のイメージ処理用の分散並列マルチ・コンピューター・アプリケーション (大部分を Java で作成) などの作成チームに参加しています。メール・アドレスは、mark.roulo@javaworld.com。
※JavaWorld 誌の許可を得て再掲。Copyright ITworld.com, Inc., an IDG Communications company。 |
記事の評価
| 
