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

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

This rule is meant to be used as a way to track code which is marked as being deprecated. Deprecated code should eventually be removed.

The following code illustrates this rule:

class Foo {
  /**
    * @deprecated
    */
  public void foo() {    // Non-Compliant
  }

  @Deprecated            // Non-Compliant
  public void bar() {
  }

  public void baz() {    // Compliant
  }
}

Deprecation should be made using both the @Deprecated Java language annotation and @deprecated Javadoc tag. The annotation enables tools such as IDEs to warn about referencing deprecated elements, and the tag can be used to explain when it was deprecated, why, and how references should be refactored.

The following code illustrates this rule:

class MyClass {

  @Deprecated             // Non-Compliant
  public void foo1() {
  }

  /**
    * @deprecated
    */
  public void foo2() {    // Non-Compliant
  }

  /**
    * @deprecated
    */
  @Deprecated
  public void foo3() {    // Compliant
  }

}

An issue is created on a file as soon as there is a block of duplicated code on this file. It gives the number of blocks in the file.

Returning null instead of an actual array or collection forces callers of the method to explicitly test for nullity, making them more complex and less readable. Moreover, in many cases, null is used as a synonym for empty.

The following code:

public static Result[] getResults() {
  return null;                             // Non-Compliant
}

public static void main(String[] args) {
  Result[] results = getResults();

  if (results != null) {                   // Nullity test required to prevent NPE
    for (Result result: results) {
      /* ... */
    }
  }
}

should be refactored into:

public static Result[] getResults() {
  return new Result[0];                    // Compliant
}

public static void main(String[] args) {
  for (Result result: getResults()) {
    /* ... */
  }
}

This rule also applies to collections:

public static List<Result> getResults() {
  return null;                             // Non-Compliant
}

should be refactored into:

public static List<Result> getResults() {
  return Collections.EMPTY_LIST;           // Compliant
}

If the finalize() method is empty, then it does not need to exist.

Empty statements, i.e. ;, are usually introduced by mistake, for example because:

• It was meant to be replaced by an actual statement, but this was forgotten.
• There was a typo which lead to the semicolon to be doubled, i.e. ;;.

Examples:

void doSomething() {
  ;                                                       // Non-Compliant - was used as a kind of TODO marker
}

System.out.println("Hello, world!");;                     // Non-Compliant - double ;

Rarely, they are used on purpose, as the body of a loop:

for (int i = 0; i < 3; System.out.println(i), i++);       // Non-Compliant

It is a bad practice to have side-effects outside of the loop body, and therefore that code should be refactored into:

for (int i = 0; i < 3; i ++) {                            // Compliant
  System.out.println(i);
}

From the official Oracle Javadoc:

NOTE: The functionality of this Enumeration interface is duplicated by the Iterator interface. In addition, Iterator adds an optional remove operation, and has shorter method names. New implementations should consider using Iterator in preference to Enumeration.

The following code:

public class MyClass implements Enumeration {  // Non-Compliant
  /* ... */
}

should be refactored into:

public class MyClass implements Iterator {     // Compliant
  /* ... */
}

Inexperienced programmers sometimes confuse comparison concepts and use equals() to compare to null.

This rule is deprecated, use squid:S1318 instead.

Exceptions are meant to represent the application's state at which an error occurred.

Making all fields final ensures that this state:

• Will be fully defined at the same time the exception is instantiated.
• Won't be updated or corrupted by some bogus error handler.

This will enable developers to quickly understand what went wrong.

The following code:

public class MyException extends Exception {

  private int status;                               // Non-Compliant

  public MyException(String message) {
    super(message);
  }

  public int getStatus() {
    return status;
  }

  public void setStatus(int status) {
    this.status = status;
  }

}

should be refactored into:

public class MyException extends Exception {

  private final int status;                         // Compliant

  public MyException(String message, int status) {
    super(message);
    this.status = status;
  }

  public int getStatus() {
    return status;
  }

}

When handling a caught exception, two mandatory informations should be logged:

• Some context to ease the reproduction of the issue.
• The original's exception, for its message and stack trace.

Noncompliant Code Example

 // Noncompliant - exception is lost
try { /* ... */ } catch (Exception e) { LOGGER.info("context"); }

// Noncompliant - context is required
try { /* ... */ } catch (Exception e) { LOGGER.info(e); }

// Noncompliant - exception is lost (only message is preserved)
try { /* ... */ } catch (Exception e) { LOGGER.info(e.getMessage()); }

// Noncompliant - exception is lost
try { /* ... */ } catch (Exception e) { throw new RuntimeException("context"); }

Compliant Solution

try { /* ... */ } catch (Exception e) { LOGGER.info("context", e); }

try { /* ... */ } catch (Exception e) { throw new RuntimeException("context", e); }

Exceptions

It is allowed to let the exception propagate.

try {
  /* ... */
} catch (RuntimeException e) {
  doSomething();
  throw e;
} catch (Exception e) {
  // Conversion into unchecked exception is also allowed
  throw new RuntimeException(e);
}

InterruptedException, NumberFormatException, ParseException and MalformedURLException exceptions are arguably used to indicate nonexceptional outcomes. As they are part of Java, developers have no choice but to deal with them. This rule does not verify that those particular exceptions are correctly handled.

int myInteger;
try {
  myInteger = Integer.parseInt(myString);
} catch (NumberFormatException e) {
  // It is perfectly acceptable to not handle "e" here
  myInteger = 0;
}

Multiple catch blocks of the appropriate type should be used instead of catching a general exception, and then testing on the type.

For example, following code:

try {
  /* ... */
} catch (Exception e) {
  if(e instanceof IOException) { /* ... */ }         // Non-Compliant
  if(e instanceof NullPointerException{ /* ... */ }  // Non-Compliant
}

should be refactored into:

try {
  /* ... */
} catch (IOException e) { /* ... */ }                // Compliant
} catch (NullPointerException e) { /* ... */ }       // Compliant

Throwing an exception from within a finally block will mask any exception which was previously thrown in the try or catch block. The masked's exception message and stack trace will be lost.

The following code:

try {
  /* some work which end up throwing an exception */
  throw new IllegalArgumentException();
} finally {
  /* clean up */
  throw new RuntimeException();                          // Non-Compliant - will mask the IllegalArgumentException
}

should be refactored into:

try {
  /* some work which end up throwing an exception */
  throw new IllegalArgumentException();
} finally {
  /* clean up */                                         // Compliant
}

Calling System.gc() or Runtime.getRuntime().gc() is a bad idea for a simple reason: there is no way to know exactly what will be done under the hood by the JVM because the behavior will depend on its vendor, version and options:

• Will the whole application be frozen during the call?
• Is the -XX:DisableExplicitGC option activated?
• Will the JVM simply ignore the call?
• ...

An application relying on those unpredictable methods is also unpredictable and therefore broken.

The task of running the garbage collector should be left exclusively to the JVM.

The complexity of an expression is defined by the number of &&, || and condition ? ifTrue : ifFalse operators it contains. A single expression's complexity should not become too high to keep the code readable.

The following code, with a maximum complexity of 3:

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

could be refactored into something like:

if (relevantMethodName1() && relevantMethodName2()) { /* ... */ }        // Compliant

/* ... */

private boolean relevantMethodName1() {
  return condition1 && condition2;
}

private boolean relevantMethodName2() {
  return condition3 && condition4;
}

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 field 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 MyClass {
  public int FIRST = 1; // Non-Compliant
  public int second = 2; // Compliant
}

FIXME tags are commonly used to mark places where a bug is suspected, but which the developer wants to deal with later. Sometimes the developer will not have the time or will simply forget to get back to that tag. This rule is meant to track those tags, and ensure that they do not go unnoticed.

The following code illustrates this rule:

int divide(int numerator, int denominator) {
  return numerator / denominator;              // FIXME denominator = 0, Non-Compliant
}

Using such generic exception prevents calling methods from handling differently each kind of error.

The following code snippet illustrates this rule:

public void foo(String bar) throws Throwable { // Non-Compliant
  throw new RuntimeException("My Message");    // Non-Compliant
}

public void foo(String bar) {
  throw new MyRuntimeException("My Message");  // Compliant
}

Avoid idempotent operations - they are have no effect. Example :
int x = 2;
x = x;