As of Java 5, enum is a keyword and can therefore no longer be used a variable names for instance. Recent versions of the Java compiler will generate errors while compiling code still using enum as a name.

The following code snippet illustrates this rule:

public void doSomething() {
  int enum = 42;            // Non-Compliant
}

java.lang.Error and its subclasses represent abnormal conditions, such as OutOfMemoryError, which should only be encountered by the Java Virtual Machine.

The following code snippet:

public class MyException extends Error { /* ... */ }       // Non-Compliant

should be refactored into:

public class MyException extends Exception { /* ... */ }   // Compliant

The software uses an HTTP request parameter to construct a pathname that should be within a restricted directory, but it does not properly neutralize absolute path sequences such as "/abs/path" that can resolve to a location that is outside of that directory. See http://cwe.mitre.org/data/definitions/36.html for more information.

FindBugs looks only for the most blatant, obvious cases of absolute path traversal. If FindBugs found any, you almost certainly have more vulnerabilities that FindBugs doesn't report. If you are concerned about absolute path traversal, you should seriously consider using a commercial static analysis or pen-testing tool.

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.

This code increments a volatile field. Increments of volatile fields aren't atomic. If more than one thread is incrementing the field at the same time, increments could be lost.

While waiting for support of closure in Java, anonymous classes is the most convenient way to inject a behavior without having to create a dedicated class. But those anonymous inner classes should be used only if the behavior can be accomplished in a few lines. With more complex code, a named class is called for.

The following code snippet illustrates this rule with a threshold of 5:

button.addActionListener(new ActionListener() {   // Non-Compliant - 6 lines till closing '}'
  public void actionPerformed(ActionEvent e) {
    doSomething1();
    doSomething2();
  }
});

According to the Java Language Specification:

For compatibility with older versions of the Java SE platform,
the declaration of a method that returns an array is allowed to place (some or all of) the empty bracket pairs that form the declaration of the array type after the formal parameter list.
This obsolescent syntax should not be used in new code.

The following code snippet illustrates this rule:

public int getVector()[] { /* ... */ }    // Non-Compliant

public int[] getVector() { /* ... */ }    // Compliant

public int[] getMatrix()[] { /* ... */ }  // Non-Compliant

public int[][] getMatrix() { /* ... */ }  // Compliant

Array designators should always be located on the type for better code readability. Otherwise, developers must look both at the type and the variable name to know whether or not a variable is an array.

The following code snippet illustrates this rule:

int matrix[][];   // Non-Compliant
int[] matrix[];   // Non-Compliant
int[][] matrix;   // Compliant

Assignments within sub-expressions are hard to spot and therefore make the code less readable. It is also a common mistake to write = when == was meant. Ideally, expressions should not have side-effects.

Noncompliant Code Example

System.out.println(i = 42);

Compliant Solution

System.out.println(i == 42);

or:

i = 42;
System.out.println(i);

Exceptions

Assignments enclosed in relational expressions are allowed.

BufferedReader br = new BufferedReader(/* ... */);
String line;
while ((line = br.readLine()) != null) {
  /* ... */
}

Instead of copying data between two arrays, use System.arrayCopy method

Finds all places 'assert' is used as an identifier is used.

Code should never throw NPE under normal circumstances. A catch block may hide the original error, causing other more subtle errors in its wake.

Here are the main reasons why commented code is a code smell :

• It always raises more questions than it gives answers
• Everybody will forget very quickly how relevant the commented code is
• This is distraction when going down the code as it stops the flow of eyes
• It is a bad SCM engine : Subversion, CVS and Git are really more trustworthy !
• The simple fact of understanding why code was commented out in the first place can take a lot of time

One might assume that new BigDecimal(.1) is exactly equal to .1, but it is actually equal to .1000000000000000055511151231257827021181583404541015625. This is so because .1 cannot be represented exactly as a double (or, for that matter, as a binary fraction of any finite length). Thus, the long value that is being passed in to the constructor is not exactly equal to .1, appearances notwithstanding. The (String) constructor, on the other hand, is perfectly predictable: 'new BigDecimal(.1)' is exactly equal to .1, as one would expect. Therefore, it is generally recommended that the (String) constructor be used in preference to this one.

Avoid throwing a NullPointerException - it's confusing because most people will assume that the virtual machine threw it. Consider using an IllegalArgumentException instead; this will be clearly seen as a programmer-initiated exception.

The Cyclomatic Complexity is measured by the number of (&&, ||) operators and (if, while, do, for, ?:, catch, switch, case, return, throw) statements in the body of a class plus one for each constructor, method (but not getter/setter), static initializer, or instance initializer in the class. The last return stament in method, if exists, is not taken into account.

Even when the Cyclomatic Complexity of a class is very high, this complexity might be well distributed among all methods. Nevertheless, most of the time, a very complex class is a class which breaks the Single Responsibility Principle and which should be re-factored to be split in several classes.

This code compares an int value with a long constant that is outside the range of values that can be represented as an int value. This comparison is vacuous and possibily to be incorrect.

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 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 of SWT.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'.

Boris Bokowski

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

This rule is deprecated, use squid:S1210 instead.

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 rule is deprecated, use squid:S1206 instead.

This class overrides equals(Object), but does not override hashCode().  Therefore, the class may violate the invariant that equal objects must have equal hashcodes.

This rule is deprecated, use squid:S1206 instead.

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
  }

This rule is deprecated, use squid:S1206 instead.