FindBugs rules - Bad practice

Bad practice - rules provided by FindBugs

Creates an empty jar file entry

The code calls putNextEntry(), immediately followed by a call to closeEntry().

This results in an empty JarFile entry.

The contents of the entry should be written to the JarFile between the calls to putNextEntry() and closeEntry().

Creates an empty zip file entry

The code calls putNextEntry(), immediately followed by a call to closeEntry().

This results in an empty ZipFile entry.

The contents of the entry should be written to the ZipFile between the calls to putNextEntry() and closeEntry().

Equals method should not assume anything about the type of its argument

The equals(Object o) method shouldn't make any assumptions about the type of o.

It should simply return false if o is not the same type as this.

Check for sign of bitwise operation

This method compares an expression such as  ((event.detail & SWT.SELECTED) > 0)

Using bit arithmetic and then comparing with the greater than operator can lead to unexpected results (of course depending on the value ofSWT.SELECTED).

If SWT.SELECTED is a negative number, this is a candidate for a bug.

Even when SWT.SELECTED is not negative, it seems good practice to use '!= 0' instead of '> 0'.

Class implements Cloneable but does not define or use clone method

clone method does not call super.clone()

This non-final class defines a clone() method that does not call super.clone().

If this class ("A") is extended by a subclass ("B"), and the subclass B calls super.clone(), then it is likely that B's clone() method will return an object of type A, which violates the standard contract for clone().

If all clone() methods call super.clone(), then they are guaranteed to use Object.clone(), which always returns an object of the correct type.

Class defines clone() but doesn't implement Cloneable

This class defines a clone() method but the class doesn't implement Cloneable.

There are some situations in which this is OK (e.g., you want to control how subclasses can clone themselves), but just make sure that this is what you intended.

Abstract class defines covariant compareTo() method

This class defines a covariant version of compareTo().  

To correctly override the compareTo() method in the Comparable interface, the parameter of compareTo() must have type java.lang.Object.

Covariant compareTo() method defined

This class defines a covariant version of compareTo().

To correctly override the compareTo() method in the Comparable interface, the parameter of compareTo() must have type java.lang.Object.

Method might drop exception

This method might drop an exception.

In general, exceptions should be handled or reported in some way, or they should be thrown out of the method.

Method might ignore exception

This method might ignore an exception.

In general, exceptions should be handled or reported in some way, or they should be thrown out of the method.

Adding elements of an entry set may fail due to reuse of Entry objects

The entrySet() method is allowed to return a view of the underlying Map in which a single Entry object is reused and returned during the iteration.

As of Java 1.6, both IdentityHashMap and EnumMap did so.

When iterating through such a Map, the Entry value is only valid until you advance to the next iteration.

If, for example, you try to pass such an entrySet to an addAll method, things will go badly wrong.

Random object created and used only once

This code creates a java.util.Random object, uses it to generate one random number, and then discards the Random object.

This produces mediocre quality random numbers and is inefficient.

If possible, rewrite the code so that the Random object is created once and saved, and each time a new random number is required invoke a method on the existing Random object to obtain it.

If it is important that the generated Random numbers not be guessable, you must not create a new Random for each random number; the values are too easily guessable.

You should strongly consider using a java.security.SecureRandom instead (and avoid allocating a new SecureRandom for each random number needed).

Don't use removeAll to clear a collection

If you want to remove all elements from a collection c, use c.clear, not c.removeAll(c).

Calling c.removeAll(c) to clear a collection is less clear, susceptible to errors from typos, less efficient and for some collections, might throw a ConcurrentModificationException.

Method invokes System.exit(...)

Invoking System.exit shuts down the entire Java virtual machine.

This should only been done when it is appropriate.

Such calls make it hard or impossible for your code to be invoked by other code.

Consider throwing a RuntimeException instead.

Method invokes dangerous method runFinalizersOnExit

Never call System.runFinalizersOnExit or Runtime.runFinalizersOnExit for any reason: they are among the most dangerous methods in the Java libraries.

Comparison of String parameter using == or !=

This code compares a java.lang.String parameter for reference equality using the == or != operators.

Requiring callers to pass only String constants or interned strings to a method is unnecessarily fragile, and rarely leads to measurable performance gains.

Consider using the equals(Object) method instead.

Comparison of String objects using == or !=

This code compares java.lang.String objects for reference equality using the == or != operators.

Unless both strings are either constants in a source file, or have been interned using the String.intern() method, the same string value may be represented by two different String objects.

Consider using the equals(Object) method instead.

Abstract class defines covariant equals() method

This class defines a covariant version of equals().

To correctly override the equals() method in java.lang.Object, the parameter of equals() must have type java.lang.Object.

Equals checks for incompatible operand

This equals method is checking to see if the argument is some incompatible type (i.e., a class that is neither a supertype nor subtype of the class that defines the equals method).

For example, the Foo class might have an equals method that looks like:

public boolean equals(Object o) {

  if (o instanceof Foo)

    return name.equals(((Foo)o).name);

  else if (o instanceof String)

    return name.equals(o);

  else return false;

}

This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive.

Without those properties, very unexpected behavoirs are possible.

Class defines compareTo(...) and uses Object.equals()

This class defines a compareTo(...) method but inherits its equals() method from java.lang.Object.

Generally, the value of compareTo should return zero if and only if equals returns true.

If this is violated, weird and unpredictable failures will occur in classes such as PriorityQueue.

In Java 5 the PriorityQueue.remove method uses the compareTo method, while in Java 6 it uses the equals method.

From the JavaDoc for the compareTo method in the Comparable interface:

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y))

Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact.

The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

equals method fails for subtypes

This class has an equals method that will be broken if it is inherited by subclasses.

It compares a class literal with the class of the argument (e.g., in class Foo it might check if Foo.class == o.getClass())

It is better to check if this.getClass() == o.getClass()

Covariant equals() method defined

This class defines a covariant version of equals().

To correctly override the equals() method in java.lang.Object, the parameter of equals() must have type java.lang.Object.

Empty finalizer should be deleted

Empty finalize() methods are useless, so they should be deleted.

Explicit invocation of finalizer

This method contains an explicit invocation of the finalize() method on an object.

Because finalizer methods are supposed to be executed once, and only by the VM, this is a bad idea.

If a connected set of objects beings finalizable, then the VM will invoke the finalize method on all the finalizable object, possibly at the same time in different threads.

Thus, it is a particularly bad idea, in the finalize method for a class X, invoke finalize on objects referenced by X, because they may already be getting finalized in a separate thread.

Finalizer nulls fields

This finalizer nulls out fields.

This is usually an error, as it does not aid garbage collection, and the object is going to be garbage collected anyway.

Finalizer only nulls fields

This finalizer does nothing except null out fields.

This is completely pointless, and requires that the object be garbage collected, finalized, and then garbage collected again.

You should just remove the finalize method.

Finalizer does not call superclass finalizer

This finalize() method does not make a call to its superclass's finalize() method.

So, any finalizer actions defined for the superclass will not be performed.

Add a call to super.finalize()

Finalizer nullifies superclass finalizer

This empty finalize() method explicitly negates the effect of any finalizer defined by its superclass.

Any finalizer actions defined for the superclass will not be performed.

Unless this is intended, delete this method.

Finalizer does nothing but call superclass finalizer

The only thing this finalize() method does is call the superclass's finalize() method, making it redundant.  Delete it.

Format string should use %n rather than \n

This format string include a newline character (\n).

In format strings, it is generally preferable better to use %n, which will produce the platform-specific line separator.

Unchecked type in generic call

This call to a generic collection method passes an argument while compile type Object where a specific type from the generic type parameters is expected.

Thus, neither the standard Java type system nor static analysis can provide useful information on whether the object being passed as a parameter is of an appropriate type.

Class defines equals() but not hashCode()

This class overrides equals(Object), but does not override hashCode().

Therefore, the class may violate the invariant that equal objects must have equal hashcodes.

Class defines equals() and uses Object.hashCode()

This class overrides equals(Object), but does not override hashCode(), and inherits the implementation of hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM).

Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes.

If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is : 

public int hashCode() {

  assert false : "hashCode not designed";

  return 42; // any arbitrary constant will do

}

Class defines hashCode() but not equals()

This class defines a hashCode() method but not an equals() method.

Therefore, the class may violate the invariant that equal objects must have equal hashcodes.

Class defines hashCode() and uses Object.equals()

This class defines a hashCode() method but inherits its equals() method from java.lang.Object (which defines equality by comparing object references).

Although this will probably satisfy the contract that equal objects must have equal hashcodes, it is probably not what was intended by overriding the hashCode() method.

(Overriding hashCode() implies that the object's identity is based on criteria more complicated than simple reference equality.)

If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is : 

public int hashCode() {

  assert false : "hashCode not designed";

  return 42; // any arbitrary constant will do

}

Class inherits equals() and uses Object.hashCode()

This class inherits equals(Object) from an abstract superclass, and hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM).

Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes.

If you don't want to define a hashCode method, and/or don't believe the object will ever be put into a HashMap/Hashtable, define the hashCode() method to throw UnsupportedOperationException.

Superclass uses subclass during initialization

During the initialization of a class, the class makes an active use of a subclass.

That subclass will not yet be initialized at the time of this use.

For example, in the following code, foo will be null.

public class CircularClassInitialization {

    static class InnerClassSingleton extends CircularClassInitialization {

        static InnerClassSingleton singleton = new InnerClassSingleton();

    }

    static CircularClassInitialization foo = InnerClassSingleton.singleton;

}

Dubious catching of IllegalMonitorStateException

IllegalMonitorStateException is generally only thrown in case of a design flaw in your code

(calling wait or notify on an object you do not hold a lock on).

Needless instantiation of class that only supplies static methods

This class allocates an object that is based on a class that only supplies static methods.

This object does not need to be created, just access the static methods directly using the class name as a qualifier.

Iterator next() method can't throw NoSuchElementException

This class implements the java.util.Iterator interface.

However, its next() method is not capable of throwing java.util.NoSuchElementException.

The next() method should be changed so it throws NoSuchElementException if is called when there are no more elements to return.

Store of non serializable object into HttpSession

This code seems to be storing a non-serializable object into an HttpSession.

If this session is passivated or migrated, an error will result.

Fields of immutable classes should be final

The class is annotated with net.jcip.annotations.Immutable, and the rules for that annotation require that all fields are final.

Method with Boolean return type returns explicit null

A method that returns either Boolean.TRUE, Boolean.FALSE or null is an accident waiting to happen.

This method can be invoked as though it returned a value of type boolean, and the compiler will insert automatic unboxing of the Boolean value.

If a null value is returned, this will result in a NullPointerException.

Clone method may return null

This clone method seems to return null in some circumstances, but clone is never allowed to return a null value.

If you are convinced this path is unreachable, throw an AssertionError instead.

equals() method does not check for null argument

This implementation of equals(Object) violates the contract defined by java.lang.Object.equals() because it does not check for null being passed as the argument.

All equals() methods should return false if passed a null value.

toString method may return null

This toString method seems to return null in some circumstances.

A liberal reading of the spec could be interpreted as allowing this, but it is probably a bad idea and could cause other code to break.

Return the empty string or some other appropriate string rather than null.

Class names should start with an upper case letter

Class names should be nouns, in mixed case with the first letter of each internal word capitalized.

Try to keep your class names simple and descriptive.

Use whole words-avoid acronyms and abbreviations (unless the abbreviation is much more widely used than the long form, such as URL or HTML).

Class is not derived from an Exception, even though it is named as such

This class is not derived from another exception, but ends with 'Exception'.

This will be confusing to users of this class.

Confusing method names

The referenced methods have names that differ only by capitalization.

Field names should start with a lower case letter

Names of fields that are not final should be in mixed case with a lowercase first letter and the first letters of subsequent words capitalized.

Use of identifier that is a keyword in later versions of Java

The identifier is a word that is reserved as a keyword in later versions of Java, and your code will need to be changed in order to compile it in later versions of Java.

Use of identifier that is a keyword in later versions of Java

This identifier is used as a keyword in later versions of Java.

This code, and any code that references this API, will need to be changed in order to compile it in later versions of Java.

Method names should start with a lower case letter

Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

Class names shouldn't shadow simple name of implemented interface

This class/interface has a simple name that is identical to that of an implemented/extended interface, except that the interface is in a different package (e.g., alpha.Foo extends beta.Foo).

This can be exceptionally confusing, create lots of situations in which you have to look at import statements to resolve references and creates many opportunities to accidently define methods that do not override methods in their superclasses.

Class names shouldn't shadow simple name of superclass

This class has a simple name that is identical to that of its superclass, except that its superclass is in a different package (e.g., alpha.Foo extends beta.Foo).

This can be exceptionally confusing, create lots of situations in which you have to look at import statements to resolve references and creates many opportunities to accidently define methods that do not override methods in their superclasses.

Very confusing method names (but perhaps intentional)

The referenced methods have names that differ only by capitalization.

This is very confusing because if the capitalization were identical then one of the methods would override the other.

From the existence of other methods, it seems that the existence of both of these methods is intentional, but is sure is confusing.

You should try hard to eliminate one of them, unless you are forced to have both due to frozen APIs.

Method doesn't override method in superclass due to wrong package for parameter

The method in the subclass doesn't override a similar method in a superclass because the type of a parameter doesn't exactly match the type of the corresponding parameter in the superclass.

For example, if you have : 

    import alpha.Foo;

    public class A {

      public int f(Foo x) { return 17; }

    }

    ----

    import beta.Foo;

    public class B extends A {

      public int f(Foo x) { return 42; }

      public int f(alpha.Foo x) { return 27; }

    }

The f(Foo) method defined in class B doesn't override the f(Foo) method defined in class A, because the argument types are Foo's from different packages.

In this case, the subclass does define a method with a signature identical to the method in the superclass, so this is presumably understood.

However, such methods are exceptionally confusing. You should strongly consider removing or deprecating the method with the similar but not identical signature.

Method may fail to close database resource

The method creates a database resource (such as a database connection or row set), does not assign it to any fields, pass it to other methods, or return it, and does not appear to close the object on all paths out of the method.

Failure to close database resources on all paths out of a method may result in poor performance, and could cause the application to have problems communicating with the database.

Method may fail to close database resource on exception

The method creates a database resource (such as a database connection or row set), does not assign it to any fields, pass it to other methods, or return it, and does not appear to close the object on all exception paths out of the method.

Failure to close database resources on all paths out of a method may result in poor performance, and could cause the application to have problems communicating with the database.

Method may fail to close stream

The method creates an IO stream object, does not assign it to any fields, pass it to other methods that might close it, or return it, and does not appear to close the stream on all paths out of the method.

This may result in a file descriptor leak.

It is generally a good idea to use a finally block to ensure that streams are closed.

Method may fail to close stream on exception

The method creates an IO stream object, does not assign it to any fields, pass it to other methods, or return it, and does not appear to close it on all possible exception paths out of the method.

This may result in a file descriptor leak.

It is generally a good idea to use a finally block to ensure that streams are closed.

Don't reuse entry objects in iterators

The entrySet() method is allowed to return a view of the underlying Map in which an Iterator and Map.Entry.

This clever idea was used in several Map implementations, but introduces the possibility of nasty coding mistakes.

If a map m returns such an iterator for an entrySet, then c.addAll(m.entrySet()) will go badly wrong.

All of the Map implementations in OpenJDK 1.7 have been rewritten to avoid this, you should to.

Suspicious reference comparison to constant

This method compares a reference value to a constant using the == or != operator, where the correct way to compare instances of this type is generally with the equals() method.

It is possible to create distinct instances that are equal but do not compare as == since they are different objects.

Examples of classes which should generally not be compared by reference are java.lang.Integer, java.lang.Float, etc.

Suspicious reference comparison of Boolean values

This method compares two Boolean values using the == or != operator.

Normally, there are only two Boolean values (Boolean.TRUE and Boolean.FALSE), but it is possible to create other Boolean objects using the new Boolean(b) constructor.

It is best to avoid such objects, but if they do exist, then checking Boolean objects for equality using == or != will give results than are different than you would get using .equals(...)

Method ignores results of InputStream.read()

This method ignores the return value of one of the variants of java.io.InputStream.read() which can return multiple bytes.

If the return value is not checked, the caller will not be able to correctly handle the case where fewer bytes were read than the caller requested.

This is a particularly insidious kind of bug, because in many programs, reads from input streams usually do read the full amount of data requested, causing the program to fail only sporadically.

Method ignores results of InputStream.skip()

This method ignores the return value of java.io.InputStream.skip() which can skip multiple bytes.

If the return value is not checked, the caller will not be able to correctly handle the case where fewer bytes were skipped than the caller requested.

This is a particularly insidious kind of bug, because in many programs, skips from input streams usually do skip the full amount of data requested, causing the program to fail only sporadically.

With Buffered streams, however, skip() will only skip data in the buffer, and will routinely fail to skip the requested number of bytes.

Negating the result of compareTo()/compare()

This code negatives the return value of a compareTo or compare method.

This is a questionable or bad programming practice, since if the return value is Integer.MIN_VALUE, negating the return value won't negate the sign of the result.

You can achieve the same intended result by reversing the order of the operands rather than by negating the results.

Method ignores exceptional return value

This method returns a value that is not checked.

The return value should be checked since it can indicate an unusual or unexpected function execution.

For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception).

If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.

Static initializer creates instance before all static final fields assigned

The class's static initializer creates an instance of the class before all of the static final fields are assigned.

Certain swing methods needs to be invoked in Swing thread

The Swing methods show(), setVisible(), and pack() will create the associated peer for the frame.

With the creation of the peer, the system creates the event dispatch thread.

This makes things problematic because the event dispatch thread could be notifying listeners while pack and validate are still processing.

This situation could result in two threads going through the Swing component-based GUI -- it's a serious flaw that could result in deadlocks or other related threading issues.

A pack call causes components to be realized. As they are being realized (that is, not necessarily visible), they could trigger listener notification on the event dispatch thread.

Non-transient non-serializable instance field in serializable class

This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods.

Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

Non-serializable class has a serializable inner class

This Serializable class is an inner class of a non-serializable class.

Thus, attempts to serialize it will also attempt to associate instance of the outer class with which it is associated, leading to a runtime error.

If possible, making the inner class a static inner class should solve the problem.

Making the outer class serializable might also work, but that would mean serializing an instance of the inner class would always also serialize the instance of the outer class, which it often not what you really want.

Non-serializable value stored into instance field of a serializable class

A non-serializable value is stored into a non-transient field of a serializable class.

Comparator doesn't implement Serializable

This class implements the Comparator interface.

You should consider whether or not it should also implement the Serializable interface.

If a comparator is used to construct an ordered collection such as a TreeMap, then the TreeMap will be serializable only if the comparator is also serializable.

As most comparators have little or no state, making them serializable is generally easy and good defensive programming.

Serializable inner class

This Serializable class is an inner class.

Any attempt to serialize it will also serialize the associated outer instance.

The outer instance is serializable, so this won't fail, but it might serialize a lot more data than intended.

If possible, making the inner class a static inner class (also known as a nested class) should solve the problem.

serialVersionUID isn't final

This class defines a serialVersionUID field that is not final.

The field should be made final if it is intended to specify the version UID for purposes of serialization.

serialVersionUID isn't long

This class defines a serialVersionUID field that is not long.

The field should be made long if it is intended to specify the version UID for purposes of serialization.

serialVersionUID isn't static

This class defines a serialVersionUID field that is not static.

The field should be made static if it is intended to specify the version UID for purposes of serialization.

Class is Serializable but its superclass doesn't define a void constructor

This class implements the Serializable interface and its superclass does not.

When such an object is deserialized, the fields of the superclass need to be initialized by invoking the void constructor of the superclass.

Since the superclass does not have one, serialization and deserialization will fail at runtime.

Class is Externalizable but doesn't define a void constructor

This class implements the Externalizable interface, but does not define a void constructor.

When Externalizable objects are deserialized, they first need to be constructed by invoking the void constructor.

Since this class does not have one, serialization and deserialization will fail at runtime.

The readResolve method must be declared with a return type of Object

In order for the readResolve method to be recognized by the serialization mechanism, it must be declared to have a return type of Object.

Transient field that isn't set by deserialization.

This class contains a field that is updated at multiple places in the class, thus it seems to be part of the state of the class.

However, since the field is marked as transient and not set in readObject or readResolve, it will contain the default value in any deserialized instance of the class.

Class is Serializable, but doesn't define serialVersionUID

This class implements the Serializable interface, but does not define a serialVersionUID field.

A change as simple as adding a reference to a .class object will add synthetic fields to the class, which will unfortunately change the implicit serialVersionUID (e.g., adding a reference to String.class will generate a static field class$java$lang$String).

Also, different source code to bytecode compilers may use different naming conventions for synthetic variables generated for references to class objects or inner classes.

To ensure interoperability of Serializable across versions, consider adding an explicit serialVersionUID.

Usage of GetResource may be unsafe if class is extended

Calling this.getClass().getResource(...) could give results other than expected if this class is extended by a class in another package.