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

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.

Don't create instances of already existing BigInteger (BigInteger.ZERO, BigInteger.ONE) and for 1.5 on, BigInteger.TEN and BigDecimal (BigDecimal.ZERO, BigDecimal.ONE, BigDecimal.TEN)

Boolean expressions should be as compact as possible to improve readability. Useless operators should be removed.

Noncompliant Code Example

if (booleanVariable == true) { /* ... */ }
if (booleanVariable != true) { /* ... */ }
if (booleanVariable || false) { /* ... */ }
doSomething(!false);

Compliant Solution

if (booleanVariable) { /* ... */ }
if (!booleanVariable) { /* ... */ }
if (booleanVariable) { /* ... */ }
doSomething(true);

Avoid instantiating Boolean objects; you can reference Boolean.TRUE, Boolean.FALSE, or call Boolean.valueOf() instead.

The null check is broken since it will throw a Nullpointer itself. The reason is that a method is called on the object when it is null. It is likely that you used || instead of && or vice versa.

Using toLowerCase() or toUpperCase() to make case insensitive comparisons is inefficient because it requires the creation of temporary, intermediate String objects.

The following code:

boolean result1 = foo.toUpperCase().equals(bar);             // Non-Compliant
boolean result2 = foo.equals(bar.toUpperCase());             // Non-Compliant
boolean result3 = foo.toLowerCase().equals(bar.LowerCase()); // Non-Compliant

should be refactored into:

boolean result = foo.equalsIgnoreCase(bar);                  // Compliant

if you need to get an array of a class from your Collection, you should pass an array of the desidered class as the parameter of the toArray method. Otherwise you will get a ClassCastException.

Sharing some naming conventions is a key point to make it possible for a team to efficiently collaborate. This rule allows to check that all class names match a provided regular expression.

The following code snippet illustrates this rule when the regular expression value is "^[A-Z][a-zA-Z0-9]*$":

public class MyFirstClass { ... } // Compliant

public class mySecondClass { ... }    // Non-Compliant

Public class variable fields do not respect the encapsulation principle and has two main disadvantages:

• Additional behavior such as validation cannot be added.
• The internal representation is exposed, and cannot be changed afterwards.

The following code:

public class MyClass {

  public static final int SOME_CONSTANT = 0;     // Compliant - constants are not checked

  public String firstName;                       // Non-Compliant

}

should be refactored into:

public class MyClass {

  public static final int SOME_CONSTANT = 0;     // Compliant - constants are not checked

  private String firstName;                      // Compliant

  public String getFirstName() {
    return firstName;
  }

  public void setFirstName(String firstName) {
    this.firstName = firstName;
  }

}

Classes in com.sun.* and sun.* packages are considered implementation details, and are not part of the Java API. They can cause problems when moving to new versions of Java because there is no backwards compatibility guarantee. Such classes are almost always wrapped by Java API classes that should be used instead.

The following code snippet illustrates this rule:

import com.sun.jna.Native;     // Non-Compliant
import sun.misc.BASE64Encoder; // Non-Compliant

Ensure that resources (like Connection, Statement, and ResultSet objects) are always closed after use. It does this by looking for code patterned like :

Connection c = openConnection();
try {
  // do stuff, and maybe catch something
} finally {
  c.close();
}

Merging collapsible if statements increases the code's readability.

The following code:

if (condition1) {
  if (condition2) {             // Non-Compliant
    /* ... */
  }
}

should be refactored into:

if (condition1 && condition2) { // Compliant
  /* ... */
}

Using Collection.size() to test for emptiness works, but using Collection.isEmpty() makes the code more readable.

The following code:

if (myCollection.size() == 0) {  // Non-Compliant
  /* ... */
}

should be refactored into:

if (myCollection.isEmpty()) {    // Compliant
  /* ... */
}