Quality profiles / java / Sonar - FB - eHour

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.

(From JDC Tech Tip): 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.

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.

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 
  }

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 implements Cloneable but does not define or use the clone method.

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.

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.

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.

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.

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.

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.

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.

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().

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).

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.

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.

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.

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().

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.

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.

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.

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.

This code invokes a method that requires a security permission check. If this code will be granted security permissions, but might be invoked by code that does not have security permissions, then the invocation needs to occur inside a doPrivileged block.

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.

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.

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.

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.

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.

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

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.

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.

This code seems to be storing a non-serializable object into an HttpSession. If this session is passivated or migrated, an error will result.

This method compares two reference values using the == or != operator, where the correct way to compare instances of this type is generally with the equals() method. Examples of classes which should generally not be compared by reference are java.lang.Integer, java.lang.Float, etc.

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.

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

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.

A String function is being invoked and "." is being passed to a parameter that takes a regular expression as an argument. Is this what you intended? For example s.replaceAll(".", "/") will return a String in which every character has been replaced by a / character.

This instanceof test will always return false, since the value being checked is guaranteed to be null. Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error.

This loop doesn't seem to have a way to terminate (other than by perhaps throwing an exception).

This regular method has the same name as the class it is defined in. It is likely that this was intended to be a constructor. If it was intended to be a constructor, remove the declaration of a void return value. If you had accidently defined this method, realized the mistake, defined a proper constructor but can't get rid of this method due to backwards compatibility, deprecate the method.

This code generates a hashcode and then computes the absolute value of that hashcode. If the hashcode is Integer.MIN_VALUE, then the result will be negative as well (since Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE).

This code compares a value that is guaranteed to be non-negative with a negative constant.

This code passes a constant month value outside the expected range of 0..11 to a method.

Loads a value from a byte array and performs a bitwise OR with that value. Values loaded from a byte array are sign extended to 32 bits before any any bitwise operations are performed on the value. Thus, if b[0] contains the value 0xff, and x is initially 0, then the code ((x << 8) | b[0]) will sign extend 0xff to get 0xffffffff, and thus give the value 0xffffffff as the result.

In particular, the following code for packing a byte array into an int is badly wrong:

int result = 0;
for(int i = 0; i < 4; i++) 
  result = ((result << 8) | b[i]);

The following idiom will work instead:

int result = 0;
for(int i = 0; i < 4; i++) 
  result = ((result << 8) | (b[i] & 0xff));

This method calls equals(Object) on two references of different class types with no common subclasses. Therefore, the objects being compared are unlikely to be members of the same class at runtime (unless some application classes were not analyzed, or dynamic class loading can occur at runtime). According to the contract of equals(), objects of different classes should always compare as unequal; therefore, according to the contract defined by java.lang.Object.equals(Object), the result of this comparison will always be false at runtime.

This method calls equals(Object), passing a null value as the argument. According to the contract of the equals() method, this call should always return false.

This method compares an expression such as

((event.detail & SWT.SELECTED) > 0)

Boris Bokowski

This class defines a field with the same name as a visible instance field in a superclass. This is confusing, and may indicate an error if methods update or access one of the fields when they wanted the other.

This class defines an enumeration, and equality on enumerations are defined using object identity. Defining a covariant equals method for an enumeration value is exceptionally bad practice, since it would likely result in having two different enumeration values that compare as equals using the covariant enum method, and as not equal when compared normally. Don't do it.

(Javadoc) A ScheduledThreadPoolExecutor with zero core threads will never execute anything; changes to the max pool size are ignored.

This code opens a file in append mode and then wraps the result in an object output stream. This won't allow you to append to an existing object output stream stored in a file. If you want to be able to append to an object output stream, you need to keep the object output stream open.

The only situation in which opening a file in append mode and the writing an object output stream could work is if on reading the file you plan to open it in random access mode and seek to the byte offset where the append started.

TODO: example.

This method contains a double assignment of a field; e.g.

  int x,y;
  public void foo() {
    x = x = 17;
  }

Assigning to a field twice is useless, and may indicate a logic error or typo.

This class defines an equals method that always returns false. This means that an object is not equal to itself, and it is impossible to create useful Maps or Sets of this class. More fundamentally, it means that equals is not reflexive, one of the requirements of the equals method.

The likely intended semantics are object identity: that an object is equal to itself. This is the behavior inherited from class Object. If you need to override an equals inherited from a different superclass, you can use use:

public boolean equals(Object o) { return this == o; }

This method checks to see if two objects are the same class by checking to see if the names of their classes are equal. You can have different classes with the same name if they are loaded by different class loaders. Just check to see if the class objects are the same.

This class defines an equals() method, that doesn't override the normal equals(Object) method defined in the base java.lang.Object class.  Instead, it inherits an equals(Object) method from a superclass. The class should probably define a boolean equals(Object) method.

This method invokes the .equals(Object o) to compare an array and a reference that doesn't seem to be an array. If things being compared are of different types, they are guaranteed to be unequal and the comparison is almost certainly an error. Even if they are both arrays, the equals method on arrays only determines of the two arrays are the same object. To compare the contents of the arrays, use java.util.Arrays.equals(Object[], Object[]).

This code creates an exception (or error) object, but doesn't do anything with it. For example, something like

if (x < 0)
  new IllegalArgumentException("x must be nonnegative");

It was probably the intent of the programmer to throw the created exception:

if (x < 0)
  throw new IllegalArgumentException("x must be nonnegative");

A value is used in a way that requires it to be always be a value denoted by a type qualifier, but there is an explicit annotation stating that it is not known where the value is required to have that type qualifier. Either the usage or the annotation is incorrect.

The code here uses File.separator where a regular expression is required. This will fail on Windows platforms, where the File.separator is a backslash, which is interpreted in a regular expression as an escape character. Amoung other options, you can just use File.separatorChar=='\\' & "\\\\" : File.separator instead of File.separator

Not enough arguments are passed to satisfy a placeholder in the format string. A runtime exception will occur when this statement is executed.

The hasNext() method invokes the next() method. This is almost certainly wrong, since the hasNext() method is not supposed to change the state of the iterator, and the next method is supposed to change the state of the iterator.

This cast will always throw a ClassCastException.

This code is casting the result of calling toArray() on a collection to a type more specific than Object[], as in:

  String[] getAsArray(Collection c) {
    return (String[]) c.toArray();
  }

This will usually fail by throwing a ClassCastException. The toArray() of almost all collections return an Object[]. They can't really do anything else, since the Collection object has no reference to the declared generic type of the collection.

The correct way to do get an array of a specific type from a collection is to use c.toArray(new String[]); or c.toArray(new String[c.size()]); (the latter is slightly more efficient).

There is one common/known exception exception to this. The toArray() method of lists returned by Arrays.asList(...) will return a covariantly typed array. For example, Arrays.asArray(new String[] { "a" }).toArray() will return a String []. FindBugs attempts to detect and suppress such cases, but may miss some.

This method compares an expression of the form (e | C) to D. which will always compare unequal due to the specific values of constants C and D. This may indicate a logic error or typo.

Typically, this bug occurs because the code wants to perform a membership test in a bit set, but uses the bitwise OR operator ("|") instead of bitwise AND ("&").

This code converts an int value to a double precision floating point number and then passing the result to the Math.ceil() function, which rounds a double to the next higher integer value. This operation should always be a no-op, since the converting an integer to a double should give a number with no fractional part. It is likely that the operation that generated the value to be passed to Math.ceil was intended to be performed using double precision floating point arithmetic.

The code multiplies the result of an integer remaining by an integer constant. Be sure you don't have your operator precedence confused. For example i % 60 * 1000 is (i % 60) * 1000, not i % (60 * 1000).

The code performs an integer shift by a constant amount outside the range 0..31. The effect of this is to use the lower 5 bits of the integer value to decide how much to shift by. This probably isn't want was expected, and it at least confusing.

This method invokes the .equals(Object o) method on an array. Since arrays do not override the equals method of Object, calling equals on an array is the same as comparing their addresses. To compare the contents of the arrays, use java.util.Arrays.equals(Object[], Object[]).

The code invokes toString on an (anonymous) array. Calling toString on an array generates a fairly useless result such as [C@16f0472. Consider using Arrays.toString to convert the array into a readable String that gives the contents of the array. See Programming Puzzlers, chapter 3, puzzle 12.

A JUnit assertion is performed in a run method. Failed JUnit assertions just result in exceptions being thrown. Thus, if this exception occurs in a thread other than the thread that invokes the test method, the exception will terminate the thread but not result in the test failing.

This method assigns a literal boolean value (true or false) to a boolean variable inside an if or while expression. Most probably this was supposed to be a boolean comparison using ==, not an assignment using =.

A call to getXXX or updateXXX methods of a result set was made where the field index is 0. As ResultSet fields start at index 1, this is always a mistake.

This method call passes a null value for a nonnull method parameter. Either the parameter is annotated as a parameter that should always be nonnull, or analysis has shown that it will always be dereferenced.

This method defines a local variable with the same name as a field in this class or a superclass. This may cause the method to read an uninitialized value from the field, leave the field uninitialized, or both.

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; }
}

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.

The return value of this method should be checked. One common cause of this warning is to invoke a method on an immutable object, thinking that it updates the object. For example, in the following code fragment,

String dateString = getHeaderField(name);
dateString.trim();

the programmer seems to be thinking that the trim() method will update the String referenced by dateString. But since Strings are immutable, the trim() function returns a new String value, which is being ignored here. The code should be corrected to:

String dateString = getHeaderField(name);
dateString = dateString.trim();

This class implements the Serializable interface, and defines a method for custom serialization/deserialization. But since that method isn't declared private, it will be silently ignored by the serialization/deserialization API.

A format-string method with a variable number of arguments is called, but more arguments are passed than are actually used by the format string. This won't cause a runtime exception, but the code may be silently omitting information that was intended to be included in the formatted string.

This call to a generic collection method contains an argument with an incompatible class from that of the collection's parameter (i.e., the type of the argument is neither a supertype nor a subtype of the corresponding generic type argument). Therefore, it is unlikely that the collection contains any objects that are equal to the method argument used here. Most likely, the wrong value is being passed to the method.

In general, instances of two unrelated classes are not equal. For example, if the Foo and Bar classes are not related by subtyping, then an instance of Foo should not be equal to an instance of Bar. Among other issues, doing so will likely result in an equals method that is not symmetrical. For example, if you define the Foo class so that a Foo can be equal to a String, your equals method isn't symmetrical since a String can only be equal to a String.

In rare cases, people do define nonsymmetrical equals methods and still manage to make their code work. Although none of the APIs document or guarantee it, it is typically the case that if you check if a Collection<String> contains a Foo, the equals method of argument (e.g., the equals method of the Foo class) used to perform the equality checks.

This method performs a nonsensical computation of a field with another reference to the same field (e.g., x&x or x-x). Because of the nature of the computation, this operation doesn't seem to make sense, and may indicate a typo or a logic error. Double check the computation.

A null pointer is dereferenced here.  This will lead to a NullPointerException when the code is executed.

There is a statement or branch that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions).

A format-string method with a variable number of arguments is called, but the number of arguments passed does not match with the number of % placeholders in the format string. This is probably not what the author intended.

There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed. Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs.

This code passes a primitive array to a function that takes a variable number of object arguments. This creates an array of length one to hold the primitive array and passes it to the function.

A random value from 0 to 1 is being coerced to the integer value 0. You probably want to multiple the random value by something else before coercing it to an integer, or use the Random.nextInt(n) method.

The code contains a conditional test is performed twice, one right after the other (e.g., x == 0 || x == 0). Perhaps the second occurrence is intended to be something else (e.g., x == 0 || y == 0).

This method contains a self assignment of a field; e.g.

  int x;
  public void foo() {
    x = x;
  }

Such assignments are useless, and may indicate a logic error or typo.

This method compares a local variable with itself, and may indicate a typo or a logic error. Make sure that you are comparing the right things.

This method invokes the Thread.interrupted() method on a Thread object that appears to be a Thread object that is not the current thread. As the interrupted() method is static, the interrupted method will be called on a different object than the one the author intended.

Class is a JUnit TestCase and defines a suite() method. However, the suite method needs to be declared as either

public static junit.framework.Test suite()

public static junit.framework.TestSuite suite()

Class is a JUnit TestCase and implements the tearDown method. The tearDown method should call super.tearDown(), but doesn't.

Class is a JUnit TestCase and implements the suite() method. The suite method should be declared as being static, but isn't.

One of the arguments is uncompatible with the corresponding format string specifier. As a result, this will generate a runtime exception when executed. For example, String.format("%d", "1") will generate an exception, since the String "1" is incompatible with the format specifier %d.

This constructor reads a field which has not yet been assigned a value.  This is often caused when the programmer mistakenly uses the field instead of one of the constructor's parameters.

Type check performed using the instanceof operator where it can be statically determined whether the object is of the type requested.

This field is never written. All reads of it will return the default value. Check for errors (should it have been initialized?), or remove it if it is useless.

This statement assigns to a local variable in a return statement. This assignment has effect. Please verify that this statement does the right thing.

This method uses using pointer equality to compare two references that seem to be of different types. The result of this comparison will always be false at runtime.

A value specified as carrying a type qualifier annotation is consumed in a location or locations requiring that the value not carry that annotation.

More precisely, a value annotated with a type qualifier specifying when=ALWAYS is guaranteed to reach a use or uses where the same type qualifier specifies when=NEVER.

For example, say that @NonNegative is a nickname for the type qualifier annotation @Negative(when=When.NEVER). The following code will generate this warning because the return statement requires a @NonNegative value, but receives one that is marked as @Negative.

public @NonNegative Integer example(@Negative Integer value) {
    return value;
}

There is a statement or branch on an exception path that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions).

A value that is annotated as possibility not being an instance of the values denoted by the type qualifier, and the value is guaranteed to be used in a way that requires values denoted by that type qualifier.

An inner class is invoking a method that could be resolved to either a inherited method or a method defined in an outer class. By the Java semantics, it will be resolved to invoke the inherited method, but this may not be want you intend. If you really intend to invoke the inherited method, invoke it by invoking the method on super (e.g., invoke super.foo(17)), and thus it will be clear to other readers of your code and to FindBugs that you want to invoke the inherited method, not the method in the outer class.

The code uses x % 2 == 1 to check to see if a value is odd, but this won't work for negative numbers (e.g., (-5) % 2 == -1). If this code is intending to check for oddness, consider using x & 1 == 1, or x % 2 != 0.

This class extends from a Servlet class, and uses an instance member variable. Since only one instance of a Servlet class is created by the J2EE framework, and used in a multithreaded way, this paradigm is highly discouraged and most likely problematic. Consider only using method local variables.

This class declares that it implements an interface that is also implemented by a superclass. This is redundant because once a superclass implements an interface, all subclasses by default also implement this interface. It may point out that the inheritance hierarchy has changed since this class was created, and consideration should be given to the ownership of the interface's implementation.

This class is bigger than can be effectively handled, and was not fully analyzed for errors.

Are you sure this for loop is incrementing the correct variable? It appears that another variable is being initialized and checked by the for loop.

It is often a better design to return a length zero array rather than a null reference to indicate that there are no results (i.e., an empty list of results). This way, no explicit check for null is needed by clients of the method.

On the other hand, using null to indicate "there is no answer to this question" is probably appropriate. For example, File.listFiles() returns an empty list if given a directory containing no files, and returns null if the file is not a directory.

This instruction assigns a value to a local variable, but the value is not read or used in any subsequent instruction. Often, this indicates an error, because the value computed is never used.

Note that Sun's javac compiler often generates dead stores for final local variables. Because FindBugs is a bytecode-based tool, there is no easy way to eliminate these false positives.

This method contains a double assignment of a local variable; e.g.

  public void foo() {
    int x,y;
    x = x = 17;
  }

Assigning the same value to a variable twice is useless, and may indicate a logic error or typo.

The result of invoking readLine() is immediately dereferenced. If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception.

This instanceof test will always return true (unless the value being tested is null). Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error. If you really want to test the value for being null, perhaps it would be clearer to do better to do a null test rather than an instanceof test.

This code invokes substring(0) on a String, which returns the original value.

The method invokes String.indexOf and checks to see if the result is positive or non-positive. It is much more typical to check to see if the result is negative or non-negative. It is positive only if the substring checked for occurs at some place other than at the beginning of the String.

The value returned by readLine is discarded after checking to see if the return value is non-null. In almost all situations, if the result is non-null, you will want to use that non-null value. Calling readLine again will give you a different line.

This method uses the same code to implement two clauses of a switch statement. This could be a case of duplicate code, but it might also indicate a coding mistake.

An argument not of type Boolean is being formatted with a %b format specifier. This won't throw an exception; instead, it will print true for any nonnull value, and false for null. This feature of format strings is strange, and may not be what you intended.

The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null. This may lead to a NullPointerException when the code is executed.

This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||). In addition, it seem possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects, could cause an exception or could be expensive.

Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.

See the Java Language Specification for details

This code casts a Collection to an abstract collection (such as List, Set, or Map). Ensure that you are guaranteed that the object is of the type you are casting to. If all you need is to be able to iterate through a collection, you don't need to cast it to a Set or List.

This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||). Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.

See the Java Language Specification for details

This method contains a redundant comparison of two references known to both be definitely null.

This method contains a redundant check of a known null value against the constant null.

This code computes a hashCode, and then computes the remainder of that value modulo another value. Since the hashCode can be negative, the result of the remainder operation can also be negative.

Assuming you want to ensure that the result of your computation is nonnegative, you may need to change your code. If you know the divisor is a power of 2, you can use a bitwise and operator instead (i.e., instead of using x.hashCode()%n, use x.hashCode()&(n-1). This is probably faster than computing the remainder as well. If you don't know that the divisor is a power of 2, take the absolute value of the result of the remainder operation (i.e., use Math.abs(x.hashCode()%n)

This method contains a self assignment of a local variable; e.g.

  public void foo() {
    int x = 3;
    x = x;
  }

Such assignments are useless, and may indicate a logic error or typo.

A Thread object is passed as a parameter to a method where a Runnable is expected. This is rather unusual, and may indicate a logic error or cause unexpected behavior.

This cast is unchecked, and not all instances of the type casted from can be cast to the type it is being cast to. Ensure that your program logic ensures that this cast will not fail.

This class doesn't do any of the patterns we recognize for checking that the type of the argument is compatible with the type of the this object. There might not be anything wrong with this code, but it is worth reviewing.

This is an integer bit operation (and, or, or exclusive or) that doesn't do any useful work (e.g., v & 0xffffffff).

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

OpenJDK introduces a potential incompatibility. In particular, the java.util.logging.Logger behavior has changed. Instead of using strong references, it now uses weak references internally. That's a reasonable change, but unfortunately some code relies on the old behavior - when changing logger configuration, it simply drops the logger reference. That means that the garbage collector is free to reclaim that memory, which means that the logger configuration is lost. For example, consider:

public static void initLogging() throws Exception {
      Logger logger = Logger.getLogger("edu.umd.cs");
      logger.addHandler(new FileHandler()); // call to change logger configuration
      logger.setUseParentHandlers(false); // another call to change logger configuration
      }

The logger reference is lost at the end of the method (it doesn't escape the method), so if you have a garbage collection cycle just after the call to initLogging, the logger configuration is lost (because Logger only keeps weak references).

public static void main(String[] args) throws Exception {
      initLogging(); // adds a file handler to the logger
      System.gc(); // logger configuration lost
      Logger.getLogger("edu.umd.cs").info("Some message"); // this isn't logged to the file as expected
      }

Ulf Ochsenfahrt and Eric Fellheimer

A final static field references a Hashtable and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the Hashtable.

A mutable static field could be changed by malicious code or by accident from another package. The field could be made final to avoid this vulnerability.

A final static field that is defined in an interface references a mutable object such as an array or hashtable. This mutable object could be changed by malicious code or by accident from another package. To solve this, the field needs to be moved to a class and made package protected to avoid this vulnerability.

A class's finalize() method should have protected access, not public.

A public static method returns a reference to an array that is part of the static state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array.

This declares a volatile reference to an array, which might not be what you want. With a volatile reference to an array, reads and writes of the reference to the array are treated as volatile, but the array elements are non-volatile. To get volatile array elements, you will need to use one of the atomic array classes in java.util.concurrent (provided in Java 5.0).

As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use. The detector has found a call to an instance of DateFormat that has been obtained via a static field. This looks suspicous.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.

This class has a writeObject() method which is synchronized; however, no other method of the class is synchronized.