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

This code invoked a compareTo or compare method, and checks to see if the return value is a specific value, such as 1 or -1. When invoking these methods, you should only check the sign of the result, not for any specific non-zero value. While many or most compareTo and compare methods only return -1, 0 or 1, some of them will return other values.

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
  /* ... */
}

This rule verifies that single-line comments are not located at the end of a line of code. The main idea behind this rule is that in order to be really readable, trailing comments would have to be property written and formatted (correct alignment, no interference with the visual structure of the code, not too long to be visible) but most often, automatic code formatters would not handle this correctly: the code would end up less readable. Comments are far better placed on the previous empty line of code, where they will always be visible and properly formatted.

However, this rule allows to write trailing "metadata comments" - for which the pattern is configurable, as those metadata comments are usually very short and heavily used in some cases.

// The following line is non-compliant
int a1 = b + c; // This is a trailing comment that can be very very long

// This very long comment is better placed before the line of code
int a2 = b + c;

// The following line is compliant with the default configuration of the rule
String a3 = "id"; // $NON-NLS-1$

Use equals() to compare object references; avoid comparing them with ==.

In some situation, this compareTo or compare method returns the constant Integer.MIN_VALUE, which is an exceptionally bad practice. The only thing that matters about the return value of compareTo is the sign of the result. But people will sometimes negate the return value of compareTo, expecting that this will negate the sign of the result. And it will, except in the case where the value returned is Integer.MIN_VALUE. So just return -1 rather than Integer.MIN_VALUE.

A value specified as carrying a type qualifier annotation is compared with a value that doesn't ever carry that qualifier.

More precisely, a value annotated with a type qualifier specifying when=ALWAYS is compared with a value that 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 boolean example(@Negative Integer value1, @NonNegative Integer value2) {
  return value1.equals(value2);
}

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 constant names match a provided regular expression.

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

public class MyClass {
  public static final int first = 1; // Non-Compliant
  public static final int SECOND = 2; // Compliant
}

public enum MyEnum {
  first, // Non-Compliant
  SECOND; // Compliant
}

Making a constant just final as opposed to static final leads to duplicating its value for every instance of the class, uselessly increasing the amount of memory required to execute the application.

The following code:

public class Myclass {
  public final THRESHOLD = 3;           // Non-Compliant
}

should be refactored into:

public class Myclass {
  public static final THRESHOLD = 3;    // Compliant
}

According to Joshua Bloch, author of "Effective Java":

The constant interface pattern is a poor use of interfaces.

That a class uses some constants internally is an implementation detail. Implementing a constant interface causes this implementation detail to leak into the class's exported API. It is of no consequence to the users of a class that the class implements a constant interface. In fact, it may even confuse them. Worse, it represents a commitment: if in a future release the class is modified so that it no longer needs to use the constants, it still must implement the interface to ensure binary compatibility. If a nonfinal class implements a constant interface, all of its subclasses will have their namespaces polluted by the constants in the interface.

Noncompliant Code Example

interface Status {                      // Non-Compliant
   int OPEN = 1;
   int CLOSED = 2;
}

Compliant Solution

public enum Status {                    // Compliant
  OPEN,
  CLOSED;
}

or

public final class Status {             // Compliant
   public static final int OPEN = 1;
   public static final int CLOSED = 2;
}

Calling overridable methods during construction poses a risk of invoking methods on an incompletely constructed object and can be difficult to discern. It may leave the sub-class unable to construct its superclass or forced to replicate the construction process completely within itself, losing the ability to call super(). If the default constructor contains a call to an overridable method, the subclass may be completely uninstantiable. Note that this includes method calls throughout the control flow graph - i.e., if a constructor Foo() calls a private method bar() that calls a public method buz(), this denotes a problem.
Example :

public class SeniorClass {
  public SeniorClass(){
      toString(); //may throw NullPointerException if overridden
  }
  public String toString(){
    return "IAmSeniorClass";
  }
}
public class JuniorClass extends SeniorClass {
  private String name;
  public JuniorClass(){
    super(); //Automatic call leads to NullPointerException
    name = "JuniorClass";
  }
  public String toString(){
    return name.toUpperCase();
  }
}

Nested if, for, while and try statements is a key ingredient for making what's known as "Spaghetti code". Such code is hard to read, refactor and therefore maintain.

Noncompliant Code Example

The following code snippet illustrates this rule with the default threshold of 3.

public void process() {
  if (condition1) {                  // Compliant - depth = 1
    /* ... */
    if (condition2) {                // Compliant - depth = 2
      /* ... */
      for(int i = 0; i < 10; i++) {  // Compliant - depth = 3, not exceeding the limit
        /* ... */
        if (condition4) {            // Non-Compliant - depth = 4
          if (condition5) {          // Depth = 5, exceeding the limit, but issues are only reported on depth = 4
            /* ... */
          }
          return;
        }
      }
    }
  }
}

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.

A collection is added to itself. As a result, computing the hashCode of this set will throw a StackOverflowException.

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.

The initial value of this parameter is ignored, and the parameter is overwritten here. This often indicates a mistaken belief that the write to the parameter will be conveyed back to the caller.

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

This method unconditionally invokes itself. This would seem to indicate an infinite recursive loop that will result in a stack overflow.

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.

One of the arguments being formatted with a format string is an array. This will be formatted using a fairly useless format, such as [I@304282, which doesn't actually show the contents of the array. Consider wrapping the array using Arrays.asList(...) before handling it off to a formatted.

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 generates a random signed integer and then computes the absolute value of that random integer. If the number returned by the random number generator is Integer.MIN_VALUE, then the result will be negative as well (since Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE).