FindBugs とは、プログラム中に存在するバグを検出するツールです。
プログラミングで問題となり得るバグパターンを検知し、ユーザにそれを知らせます。
以下、FindBugs が定義するバグパターンの一覧と簡単なサンプルコードを示します。
対象バージョンは 1.2.1 です。
Bad practice
このカテゴリのバグパターンは、「バッド・プラクティス」。
良くないコード記述法を指します。
AM: Creates an empty jar file entry
空のjarファイルを作成しています。
putNextEntry() メソッド呼出の後、すぐに closeEntry() を呼び出しています。
jar圧縮するコンテンツは putNextEntry() メソッドを呼び出した後で
書き込み、最後に closeEntry() メソッドを呼び出す必要があります。
はっきり言ってしまえば、このバグパターンは java.util.jar パッケージの
クラス設計がわかりにくい為に存在しているようなものです。
AM: Creates an empty zip file entry
空のzipファイルを作成しています。
putNextEntry() メソッド呼出の後、すぐに closeEntry() を呼び出しています。
BC: Equals method should assume anything about the type of its argument
equalsメソッドはどんな値に対しても正常に働く必要があります。
class SomeObject {
public boolean equals(Object o) {
SomeObject taregt = (SomeObject)o;
return id == target.id;
}
}
上のようなコードでは、oがSomeObject以外のクラスだった場合に
正しく働きません(例外が発生する)。
こういった場合、false を返すようにすべきです。
class SomeObject {
public boolean equals(Object o) {
if (o instanceof SomeObject) {
SomeObject taregt = (SomeObject)o;
return id == target.id;
}
return false;
}
}
BC: Random object created and used only once
乱数を生成するのに、Randomオブジェクトを毎回生成してそれを使っています。
int getRandomNumber() {
return new Random().nextInt();
}
このようなRandomオブジェクトの使用は効率的でなく、
適切な乱数を生成することも出来ません。
Randomオブジェクトは一度生成したら、それをプログラムの中で使い回します。
こうすることによって、より精度の高い乱数を発生させることができます。
ちなみに、さらに本格的な(より予想されにくい)乱数を生成したい場合には
java.security.SecureRandom を使いましょう。
CN: Class implements Cloneable but does not define or use clone method
クラスが Cloneable を実装していますが
cloneメソッドを定義しておらず、かつcloneメソッドはどこからも呼び出されていません。
public class ClassA implements Cloneable {
// cloneメソッドが定義されていない
// cloneメソッドはどこからも呼び出されていない
}
Cloneable の宣言を外すか、clone メソッドを実装するか、
どちらかにしましょう。
CN: clone method does not call super.clone()
cloneメソッドがスーパークラスのcloneメソッドを呼び出していません。
この場合、全てのクラスのスーパークラスであるObjectクラスの
cloneメソッドが呼び出されない事になります。
全ての clone メソッド内で super.clone() を呼び出せば、
Object.clone() が呼び出されることが保証されます。
これは、常に正しい型のオブジェクトを返すことを意味します。
public class ClassA implements Cloneable {
protected Object clone() throws CloneNotSupportedException {
// super.clone() が呼び出されていない
}
}
Co: Abstract class defines covariant compareTo() method
引数の異なるcompareToメソッドが定義されています。
Comparable インターフェイスのcompareToメソッドをオーバーライドする場合、
Object を引数に取る必要があります。
Co: Covariant compareTo() method defined も同様です。
DE: Method might drop exception
メソッド内で発生した例外を捨てている可能性があります。
DE: Method might ignore exception
メソッド内で発生した例外を無視している可能性があります。
DP: Classloaders should only be created inside doPrivileged block
クラスローダは、doPrivileged block 内で作成される必要があります。
AccessController.doPrivileged(
new PrivilegedAction<Object>() {
public Object run() {
// ここでクラスローダ作成を行う
}
});
doPrivileged block についてはあまり良くわかりませんが、
上のように java.security パッケージを使うと良いようです。
DP: Method invoked that should be only be invoked inside a doPrivileged block
リフレクションを使ったメソッド呼出は、doPrivileged block 内で行われる必要があります。
Dm: Method invokes System.exit(...)
System.exit(...) メソッドを呼び出しています。
このメソッドはJavaVMを強制終了させる為、テスト以外では使用されるべきではありません。
Dm: Method invokes runFinalizersOnExit, one of the most dangerous methods in the Java libraries.
System.runFinalizersOnExit メソッドを呼び出しています。
これは、Javaライブラリの中でも最も危険なメソッドのうちの一つです。
呼び出してはいけません。
ES: Comparison of String parameter using == or !=
パラメータとして与えられた文字列を == や != を用いて比較しています。
ES: Comparison of String objects using == or !=
文字列を == や != を用いて比較しています。
文字列の比較はequalsメソッド等を使用する必要があります。
public void func1(String obj1, String obj2) {
if (obj1.equals(obj2)) {
// 文字列の比較をequalsメソッドで行っている。正しい
}
}
public void func2(String obj1, String obj2) {
if (obj1 == obj2) {
// 文字列の比較を == 演算子で行っている。間違っている可能性が高い
}
}
Eq: Abstract class defines covariant equals() method
Abstract クラス内に共変な（引数がObjectでない）equals メソッドが定義されています。
Eq: Class defines compareTo(...) and uses Object.equals()
compareTo() メソッドが定義されているのに、equals() メソッドが定義されていません。
Javaのドキュメントには x.compareTo(y) == 0 <=> x.equals(y) を満たすことが
強く推奨されていて、これを実現するには equals() メソッドをオーバーライドする必要があります。
Eq: Covariant equals() method defined
引数の異なるequalsメソッドが定義されています。
Objectクラスのequalsメソッドをオーバーライドする場合、
Object を引数に取る必要があります。
Eq: Covariant equals() method defined, Object.equals(Object) inherited
共変な（引数がObjectでない）equals メソッドが定義されていますが、
Object.equals(Object) は継承のままです。
大抵の場合、equals(Object) をオーバーライドすべきです。
public class ClassA {
public boolean equals(ClassA obj) {
// おそらくこのメソッドはObjectを引数に取るべき
}
}
FI: Empty finalizer should be deleted
空のfinalizerは削除されるべきです。
protected void finalize() throws Throwable {
// 定義が空。削除すべき
}
finalizer を定義すると、そのインスタンスを使うことによって掛かるコストが大幅に増加します。
空定義ならば、削除しておいた方が良いでしょう。
FI: Explicit invocation of finalizer
finalize() メソッドを強制的に呼び出しています。
通常、finalizer はオブジェクトが消滅する際にVMから一度だけ呼び出されるものです。
よって、明示的な呼び出しをする事は良くない考えです。
public void func() throws Throwable {
finalize(); // 明示的な finalize() の呼び出しは避けるべき
}
FI: Finalizer nulls fields
ファイナライザがインスタンスにnullを代入しています。
これは通常間違っていて、nullを代入してもオブジェクトのGCは助長されません。
確実にGCしたいのなら、別の方法を取るべきです。
FI: Finalizer only nulls fields
上と同様です。
ファイナライザがnullを代入する事しかしていません。
こういったファイナライザは削除すべきです。
FI: Finalizer does not call superclass finalizer
finalizerがスーパークラスのfinalizerを呼んでいません。
protected void finalize() throws Throwable {
// super.finalize() が存在しない
}
これはJavaが持つ弱点の一つです。
いちいち毎回 super を呼び出すのは面倒ですし、忘れる事もあるのですから。
FI: Finalizer nullifies superclass finalizer
finalizerが空です。
この場合、Object.finalizer() が実行されません。
こういったfinalizerは削除しましょう。
FI: Finalizer does nothing but call superclass finalizer
単にスーパークラスのfilalizeを呼び出しているだけのfinalizerは削除されるべきです。
protected void finalize() throws Throwable {
super.finalize();
// super.finalize() を呼び出しているだけ。削除すべき
}
HE: Class defines equals() but not hashCode()
クラスがequalsメソッドを定義(オーバーライド)しているのに
hashCodeメソッドを定義(オーバーライド)していません。またはその逆です。
通常、これらのメソッドは両方オーバーライドしないと意味を成しません。
public class ClassA() {
public boolean equals(Object obj) {
// equalsメソッドをオーバーライドしている
}
// ClassA では hashCode() をオーバーライドしていない。これは間違っている可能性が高い。
}
以下のパターンも同様です。
HE: Class defines equals() and uses Object.hashCode()
HE: Class defines hashCode() but not equals()
HE: Class defines hashCode() and uses Object.equals()
HE: Class inherits equals() and uses Object.hashCode()
IC: Superclass uses subclass during initialization
あるクラスの初期化時に、そのサブクラスを使用しています。
この段階で、まだサブクラスは初期化されていません。
これによって思わず誤動作をする可能性があります。
public class CircularClassInitialization {
static class InnerClassSingleton extends CircularClassInitialization {
static InnerClassSingleton singleton = new InnerClassSingleton();
}
static CircularClassInitialization foo = InnerClassSingleton.singleton;
}
このようにすると foo は null になってしまうらしいのですが…
こんな複雑な事する人、いますか(笑)？
IMSE: Dubious catching of IllegalMonitorStateException
疑わしい IllegalMonitorStateException のcatchがあります。
この例外は RuntimeException のサブクラスなので
通常プログラム内でcatchする必要はありません。
ISC: Needless instantiation of class that only supplies static methods
staticメソッドしか実装していないユーティリティクラスをインスタンス化しています。
public class SomeUtils {
...
public static void someFunc() {
...
}
...
}
new SomeUtils().someFunc(); // Oh!
これを未然に防ぐためにも、
ユーティリティクラスのデフォルトコンストラクタを private で定義しておくと良いでしょう。
public class SomeUtils {
private SomeUtils() {
// Can't call
}
}
new SomeUtils().someFunc(); // compile error!!
It: Iterator next() method can't throw NoSuchElement exception
Iterator.nextメソッド内に NoSuchElementException をthrowするロジックがありません。
nextメソッドでは、次の要素が無い状態で呼ばれたときに
適切に NoSuchElementException をthrowする必要があります。
J2EE: Store of non serializable object into HttpSession
シリアライズ不可能なオブジェクトを HttpSession 内に格納しています。
これは、セッションがシリアライズされるときに例外を発生します。
セッションをシリアライズしないという前提ならば構いませんが…
NP: Clone method may return null
clone() メソッドが null を返す場合があります。
このメソッドは null を返してはいけません。
protected Object clone() throws CloneNotSupportedException {
...
if (valid()) {
...
}
// this path is unreachable
return null;
}
こういった場面では代わりに AssertionError を throw しましょう。
NP: equals() method does not check for null argument
equals() メソッド内で、パラメータのnullチェックを行っていません。
NP: toString method may return null
toString() メソッドが null を返す場合があります。
このメソッドは null を返すべきではありません。
代わりに空文字を返すなどの処置をしましょう。
Nm: Class names should start with an upper case letter
クラス名は大文字で始まるべきです。
っていうかホント、Javaやる前にそれ位のルールは学習しておかないと…
Nm: Class is not derived from an Exception, even though it is named as such
あるクラスが、例外クラスではないのに Exception で終わるクラス名で定義されています。
これは混乱を招く可能性があります。
Nm: Confusing method names
同一クラス内で、大小文字の違いだけによる複数のメソッドが定義されています。
これは紛らわしいだけでなく、
リフレクト処理を使っている場面などにおいては致命的なバグになり得ます。
public class ClassA {
public void setValue() {
}
public void setvalue() {
}
// setValue と setvalue は大小文字の違いだけ。紛らわしいので止めた方が良い
}
Nm: Field names should start with an lower case letter
フィールド名は小文字で始まるべきです。
VBからの移行者は、このルールを平気で破るのです。
Nm: Use of identifier that is a keyword in later versions of Java
次期バージョンのJavaではキーワードとなる識別子が使用されています。
例えば、以前ならenumやassertはキーワードではなかったので
こういった名称を変数名に付けてしまったプロジェクトなどでは
新バージョンへの移行に手間が掛かることになってしまいます。
Nm: Method names should start with an lower case letter
メソッド名は小文字で始まるべきです。
これも上と同じ。JavaにはJavaのルールがあるのです。
あなただけにしか通用しないローカルルールは、現場では決して受け入れられません。
Nm: Very confusing method names (but intentional)
非常に紛らわしいメソッド名が付けられています。
詳細は不明です。
ODR: Method may fail to close database resource
メソッド内で生成されたデータベースリソース(ConnectionやStatement、ResultSet)が
closeされないままメソッドが終了してしまう可能性があります。
public void func(Connection conn) throws SQLException {
Statement stmt = conn.createStatement();
stmt.executeQuery("...");
// Statement がcloseされていない
}
ODR: Method may fail to close database resource on exception
メソッド内で生成されたデータベースリソース(ConnectionやStatement、ResultSet)が
closeされないままメソッドが終了してしまう可能性があります。
これらのリソースは、途中で例外が発生した場合でもメソッド内でcloseする必要があります。
finally節を使いましょう。
public void func(Connection conn) throws SQLException {
Statement stmt = conn.createStatement();
// 次の文で例外が発生すると、Statementがcloseされないままメソッドが終了してしまう
stmt.executeQuery("...");
stmt.close();
}
OS: Method may fail to close stream
メソッド内で生成されたストリームが
closeされないままメソッドが終了してしまう可能性があります。
public void func(String name) throws IOException {
InputStream stream = new FileInputStream(name);
stream.read();
// InputStream がcloseされていない
}
OS: Method may fail to close stream on exception
メソッド内で生成されたストリームが
closeされないままメソッドが終了してしまう可能性があります。
これらのストリームは、途中で例外が発生した場合でもメソッド内でcloseする必要があります。
public void func(String name) throws IOException {
InputStream stream = new FileInputStream(name);
// 次の文で例外が発生すると、InputStreamがcloseされないままメソッドが終了してしまう
stream.read();
stream.close();
}
RR: Method ignores results of InputStream.read()
InputStream.read() メソッドの戻り値を無視しています。
public void func(String name) throws IOException {
InputStream stream = new FileInputStream(name);
try {
byte[] buff = new byte[100];
stream.read(buff, 0, 100);
// readメソッドの戻り値を無視している。
// このメソッドは実際に読み込まれたバイト数を返すので、無視しない方が良い
} finally {
stream.close();
}
}
RR: Method ignores results of InputStream.skip()
InputStream.skip() メソッドの戻り値を無視しています。
SI: Static initializer for class creates instance before all static final fields assigned
static-initializer内で
finalフィールドを初期化し終わる前にインスタンスの生成をしようとしています。
まず全てのfinalフィールドを初期化してから、他の処理を行うようにしましょう。
SQL: Nonconstant string passed to execute method on an SQL statement
動的に生成したSQL文を使用しています。
これはSQLインジェクションの危険性があるので
Prepared Statement を使いましょう。
SQL: A prepared statement is generated from a nonconstant String
Prepared Statement を使っていますが、その中で
動的に生成したSQL文を使用しています。
これでは Prepared Statement の意味がありません。
SW: Certain swing methods should only be invoked from the Swing event thread
Swingの show / setVisible / pack メソッドが、イベントスレッドからコールされていません。
詳細は不明ですが、このようなコーディングはデッドロックやその他の不具合が
発生する可能性があるらしいです。
frame.show();
上のコードは、以下のように書き換えなければなりません。
private static class FrameShower implements Runnable {
final Frame frame;
public FrameShower(Frame frame) {
this.frame = frame;
}
public void run() {
frame.show();
}
}
void func() {
...
EventQueue.invokeLater(new FrameShower(frame));
...
}
EventQueueクラスは java.awt パッケージにあります。
このようにすると、キューに登録されたイベントが順次実行され
マルチスレッド上からも安全に使用することが出来る…らしいです。
Se: Non-transient non-serializable instance field in serializable class
直列化可能なクラス内で、非直列化可能なフィールドが定義されています。
このフィールドは transient にするか、
独自の writeObject / readObject メソッドを定義する必要があります。
public class ClassA implements Serializable {
private Iterator it;
// Iteratorは非直列化フィールドなので、正しくシリアライズされない可能性がある。
// transient にするか、read/writeObject メソッドを実装して下さい。
}
Se: Non-serializable class has a serializable inner class
直列化不可能なクラスが、直列化可能なインナークラスを持っています。
このインナークラスはシリアライズ可能ですが、その際に
直列化不可能なアウタークラスをシリアライズしようとして例外が発生してしまいます。
public class Outer {
class Inner implements Serializable {
...
}
public Inner inner = new Inner();
}
public class Client {
public void write(ObjectOutputStream out, Inner innerObject) {
out.writeObject(innerObject); // ここで NotSerializableException が発生
}
}
これの一番簡単な解決法は、インナークラスをstaticにすることです。
アウタークラスを直列化可能にするという手法もありますが
それは本当にアウタークラスをシリアライズする必要があるときだけにしましょう。
Se: Non-serializable value stored into instance field of a serializable class
直列化可能クラスのフィールドに、非直列化可能な値を代入しています。
このフィールドをシリアライズしようとしたときに、例外が発生するでしょう。
Se: Comparator doesn't implement Serializable
Comparator を実装したクラスに、Serializable が実装されていません。
ほとんどの場合、Comparator を実装したクラスは
ごくわずかの状態(フィールド)しか存在しないか、完全なStatelessです。
これをシリアライズするのは簡単で、かつ良いコーディング法だとされています。
Se: Serializable inner class
Serializableなインナークラスが定義されています。
このクラスのインスタンスをシリアライズするとき、
そのアウタークラスまでシリアライズされます。
これの一番簡単な解決法は、インナークラスをstaticにすることです。
Se: Method must be private in order for serialization to work
独自の writeObject / readObject メソッドが定義されていますが、
アクセス識別子が private になっていません。
これはうまく働きません。
Se: serialVersionUID isn't final
serialVersionUID が final として定義されていません。
public class ClassA implements Serializable {
static long serialVersionUID = 8306714797267301396L; // finalにして下さい
}
Se: serialVersionUID isn't long
serialVersionUID がlong型で定義されていません。
public class ClassA implements Serializable {
static final int serialVersionUID = 12345; // long型にして下さい
}
Se: serialVersionUID isn't static
serialVersionUID が static として定義されていません。
public class ClassA implements Serializable {
final long serialVersionUID = 8306714797267301396L; // staticにして下さい
}
Se: Class is Serializable but its superclass doesn't define a void constructor
非直列化可能なクラスから派生した直列化可能なクラスにおいて、
その非直列化可能なクラスに引数無のコンストラクタが定義されていません。
具体例を挙げます。
public class ClassA { // このクラスは非直列化可能
private int i;
public ClassA(int i) {
this.i = i;
}
// 引数付のコンストラクタを定義したので、
// デフォルト(引数無)コンストラクタは定義されません。
}
public class ClassB extends ClassA {
private int j;
// スーパークラスであるClassAはnon-serializableであるが、
// 引数無のコンストラクタが定義されていない。
}
この場合、ClassBクラスのインスタンスを直列化(serialize)する際には
ClassBのフィールドのみ直列化され、ClassAのフィールドは直列化されません。
そしてこれを直列化復元(deserialize)する際、
ClassBのフィールドは正しく復元されますが
ClassAのフィールドは直列化されていないので復元することが出来ません。
こういった場合、JavaVMはClassAの引数無コンストラクタを呼び出すことによって
ClassAのフィールドを復元することを試みます。
この例のように、スーパークラスに引数無コンストラクタが存在しない場合には
実行時例外(InvalidClassException)が発生してしまいます。
Se: Class is Externalizable but doesn't define a void constructor
クラスはExternalizableですが、引数無のコンストラクタが定義されていません。
Externalizableであるクラスのインスタンスを復元するとき、
JavaVMはクラスの引数無コンストラクタを呼び出した後で
readExternalメソッドを呼び出します。
もし引数無コンストラクタが存在しない場合、
直列化／直列化復元の際に実行時例外(InvalidClassException)が発生してしまいます。
Se: The readResolve method must be declared with a return type of Object
readResolveメソッドは、Object型の戻り値を返すように定義する必要があります。
Se: Transient field that isn't set by deserialization
transient宣言されたフィールドがありますが、
そのフィールドは直列化復元時に値がセットされません。
SnVI: Class is Serializable, but doesn't define serialVersionUID
クラスは直列化可能(Serializable)ですが、serialVersionUIDが定義されていません。
public class SerClassA implements Serializable {
// serialVersionUIDが定義されていない
}
UI: Usage of GetResource may be unsafe if class is extended
this.getClass().getResource(...) メソッドが呼び出されていますが、
このクラスの派生クラスが別パッケージで定義された場合にこれは別の結果を返します。
package a;
public class ClassA {
public Resource getResource() {
return this.getClass().getResource("resource");
}
}
package b;
public class ClassB extends ClassA {
}
public class Main {
void func() {
new ClassA().getResource(); // a.resource のリソースが返される
new ClassB().getResource(); // b.resource のリソースが返される
}
}
解決方法はいくつかあります。
例えば以下のようにクラス名を直接指定する方法があります。
return ClassA.class.getResource("resource");
Correctness
このカテゴリのバグパターンは、正しくない式やメソッドの使用を指します。
BC: Impossible cast
常に ClassCastException を引き起こすようなキャストを使用しています。
BC: instanceof will always return false
instanceof 構文を使っていますが、その値が常に false を返します。
これは安全ですが、記述が間違っている可能性が高いです。
BIT: Incompatible bit masks (BIT_AND)
不適切なビットマスクの使用法です。
例えば、 (value & 4) と 7 を比較しています。
この式の結果は value の値に関わらず、常に false になります。
BIT: Incompatible bit masks (BIT_AND_ZZ)
不適切なビットマスクの使用法です。
例えば、 (value & 0) と 0 を比較しています。
この式の結果は value の値に関わらず、常に true になります。
BIT: Incompatible bit masks (BIT_IOR)
不適切なビットマスクの使用法です。
例えば、 (value | 5) と 4 を比較しています。
この式の結果は value の値に関わらず、常に false になります。
BIT: Bitwise OR of signed byte value (BIT_IOR_OF_SIGNED_BYTE)
byte配列から取得した値に対して | 演算子を使用しています。
この値は符号付き整数なので、演算は意図しない結果を返す可能性があります。
| 演算子は通常、正数のみに使うからです。
BOA: Class overrides a method implemented in super class Adapter wrongly
このクラスは、親クラス(イベントのAdapter)のメソッドをオーバーライドしています。
このメソッドはイベントが発生したときにコールされません。
Bx: Primitive value is unboxed and coerced for ternary operator
以下のようなケースでは、プリミティブ型の値が強制的にUnboxされてしまいます。
Integer e1 = new Integer(2);
Float e2 = new Float(1.52);
float f = (b ? e1 : e2);
詳細は不明ですが、これによって多少のパフォーマンス劣化が生じるという事でしょうか。
DLS: Overwritten increment
インクリメント演算子の処理が、他の処理によって上書きされています。
int i = 10;
i = i++; // 結果は10となる。つまり、インクリメント処理は実行されない
DMI: Bad constant value for month
日付の月に 0～11 以外の値が使われています。
よくある間違いは、1～12 を使ってしまう事です。
DMI: hasNext method invokes next
hasNext メソッドの中から next メソッドを呼び出しています。
こういった使い方はほとんどの場合、間違っています。
hasNext メソッド呼び出しによりイテレータの位置を移動させてはいけません。
DMI: Invocation of toString on an array
配列に対してtoStringメソッドを呼び出しています。
これは、[C@16f0472 のような意味の無い文字列を返すだけです。
代わりに、Arrays.toString を使いましょう。
DMI: Double.longBitsToDouble invoked on an int
Double.longBitsToDouble() メソッドを呼び出していますが、引数にintを渡しています。
引数には long を渡す必要があります。
Dm: Can't use reflection to check for presense of annotation with default retention
Annotation(注釈)は、明示的にRetentionを指定しない限り
リフレクションを使った参照が出来ません。
// デフォルトでは、注釈情報はVM実行時に参照できない
public @interface DefaultAnnotation {
}
// 注釈情報をVM実行時に参照するには、Retentionを付ける必要がある
@Retention(RetentionPolicy.RUNTIME)
public @interface RuntimeAnnotation {
}
@DefaultAnnotation
@RuntimeAnnotation
class ClassA {
...
}
class Sample {
public void func() {
// DefaultAnnotationは参照できない
ClassA.class.getAnnotation(DefaultAnnotation.class);
// RuntimeAnnotationは参照できる
ClassA.class.getAnnotation(RuntimeAnnotation.class);
}
}
EC: equals() used to compare array and nonarray
配列と配列以外のオブジェクトを equals() で比較しています。
これはおそらく意図するロジックではないはずです。
EC: Invocation of equals() on an array，which is equivalent to ==
配列に対して equals() メソッドを呼び出しています。
これは == 演算子と同じ結果を返します。
配列の内容を比較するには java.util.Arrays.equals(Object[], Object[]) を使用します。
EC: Call to equals() with null argument
equalsメソッドの引数にnullを渡しています。
これは常に false を返します。
EC: Call to equals() comparing unrelated class and interface
Object.equals() を呼び出していますが、関連の無いクラス又はインターフェイス同士を比較しています。
このとき常に false を返しますが、おそらく意図する動作ではないはずです。
public void func1(String obj1, Object obj2) {
if (obj1.equals(obj2)) {
// String は Object のサブクラスなので、上のequalsメソッド呼び出しは正しい
}
}
public void func2(String obj1, Float obj2) {
if (obj1.equals(obj2)) {
// String と Float は型が異なるので、上のequalsメソッド呼び出しは常にfalseを返す
// よって、間違った記述である可能性が高い
}
}
EC: Call to equals() comparing different interface types
Object.equals() を呼び出していますが、異なるインターフェイス同士を比較しています。
このとき常に false を返しますが、おそらく意図する動作ではないはずです。
EC: Call to equals() comparing different types
Object.equals() を呼び出していますが、異なるクラス同士を比較しています。
このとき常に false を返しますが、おそらく意図する動作ではないはずです。
Eq: Covariant equals() method defined for enum
Enumクラス内に共変な equals メソッドが定義されています。
詳細は不明です。
FE: Doomed test for equality to NaN.
ある数値を、NaNと比較しようとしています。
しかしこれは常に false を返します。
double d = Double.NaN;
if (d == Double.NaN) {
// 上の式は常にfalse
}
if (Double.NaN == Double.NaN) {
// これも常にfalse
}
if (Double.isNaN(d)) {
// こうすると正しく動く
}
ちなみに、NaNとは0やnullとは異なります。
NaNは「Not A Number」。つまり「数値ではない何か」という意味です。
SQLのNULLと考え方は似ています。SQLでも NULL != NULL ですね。
GC: No relationship between generic parameter and method argument
Genericメソッドに関係しているらしいですが…詳細は不明です。
HE: Use of class without a hashCode() method in a hashed data structure
ハッシュ値を使う構造体の値に、hashCodeメソッドが定義されていないクラスの
インスタンスが格納されています。
通常、こういったクラスには equals および hashCode メソッドを実装する必要があります。
Map map = new HashMap();
MyObj myObj = new MyObj();
map.put("key", "abc"); // StringにはhashCodeメソッドが実装されている
map.put(myObj, "abc"); // MyObjにhashCodeメソッドが実装されていない
ICAST: Integer shift by an amount not in the range 0..31
Integer 型の数値を 0～31 以外の値でシフト演算しています。
ICAST: int value cast to double and then passed to Math.ceil
int型の数値をdouble型にキャストして、その値を
Math.ceil メソッドに渡しています。
Math.ceil(3); // Math.ceilはdoubleを引数に取るため、3はdoubleにキャストされる
これは意味がありません。
ICAST: int value cast to float and then passed to Math.round
int型の数値をfloat型にキャストした後で、その値を
Math.round メソッドに渡しています。
IJU: JUnit assertion in run method will not be noticed by JUnit
Thread.run メソッド内でJUnitのアサーションを発生させても
それはJUnitに通知されません。
IJU: TestCase declares a bad suite method
テストケース内で間違った suite メソッドが定義されています。
正しい suite メソッドの記述法は
public static junit.framework.Test suite()
または
public static junit.framework.TestSuite suite()
のどちらかです。
IJU: TestCase has no tests
TestCase クラスに一つもテストメソッドが定義されていません。
IJU: TestCase implements setUp but doesn't call super.setUp()
TestCaseクラス で setUp メソッドを実装していますが、
その内部で super.setUp() を呼び出していません。
ちなみに、JUnit4を使えばこんな面倒な記述をする必要はありません。
IJU: TestCase implements a non-static suite method
TestCase クラスで suite メソッドを定義していますが、
staticとして定義されていません。
IJU: TestCase implements tearDown but doesn't call super.tearDown()
TestCaseクラス で tearDown メソッドを実装していますが、
その内部で super.tearDown() を呼び出していません。
IL: A container is added to itself
コンテナクラスが、自分に自分自身を追加しています。
これはハッシュコードの計算時に StackOverflowException を引き起こす可能性があります。
IL: An apparent infinite loop
無限ループと思われるロジックがあります。
IL: An apparent infinite recursive loop
メソッド内で、無条件に同メソッドを呼び出しています。
これは無限ループを引き起こすでしょう。
IM: Integer multiply of result of integer remainder
* と % 演算子が同時に使われています。
これは優先順位が紛らわしいので括弧を付けた方が良いでしょう。
int v = i % 60 * 1000; // これは (i % 60) * 1000 と同じ
int v = (i % 60) * 1000; // 明示的に括弧を付けた方がわかりやすい
INT: Bad comparison of nonnegative value with negative constant
非負数(変数)と負数(定数)を比較しています。
これは比べるまでもなく結果がわかっているはずです。
INT: Bad comparison of signed byte
byte値の比較方法が間違っている可能性があります。
func(byte b) {
if (b == 0xbf) {
// byteは -128 ～ 127 の範囲しか取らないので、上の式は常に false を返す
}
if ((b & 0xff) == 0xbf) {
// b を符号無し(signed)byteとして扱う場合、このようにすればうまくいく
}
}
INT: Integer remainder modulo 1
exp & 1 というコードがあります。
これは常に0を返します。おそらく記述が間違っています。
IP: A parameter is dead upon entry to a method but overwritten
メソッドの引数が、それを参照することなくメソッド内部で上書きされています。
つまり、引数で与えられた値を無視していることになります。
これはおそらく、意図した動作では無いはずです。
void func(String str) {
str = "abc"; // ここで上書きしている
...
System.out.println(str);
}
JCIP: Fields of immutable classes should be final
net.jcip.annotations.Immutable 注釈の付いたクラスのフィールドが
final で定義されていません。
MF: Class defines field that masks a superclass field
親クラスと同名のフィールドを定義しています。
これは紛らわしいので止めた方が良いでしょう。
MF: Method defines a variable that obscures a field
メソッド内で、フィールドと同名のローカル変数を定義しています。
NP: Null pointer dereference
nullポインタが参照されています。
このコードを実行するとNullPointerExceptionが発生するでしょう。
public void func() {
String str = null;
...
if (str.length() > 0) {
// (str == null)なので例外発生
}
}
NP: Null pointer dereference in method on exception path
例外が発生した場合にnullポインタが参照されています。
このコードを実行するとNullPointerExceptionが発生するでしょう。
public void func(boolean b) {
String str = null;
try {
if (b) {
throw new Exception();
}
} catch (Exception e) {
if (str.length() > 0) {
// 上のブロックで例外が発生した場合、(str == null)なので例外発生
}
}
}
NP: Method does not check for null argument
メソッド内で、パラメータのnullチェックをしていません。
NP: Null value is guaranteed to be dereferenced
詳細は不明です。
NP: Null value is guaranteed to be dereferenced on exception path
詳細は不明です。
NP: Method call passes null to a parameter declared @NonNull
@NonNull として定義されたメソッドパラメータに null値を渡して呼び出しています。
ちなみにこのアノテーションはJava標準ライブラリではありません。
が、非常に有用なアノテーションだと思います。
Findbugs配布パッケージの annotations.jar にこれらのアノテーション群が
格納されています。
NP: Method may return null, but is declared @NonNull
@NonNull として定義されているのに、メソッドが null を返しています。
NP: A known null value is checked to see if it is an instance of a type
明らかなnull値に対してinstanceof演算子を使用しています。
これは常に false を返しますが、おそらく意図した動作では無いはずです。
NP: Possible null pointer dereference
nullポインタが参照されている可能性があります。
このコードを実行するとNullPointerExceptionが発生する可能性があります。
public void func(boolean b) {
String str = null;
if (b) {
str = "";
}
if (str.length() > 0) {
// (str == null)の可能性がある
}
}
NP: Possible null pointer dereference in method on exception path
例外が発生した場合にnullポインタが参照されている可能性があります。
このコードを実行すると NullPointerException が発生する可能性があります。
public void func(boolean a, boolean b) {
String str = null;
try {
if (b) {
throw new Exception();
}
str = "";
} catch (Exception e) {
if (a) {
str = "";
}
}
if (str.length() > 0) {
// 上のブロックで例外が発生した場合、(str == null)の可能性がある
}
}
NP: Method call passes null for unconditionally dereferenced parameter
メソッドパラメータに null を渡していますが、
メソッド内ではこの値をnullチェック無しで使用しています。
この場合、 NullPointerException が発生してしまいます。
NP: Non-virtual method call passes null for unconditionally dereferenced parameter
詳細は不明です。
NP: Store of null value into field annotated NonNull
@NonNull で宣言されたフィールドにnull値を代入しています。
NP: Read of unwritten field
何も書き込まれていないフィールドを読み込んでいます。
この場合、NullPointerException が発生する可能性があります。
NS: Questionable use of non-short-circuit logic
疑わしいnon-short-circuitロジック(& や |)が使われています。
short-circuitロジック(&& や ||)と違い、このロジックは常に両辺を評価します。
言葉で説明しても判りにくいと思うので例を挙げます。
public void func(String str) {
// short-circuitロジックの例
if (str != null && str.length() > 0) {
// この場合、(str != null) が成立しなければ右辺の評価は行われません。
System.out.println(str);
}
// non-short-circuitロジックの例
if (str != null & str.length() > 0) {
// この場合、(str != null) が成立しなくても右辺の評価を行います。
// よって、str == null の場合に右辺を評価して例外が発生してしまいます。
System.out.println(str);
}
}
一般的に、boolean値同士の比較ではnon-short-circuitロジックを
使わない方が良いでしょう。
Nm: Class defines equal(); should it be equals()?
equalメソッドが定義されています。
equalsメソッドの間違いではありませんか？
別言語からの移行者にはありがちな間違いと言えそうです。
public class ClassA() {
public boolean equal(Object obj) {
// equalsの間違い？
}
}
Nm: Class defines hashcode(); should it be hashCode()?
hashcodeメソッドが定義されています。
hashCodeメソッドの間違いではありませんか？
public class ClassA() {
public int hashcode() {
// hashCodeの間違い？
}
}
Nm: Class defines tostring(); should it be toString()?
tostringメソッドが定義されています。
toStringメソッドの間違いではありませんか？
public class ClassA() {
public String tostring() {
// toStringの間違い？
}
}
Nm: Apparent method/constructor confusion
クラス名と同名のメソッドが定義されています。
これはコンストラクタと混乱する可能性があるので避けましょう。
Nm: Very confusing method names
親子関係にあるクラス内で、
大小文字の違いだけによる複数のメソッドが定義されています。
これは非常に紛らわしいので止めましょう。
public class ClassA {
public void setValue() {
}
}
public class ClassB extends ClassA {
public void setvalue() {
}
// ClassA.setValue と ClassB.setvalue は大小文字の違いだけ。
// 非常に紛らわしいので止めた方が良い
}
QBA: Method assigns boolean literal in boolean expression
if / while 文の条件ブロック内に a = false のような記述があります。
多くの場合、これは a == false の間違いです。
※ より正確には !a が正解です
RC: Suspicious reference comparison
疑わしい比較をしています。
例えば、Integer型の値同士を == 演算子で比較しています。
これは equals() メソッドを使うのが正しい比較方法です。
RCN: Nullcheck of value previously dereferenced
冗長なnullチェックがあります。
以前にnullチェックしたインスタンスに対して、再びnullチェックをしています。
public void func(Object obj) {
if (obj != null) {
...
if (obj != null) {
// objに対してnullチェックを行っているが、このインスタンスは
// 以前にnullチェックをして以降値が変更されていない。
// よって、ここでのnullチェックは不要である。
}
}
}
RE: Invalid syntax for regular expression
不正な正規表現パターンが使用されています。
RE: File.separator used for regular expression
正規表現パターンに File.separator を使用しています。
この文字は Windows プラットフォームの場合 \ になります。
これは正規表現内ではエスケープ文字と見なされるので、おそらく正常に動作しないでしょう。
RE: "." used for regular expression
正規表現パターンとして、"." が使用されています。
これは本当に意図したパターンでしょうか？
String str;
str.replaceAll(".", "A"); // str文字列の全ての文字が「A」に変換される
RV: Random value from 0 to 1 is coerced to the integer 0
0～1の値を持つランダム値をInteger型に変換していますが、これは常に0になります。
RV: Bad attempt to compute absolute value of signed 32-bit hashcode
32bitの符号付きハッシュコード(int型)の絶対値を
求める方法が間違っています。
Integer.MIN_VALUE は、intで表現できる最小の数ですが
Math.abs(Integer.MIN_VALUE) は Integer.MIN_VALUE を返してしまいます。
MIN_VALUE(-2147483648)の絶対値を取るとその値は 2147483648 になりますが
これはintで表現できる範囲を超えてしまっている為
結果的に -2147483648 で返すしか無い為です。
RV: Bad attempt to compute absolute value of signed 32-bit random integer
32bitの符号付きランダム整数値(int型)の絶対値を
求める方法が間違っています。
RV: Method discards result of readLine after checking if it is nonnull
readLine メソッドの戻り値を non-null だと確認した後で
その値を捨てています。
RV: Method ignores return value
メソッドの戻り値を無視しています。
public void func(String str) {
str.substring(0,10);
// 上の文では、substringメソッドの戻り値を無視している。
// str = str.substring(0,10); のようにする必要がある。
System.out.println(str);
}
SA: Double assignment of field
フィールドに対する2重の代入文があります。
x = x = 17; // Oh!
SA: Self assignment of field
フィールドに対する無意味な自己代入文があります。
int x;
public void foo() {
x = x;
}
SA: Self comparison of field with itself
同じフィールド同士を比較しています。
おそらく記述ミスでしょう。
private int value;
public void func() {
if (value == value) {
// おそらく記述ミス
}
}
SA: Nonsensical self computation involving a field (e.g., x & x)
同じフィールド同士で演算しています。
おそらく記述ミスでしょう。
private int value;
public void func() {
if (value - value == 0) {
// おそらく記述ミス
}
}
SA: Double assignment of local variable
ローカル変数に対する2重の代入文があります。
x = x = 17; // Oh!
SA: Self comparison of value with itself
同じローカル変数もしくはメソッドパラメータ同士を比較しています。
おそらく記述ミスでしょう。
SA: Nonsensical self computation involving a variable (e.g., x & x)
同じローカル変数もしくはメソッドパラメータ同士で演算しています。
おそらく記述ミスでしょう。
SF: Dead store due to switch statement fall through
case文で代入された変数が、fall through されて次のcase文で上書きされています。
これはおそらく、break文やreturn文を書き忘れているのでしょう。
SIO: Unnecessary type check done using instanceof operator
instanceof 演算子による不必要な型チェックが行われています。
SQL: Method attempts to access a prepared statement parameter with index 0
PreparedStament に対して、インデックス0でアクセスしようとしています。
インデックスは1以上の値しか取らないので、おそらくその処理は間違っています。
SQL: Method attempts to access a result set field with index 0
ResultSet に対して、インデックス0でアクセスしようとしています。
STI: Unneeded use of currentThread() call, to call interrupted()
interrupted() メソッドの呼び出し時に、不必要な currentThread() メソッドが呼び出されています。
Thread.currentThread().interrupted(); // Thread.interrupted() で良い
STI: Static Thread.interrupted() method is mistakenly attempted to be called on an arbitrary Thread object
Thread.interrupted() メソッドが、間違った場面で使われています。
UCF: Useless control flow in method
利用されない制御の流れがあります。
大抵の場合、if文の中身が空です。
if (argv.length == 1);
System.out.println("Hello, " + argv[0]);
例えばこのような場合、if文の文末に誤った ; がある為
if文の真偽に関わらず2行目が実行されてしまいます。
UMAC: Uncallable method defined in anonymous class
無名クラス内に、実行できないメソッドが定義されています。
オーバーライドと関係があるようですが…詳細は不明です。
UR: Uninitialized read of field in constructor
コンストラクタ内で、初期化前のフィールドを利用しようとしています。
String s;
public ClassA(String paramS) {
func(s); // このときsはまだ初期化されていない!!
this.s = paramS;
}
UwF: Field only ever set to null
あるフィールドに、常にnullしかセットされません。
これは意味のないフィールドであるはずです。
UwF: Unwritten field
全く書き込まれないフィールドがあります。
finalやstaticにすることを検討して下さい。
VA: Primitive array passed to function expecting a variable number of object
JDK5から使用可能な可変長引数に、配列を渡しています。
void func(String... args) {
for (Object arg : args) {
System.out.println(arg);
}
}
void enter() {
func(new String[] { "A", "B", "C"} );
}
上のような場合、funcメソッドは args を長さ1で受け取ります。
つまり、enter() を実行した結果は1行で表示されます。
正しくは以下のようにします。
void func(String... args) {
for (Object arg : args) {
System.out.println(arg);
}
}
void enter() {
func("A", "B", "C"});
}
このようにすれば、enter() の実行結果は3行で表示されます。
Internationalization
このカテゴリのバグパターンは、国際化に関連するものを指します。
Dm: Method invokes dubious String.toUpperCase() or String.toLowerCase; use the Locale parameterized version instead
引数無しの String.toUpperCase() または toLowercase を呼び出しています。
代わりに、ロケール引数を取るメソッドを呼び出しましょう。
Malicious code vulnerability
このカテゴリのバグパターンは、悪意あるコードによって
セキュリティ上の危険があるものを指します。
EI: Method may expose internal representation by returning reference to mutable object
フィールドの配列をそのまま戻り値として返しています。
MS: Public static method may expose internal representation by returning array
の欄を参考にして下さい。
EI2: Method may expose internal representation by incorporating reference to mutable object
メソッドが、引数として受け取った配列をそのままフィールドに格納しています。
この配列の内容を外部または内部で変更した場合、
呼び出し側もしくはこのクラス内ではその変更内容を認知できません。
例を挙げます。
public class ClassA {
static String[] TITLES = { "title1", "title2" };
public static void setTitles(String[] titles) {
TITLES = titles;
}
}
public class MaliceClass {
public void func() {
String[] badTitles = new String[] { "title1", "title2" };
ClassA.setTitles(badTitles);
...
badTitles[0] = "bad-title1";
// 上のコードを実行することで、ClassA.TITLES が変更されてしまいます。
// そしてこの事をClassA側では認識できません。
}
}
この問題を解決する方法の一つは、setTitlesメソッド内で
配列のコピーを生成することです。
public static void setTitles(String[] titles) {
TITLES = new String[titles.length];
for (int i = 0 ; i < titles.length ; i++) {
TITLES[i] = titles[i];
}
}
FI: Finalizer should be protected, not public
finalize() メソッドは public ではなく protected で宣言すべきです。
public class ClassA {
public void finalize() throws Throwable {
// protectedで宣言すべき。finalize()は外部から呼ぶ必要が無い
}
}
MS: May expose internal static state by storing a mutable object into a static field
EI2: Method may expose internal representation by incorporating reference to mutable object の static 版です。
MS: Field isn't final and can't be protected from malicious code
mutable(可変性)のfinalフィールドは、悪意のあるコードにより(もしくは偶発的に)
内容が変更される危険性があります。
MS: Public static method may expose internal representation by returning array
public static なメソッドで配列を戻り値として返すと、その内容を
外部で変更される危険性があります。
例を挙げます。
public class ClassA {
static final String[] TITLES = new String[] { "title1", "title2 };
public static String[] getTitles() {
return TITLES;
}
}
MS: Field should be package protected の場合と同様の問題が発生します。
getTitles() の戻り値を外部コードが変更することで、
ClassA.TITLES までもが変更されてしまいます。
これを回避する方法の一つが、配列のコピーを返す事です。
こうすれば、戻り値をいくら外部コードで変更されようとも
ClassA.TITLES は一切その影響を受けません。
MS: Field should be both final and package protected
このフィールドは final かつ パッケージprotected として定義すべきです。
そうしないと、悪意のある外部コードにより（もしくは偶発的に）内容が変更される危険性があります。
例を挙げます。
public class ClassA {
public static String[] TITLES = { "title1", "title2 };
}
この場合、外部コードが自由にTITLESの値を変更できてしまいます。
このフィールドはstaticなので、全てのオブジェクトに影響が出ることになります。
MS: Field is a mutable array
配列フィールドが外部から変更可能になっています。
悪意のある外部コードにより（もしくは偶発的に）内容が変更される危険性があります。
例を挙げます。
public class ClassA {
public static final String[] TITLES = new String[] { "title1", "title2 };
}
この場合、外部コードが自由にTITLESの値を変更できてしまいます。
finalが宣言してあるので一見変更不可能なように思えますが、実は可能なのです。
例えば
ClassA.TITLES = new String[] { "bad-title1", "bad-title2" };
のようなコードは実行できませんが(final宣言の為)、
ClassA.TITLES[0] = "bad-title";
のようなコードは実行できてしまいます。
MS: Field is a mutable Hashtable
Hashtableフィールドが外部から変更可能になっています。
MS: Field should be moved out of an interface and made package protected
フィールドはインターフェイスの外に出すか package protected として定義すべきです。
MS: Field should be package protected
フィールドは package protected として定義すべきです。
MS: Field isn't final but should be
フィールドは final ではありませんが、finalにすべきです。
例を挙げます。
public class ClassA {
public static String TITLE = "title";
}
この場合、外部コードが自由にTITLEの値を変更できてしまいます。
このフィールドはstaticなので、全てのオブジェクトに影響が出ることになります。
Multithreaded correctness
このカテゴリのバグパターンは、マルチスレッド時に問題のあるコードを指します。
DC: Possible double check of field
このロジックは「double-checked locking」である可能性があります。
詳しくは他のページに譲りますが、このロジックは正しく働かない危険性があります。
参考ページ ： The "Double-Checked Locking is Broken" Declaration
Dm: Monitor wait() called on Condition
java.util.concurrent.locks.Condition のインスタンスに対して
wait() メソッドが呼び出されています。
正しくは await() を使います。
Dm: A thread was created using the default empty run method
runメソッドの定義されていないスレッドが作成されています。
このスレッドは何もしません。ただ資源を消費するだけです。
ESync: Empty synchronized block
空の同期化ブロックが存在します。
IS: Inconsistent synchronization
同期化が考慮されていません。
IS: Field not guarded against conconcurrent access
net.jcip.annotations.GuardedBy 注釈の付いたフィールドが
並列的アクセスに対して考慮されていません。
JLM: Synchronization performed on java.util.concurrent Lock in method
java.util.concurrent.locks.Lock のインスタンスに対して
同期化を使用しています。代わりに lock() / unlock() メソッドを使いましょう。
LI: Incorrect lazy initialization of static field
staticフィールドの不正な遅延初期化が行われています。
ML: Method synchronizes on an updated field
更新されるフィールドを使って同期化されています。
MWN: Mismatched notify()
組合せが間違ったnotify()の呼び出しがあります。
MWN: Mismatched wait()
組合せが間違ったwait()の呼び出しがあります。
NN: Naked notify in method
メソッド中に裸の notify があります。
NP: Synchronize and null check on the same field
同じフィールドに対してnullチェックおよびSynchronizeの引数として使っています。
通常、Synchronizeの引数は決してnullでない値を使用するべきです。
別のフィールドでSynchronizeした方が良いでしょう。
No: Using notify() rather than notifyAll() in method
notifyAll ではなく notify メソッドを使っています。
RS: Class's readObject() method is synchronized
readObject メソッドが同期化されています。
Javaの仕様により、このメソッドは常に単一のスレッドから呼び出されるので
同期化をする必要はありません。
public class ClassA implements Serializable {
synchronized private void readObject(java.io.ObjectInputStream in)
throws IOException, ClassNotFoundException {
// このメソッドをsynchronizedにする必要は無い
}
}
Ru: Invokes run on a thread (did you mean to start it instead?)
runメソッドを呼び出しています。startメソッドを呼び出すべきではないですか？
public void func(Thread thread) {
thread.run(); // start() の間違い？
}
runメソッドは通常Thread内のネイティブメソッドから呼び出されます。
直接コールするのは間違っています。
SC: Constructor invokes Thread.start()
コンストラクタ内でスレッドを開始しています。
将来このクラスが拡張(またはサブクラス化)されたとき、
サブクラスのコンストラクタが実行される前にスレッドが開始してしまいます。
これは良くない事です。
public ClassA(Thread thread) {
thread.start(); // コンストラクタ内で Thread.start() を実行している
}
SP: Method spins on field
フィールドをループの終了条件と使っていますが、
コンパイラはパフォーマンスの為に
そのフィールドの読み込みをループの外に出してしまう可能性があります。
この場合、無限ループに陥ってしまうでしょう。
STCAL: Call to static Calendar
カレンダーオブジェクトの使用はマルチスレッドに対して安全ではありません。
STCAL: Call to static DateFormat
DateFormatオブジェクトの使用はマルチスレッドに対して安全ではありません。
STCAL: Static Calendar
STCAL: Call to static Calendar と同様です。
STCAL: Static DateFormat
STCAL: Call to static DateFormat と同様です。
SWL: Method calls Thread.sleep() with a lock held
ロックを掴んだままの状態で Thread.sleep() メソッドを呼び出しています。
これはパフォーマンスを非常に悪くしたり、デッドロックを起こしたりする可能性があります。
代わりに wait() メソッドなどを使いましょう。
TLW: Wait with two locks held
詳細は不明です。
UG: Unsynchronized get method, synchronized set method
setterメソッドが同期化されているのに、getterメソッドが同期化されていません。
この場合、getterメソッドも同期化するべきです。
public class ClassA {
synchronized public void setCount(int count) {
this.count = count;
}
public int getCount() {
return count;
// setCountは同期化されているのに、getCountは同期化されていません。
// getCountも同期化しましょう。
}
}
UL: Method does not release lock on all paths
ロックが開放されていません。
通常、ロックは以下の構文で開放します。
Lock l;
l.lock();
try {
...
} finally {
l.unlock();
}
UL: Method does not release lock on all exception paths
例外が発生したときに、ロックが開放されません。
UW: Unconditional wait
条件ブロックに囲まれていない Object.wait() の呼び出しがあります。
この場合、そのスレッドは無限にウェイトが掛かってしまう可能性があります。
VO: A volatile reference to an array doesn't treat the array elements as volatile
配列に対して volatile 識別子を使用していますが、これは無効です。
代わりに、JDK5で導入された java.util.concurrent パッケージにある
アトミック配列変数を使用しましょう。
WS: Class's writeObject() method is synchronized but nothing else is
writeObjectメソッドが同期化されていますが
その他のメソッドは同期化されていません。
このことによる具体的な障害内容は不明です。
Wa: Condition.await() not in loop in method
Condition.await() メソッド呼び出しがループ内にありません。
Wa: Wait not in loop
waitメソッドがループの中にありません。
public void func() throws InterruptedException {
wait();
// waitメソッドは本来ループ内にあるはず
}
Performance
このカテゴリのバグパターンは、パフォーマンスに問題のあるコードを指します。
Bx: Primitive value is boxed and then immediately unboxed
プリミティブ値がBoxされ即座にUnboxされている箇所があります。
Bx: Primitive value is boxed then unboxed to perform primative coercion
プリミティブ値がBoxされ強制型変換によってUnboxされている箇所があります。
new Double(d).intValue(); // これは良くない
(int)d; // こちらの方が良い
Bx: Method allocates a boxed primitive just to call toString
プリミティブ値がBoxされ、toString メソッドがコールされています。
new Integer(1).toString(); // これは良くない
Integer.toString(1); // こちらの方が良い
Bx: Method invokes inefficient floating-point Number constructor; use static
浮動点小数を引数に持つコンストラクタを使用しています。
new Double(d); // これは良くない
Double.valueOf(d); // こちらの方が良い
Bx: Method invokes inefficient Number constructor; use static valueOf instead
Number系クラスのコンストラクタを呼び出しています。
代わりに valueOf メソッドを使用して下さい。
public void func() {
new Integer(10); // 代わりに Integer.valueOf(10) を使いましょう
}
valueOfは、-128～127 の場合には内部でキャッシュした値を返すので非常に高速です。
Dm: The equals and hashCode methods of URL are blocking
URLクラスのインスタンスに対する equals / hashCode メソッドの呼び出しは
内部的にDNS解決が発生するため、非常にコストの掛かる処理になります。
代わりに URI を使いましょう。
Dm: Maps and sets of URLs can be performance hogs
上の理由により、URLをMapのキーやSetに格納することは
パフォーマンスが非常に悪くなります。
代わりに URI を使いましょう。
Dm: Method invokes dubious Boolean constructor; use Boolean.valueOf(...) instead
Booleanクラスのコンストラクタを呼び出しています。
代わりに Boolean.valueOf メソッドを使用して下さい。
public void func() {
new Boolean(false); // 代わりに Boolean.valueOf() を使いましょう
}
Dm: Explicit garbage collection; extremely dubious except in benchmarking code
強制的に System.gc() メソッドを呼び出しています。
ベンチマークのコード以外でこのメソッドを使用するのは非常に疑わしい事です。
public void func() {
System.gc();
// 強制的な System.gc() の呼び出し
}
Dm: Method allocates an object, only to get the class object
getClass() メソッドを呼び出すためだけにオブジェクトをnewしています。
これは通常 class キーワードを使用します。
public void func() {
new TestClass().getClass(); // 代わりに TestClass.class を使いましょう
}
Dm: Use the nextInt method of Random rather than nextDouble to generate a random integer
int型の乱数を得るために、nextDouble() よりも nextInt() を使用するべきです。
public void func() {
int value = (int)(r.nextDouble() * n); // 代わりに r.nextInt(n) を使いましょう
}
Dm: Method invokes inefficient new String(String) constructor
new String(String str) コンストラクタが使用されています。
これは通常 str で十分です。
public void func() {
String newStr = new String(str); // 代わりに String newStr = str; を使いましょう
}
Dm: Method invokes inefficient String.equals(""); use String.length() == 0 instead
String.equals("") メソッドを呼び出しています。
代わりに String.length() == 0 を使用するべきです。
public void func(String str) {
if (str.equals("")) {
// 代わりに str.length() == 0 を使いましょう
}
}
Dm: Method invokes toString() method on a String
Stringクラスのインスタンスに対して toString() メソッドを呼び出しています。
public void func() {
String str = "...";
str.toString(); // 代わりに str を使いましょう
}
Dm: Method invokes inefficient new String() constructor
new String() という記述があります。
代わりに "" を使用するべきです。
public void func() {
String str = new String(); // 代わりに str = ""; を使いましょう
}
HSC: Huge string constants is duplicated across multiple class files
大きな文字列が複数のクラスに渡って重複定義されています。
詳しくはわかりませんが、JDKのバグのようです。
ちなみに、次期JDK6.0ではこのバグが修正されるとの事です。
ITA: Method uses toArray() with zero-length array argument
toArrayメソッドの引数に長さ0の配列を渡しています。
users.toArray(new User[0]); // これは次のようにするのが良い
users.toArray(new User[users.size()]);
SBSC: Method concatenates strings using + in a loop
ループ内でStringインスタンスに対して + 演算子を使用しています。
これは、大量のオブジェクトを生成する危険性があります。
代わりに StringBuffer(JDK5ならStringBuilder) を使用するべきです。
public void func() {
String s = "";
for (int i = 0 ; i < 100 ; i++) {
s = s + "a"; // ループ内での + 演算子の使用は、避けるべきです
}
}
SIC: Should be a static inner class
このインナークラスは static にするべきです。
public class ClassA {
class InnerClass {
// このインナークラスはstaticにすることが可能なので、するべきです。
}
}
SIC: Could be refactored into a named static inner class
この匿名インナークラスは、名前付きのstaticクラスにするべきです。
public class OuterClass {
private int someValue;
public void func() {
MouseListener listener = new MouseAdapter() {
// someValue を使用していないのでstaticクラス化できる
};
}
}
匿名クラスは外部クラス(例ではOuterClass)への参照情報を保持しています。
これは単純に、インスタンスサイズを増加させます。
さらに、参照情報が増えるという事は
ガーベッジコレクションのタイミングが遅くなる可能性があるという事です。
ですから、もし匿名クラス内で外部クラスを参照していないのならば
このクラスは匿名にせずに名前付きstaticクラスにするべきなのです。
Eclipseならば、Refactor->Convert Anonymous Class to Nested... コマンドにより
簡単にこの変更ができます。
SIC: Could be refactored into a static inner class
このインナークラスはstaticクラスにするべきです。
public class Outer {
private int someValue;
class Inner {
public Inner() {
// コンストラクタからのみ、Outerクラスの情報を参照している
System.out.println(someValue);
}
...
}
}
こういったクラスを考えます。
先程の例とは違い、今度はインナークラス内から外部クラスの情報を参照しています。
ですので、そのままではこれをstatic化は出来ません。
しかし次のように変更可能です。
public class Outer {
private int someValue;
static class Inner {
public Inner(int someValue) {
System.out.println(someValue);
}
...
}
}
someValueをコンストラクタの引数に渡すことにより、
InnerクラスはもはやOuterクラスの参照を持つ必要が無くなりました。
これによって、Innerクラスをstatic化できます。
SS: Unread field: should this field be static?
コンパイル時に値が決定されるようなfinalフィールドがあります。
このフィールドはstaticであるべきではありませんか？
public class ClassA {
private final String TITLE = "title";
// このフィールドは常に"title"を示す。よって、staticにするべきである。
}
UM: Method calls static Math class method on a constant value
Mathクラスのメソッドへ渡す引数に、特定の定数を使用しています。
こういった場合、Mathメソッドを使う必要がありません。
double d = Math.acos(1); // d = 0.0 と同じ
UPM: Private method is never called
このprivateメソッドはどこからも呼ばれていません。
public class ClassA {
private void func() {
// このメソッドはどこからも呼ばれていないので、削除もしくはコメントアウトすべきです。
}
}
UrF: Unread field
全く読み込まれないフィールドがあります。
必要無ければ削除、もしくはコメントアウトしておきましょう。
UuF: Unused field
全く使用されないフィールドがあります。
必要無ければ削除、もしくはコメントアウトしておきましょう。
WMI: Inefficient use of keySet iterator instead of entrySet iterator
Map の各エントリーにアクセスするのに、keySet() メソッドを使用しています。
これは entrySet() メソッドに置き換えられるべきです。
public void func1(Map map) {
for (Iterator it = map.keySet() ; it.hasNext() ; ) {
Object key = it.next();
Object value = map.get(key);
...
}
}
上のような処理は、
public void func1(Map map) {
for (Iterator it = map.entrySet() ; it.hasNext() ; ) {
Map.Entry entry = (Map.Entry)it.next();
Object key = entry.getKey();
Object value = entry.getValue();
...
}
}
で置き換えましょう。
これは一見すると(行数も増えているし)良くない処理のように思えますが
ループ中に map.get(key) を呼び出すことはパフォーマンスの点で非常に良くない事なのです。
Dodgy
このカテゴリのバグパターンは無駄なコード、危険性のあるコードを指します。
BC: Questionable cast to abstract collection
Collection を AbstractList などのAbstractクラスにキャストしています。
もしコレクションからイテレータを取り出したいだけだったら、
このようなキャストは必要ありません。
BC: Questionable cast to concrete collection
Collection を ArrayList などのクラスにキャストしています。
これは大抵の場合、良くありません。
せめて、AbstractList のような抽象クラスにキャストしましょう。
BC: Unchecked/unconfirmed cast
無条件でキャストが行われています。
このキャストは失敗する可能性があります。
BC: instanceof will always return true
常にtrueを返すinstanceof演算子の使用箇所があります。
CI: Class is final but declares protected field
finalクラスにprotectedフィールドが宣言されています。
これはprivateに変更すべきです。
DB: Method uses the same code for two branches
if/else構文内で、2つ以上のブロックに同じコードが使用されています。
DB: Method uses the same code for two switch clauses
switchのcase節内で、2つ以上のブロックに同じコードが使用されています。
DLS: Dead store to local variable
読み込まれないローカル変数への書き込みが行われています。
DLS: Dead store of null to local variable
ローカル変数にnullが代入されていますが、その後読み込まれていません。
これはガーベッジコレクタにとっては有効な場合もありますが
ほとんどの場合は良くないコーディング手法です。
さらに、Java SE 6.0においてこの処理はもはや不要となるようです。
DMI: Code contains a hard coded reference to an absolute pathname
ハードコードされた絶対パスが記述されています。
DMI: Non serializable object written to ObjectOutput
シリアライズ不可能なオブジェクトを
ObjectOutputに書き出そうとしています。
DMI: Invocation of substring(0), which returns the original value
substring(0) という呼び出しをしています。
これは元の値をそのまま返します。つまり意味がありません。
FE: Test for floating point equality
浮動小数点同士を == 演算子で比較しています。
この型の数値は結果が丸められるので、思った通りの結果が得られない可能性があります。
同値比較をしたい場合には、BigDecimal などの固定小数点クラスを使用して下さい。
IA: Ambiguous invocation of either an inherited or outer method
詳細は不明です。
IC: Initialization circularity
参照し合う2つのクラスのstatic-initializerから
循環が見つかりました。
これらの循環によって様々な予期せぬ動作が起こる可能性があります。
ICAST: int division result cast to double or float
int型数値の除算結果を double や float にキャストしています。
int x = 2;
int y = 5;
// Wrong: yields result 0.0
double value1 = x / y;
// Right: yields result 0.4
double value2 = x / (double) y;
このように、除算時にキャストすることで正しい結果が得られます。
ICAST: Result of integer multiplication cast to long
int型数値の乗算結果をlongにキャストしています。
これは、値がオーバーフローしている可能性が高いです。
long seconds = 1000*3600*24*days;
// ↑これはint型の演算なのでlongにキャストする前に値がオーバーフローする
long seconds = 1000L*3600*24*days;
// ↑このようにして、演算前にlongにキャストしておけばOK
ICAST: Unsigned right shift cast to short/byte
符号無し右シフト >>> の演算結果を short/byte にキャストしています。
IM: Computation of average could overflow
平均値を求める演算内でオーバーフローが発生する可能性があります。
avg = (low + high) / 2;
これはごく一般的な平均値の求め方ですが、意外な落とし穴があります。
例えば、
(Integer.MAX_VALUE + 2) / 2
上の式は、(Integer.MAX_VALUE / 2 + 1) を返すように見えますが
実は違います。Integer.MAX_VALUE + 2 の演算でオーバーフローが発生し
この値は負の値になってしまいます。それを2で割っても、当然結果は負の値です。
これを回避する一つの方法は次のようになります。
avg = (low + high) >>> 2;
>>> とは見慣れない演算子かもしれませんが、これは「符号無しシフト」を行う演算子です。
これによって、常に演算結果は正の値を返すようになります。
ただしこの方法では、avgが負の値を返すような場合に対処できません。
このバグパターンは、JDKのライブラリ内にも存在します。
これにより、例えば要素数が2^15を超える配列をソートするような場合に
正しく動作しない可能性があります。
IM: Check for oddness that won't work for negative numbers
負の数値に対して正しい値を返さないチェックがされています。
if (x % 2 == 1) { // 奇数チェック
// このチェックは正数に対しては正常に動作するが、負数に対しては動作しない
// ((-1) % 2) は -1 になります
}
if (x % 2 != 0) {
// これならば負数に対しても正しい動作をします
}
INT: Vacuous comparison of integer value
常に同じ結果を返すようなint値の比較が行われています。
int i;
if (i <= Integer.MAX_VALUE) {
// こんな事する人、いる？
}
MTIA: Class extends Servlet class and uses instance variables
Servletの派生クラス内で、インスタンス変数（フィールド）を使用しています。
サーブレットクラスはマルチスレッドとして動くので、フィールドの使用は避けるべきです。
MTIA: Class extends Struts Action class and uses instance variables
StrutsのAction派生クラス内で、インスタンス変数（フィールド）を使用しています。
Actionクラスはマルチスレッドとして動くので、フィールドの使用は避けるべきです。
NP: Immediate dereference of the result of readLine()
readLine() メソッド呼び出しの戻り値を直接参照しています。
これは null を返す可能性があり、このときに NullPointerException が発生してしまいます。
NP: Load of known null value
以前nullチェックを行いnullと判定された変数を参照しようとしています。
これは明らかに間違いです。
おそらく別の変数を参照しようとして間違えたか、null判定が間違っているか、
どちらかでしょう。
if (value == null) {
// おそらく value != null の間違い
value.func();
}
NP: Possible null pointer dereference due to return value of called method
メソッドの戻り値を参照していますが、この値は null である可能性があります。
これを防ぐには、呼び出し側にnullチェックを入れるか
メソッドの戻り値に @NonNull アノテーションを付けるなどして null でないことを保証しましょう。
PZLA: Consider returning a zero length array rather than null
配列を返すメソッドでは、nullよりもサイズ0の配列を返すことを検討すべきです。
nullを返すと、呼び出し側で余計なnullチェックロジックが必要になります。
public Object[] getObjects() {
if (...) {
return null;
// 上の行は、return new Object[0]; とする方が良い。
}
...
}
QF: Complicated，subtle or wrong increment in for-loop
forループ内で、間違った変数をインクリメントさせている可能性があります。
for (int i = 0; i < 10; i++) {
for (int k = 0; k < 10; i++) { // k++の間違い
...
}
}
RCN: Redundant comparison of non-null value to null
冗長な比較が行われています。
nullでないと判明した値とnullであると判明した値が比較されています。
RCN: Redundant comparison of two null values
冗長な比較が行われています。
nullであると判明した2値が比較されています。
RCN: Redundant nullcheck of value known to be non-null
冗長な比較が行われています。
nullでないと判明した値とnull値が比較されています。
RCN: Redundant nullcheck of value known to be null
冗長な比較が行われています。
nullであると判明した値とnull値が比較されています。
REC: java.lang.Exception is caught when Exception is not thrown
Exceptionが発生しない箇所でExceptionをキャッチしています。
これは、RuntimeException をもキャッチしてしまうので潜在的なバグを見逃してしまう可能性があります。
try {
} catch (Exception e) {
// 全ての例外をまとめて Exception で受けるのは良くない。
// 面倒でも例外ごとにcatchするか、RuntimeException を別にcatchするようにする。
}
RI: Class implements same interface as superclass
あるクラスの定義文で、スーパークラスで定義されているインターフェイスと同じインターフェイスが
定義されています。
これは冗長な記述になります。
class ClassA implements Serializable {
...
}
class ClassB extends ClassA implements Serializable {
// 上の定義にある implements Serializable は必要無い
...
}
RV: Method checks to see if result of String.indexOf is positive
String.indexOf メソッドの戻り値に対して「0より大きい値」というチェックをしています。
正しくは「0以上の値」というチェックをする必要があります。
if (str.indexOf("...") > 0) {
// 上のチェック方法は間違っている。正しくは str.indexOf("...") >= 0
}
RV: Remainder of hashCode could be negative
ハッシュコードの剰余が負数になる可能性があります。
詳細は不明です。
RV: Remainder of 32-bit signed random integer
32ビットの符号付き整数をランダム生成するロジックがありますが
代わりに Random.nextInt(n) を使うことを強く推奨します。
SA: Self assignment of local variable
ローカル変数に対する無意味な自己代入文があります。
public void foo() {
int x = 3;
x = x;
}
SF: Switch statement found where one case falls thru to the next case
swtich句内で、次のcase文に続いているcase文があります。
switch (n) {
case 1:
...
// 通常、ここには break か return が入るはず
case 2:
...
}
ST: Write to static field from instance method
インスタンスメソッドから、staticフィールドへの書き込みを行っています。
public Some {
private static String staticValue;
public void instanceMethod() {
staticValue = "abc"; // staticフィールドへの書き込みをしている
}
}
Se: Transient field of class that isn't seriablizable.
transient宣言されたフィールドがありますが、
そのクラスは seriablizable ではありません。
直列化不能のクラスで transient 宣言をしても意味がありません。
UCF: Useless control flow in method
利用されない制御の流れがあります。
大抵の場合、if文の中身が空です。
if (argv.length == 1);
System.out.println("Hello, " + argv[0]);
例えばこのような場合、if文の文末に誤った ; がある為
if文の真偽に関わらず2行目が実行されてしまいます。
UwF: Field not initialized in constructor
フィールドがコンストラクタの中で初期化されていません。
XFB: Method directly allocates a specific implementation of xml interfaces
XML関連インターフェイスの実装クラスを直接使用しています。
通常これにはFactoryクラスを使用します。
こうすることによって、実行時に動的に実装クラスを選択させることが可能になります。
