No need to check for null before an instanceof; the instanceof keyword returns false when given a null argument.

A field that's only used by one method could perhaps be replaced by a local variable.

For better readability, do not put more than one statement on a single line.

The following code snippet illustrates this rule:

a = "something"; b = "somethingElse"; // Non-Compliant
c = "anotherThing"; // Compliant

StringBuffer sb = new StringBuffer('c'); The char will be converted into int to intialize StringBuffer size.

This rule is deprecated, use squid:S1317 instead.

Avoid instantiating String objects; this is usually unnecessary.

Duplicated string literals are error-prone to refactor, as one must pay attention to update all occurrences. Constants can be referenced from many places, but there value is updated in a single place.

The following code:

public void run() {
  prepare("action1");                              // Non-Compliant - "action1" is duplicated 3 times
  execute("action1");
  release("action1");
}

@SuppressWarning("all")                            // Compliant - annotations are excluded
private void method1(} { /* ... */ }
@SuppressWarning("all")
private void method2(} { /* ... */ }

public String method3(String a) {
  System.out.println("'" + a + "'");               // Compliant - literal "'" has less than 5 characters and is excluded
  return "";                                       // Compliant - literal "" has less than 5 characters and is excluded
}

should be refactored into:

private static final String ACTION_1 = "action1";  // Compliant

public void run() {
  prepare(ACTION_1);                               // Compliant
  execute(ACTION_1);
  release(ACTION_1);
}

Avoid calling toString() on String objects; this is unnecessary.

Appending String.valueOf() to a String decreases the code readability. The argument passed to String.valueOf() should be directly appended instead.

The following code:

public void display(int i){
  System.out.println("Output is " + String.valueOf(i));    // Non-Compliant
}

should be refactored into:

public void display(int i){
  System.out.println("Output is " + i);                    // Compliant
}

It is preferable to place string literals on the left-hand side of an equals() or equalsIgnoreCase() method call. This prevents null pointer exceptions from being raised, as a string literal can never be null by definition.

The following code:

String myString = null;

System.out.println("Equal? " + myString.equals("foo"));                        // Non-Compliant - will raise a NPE
System.out.println("Equal? " + (myString != null && myString.equals("foo")));  // Non-Compliant - null check could be removed

should be refactored into:

System.out.println("Equal?" + "foo".equals(myString));                         // Compliant - properly deals with the null case

String literals, just like any other Object, should be compared using the equals() method. Using == and != does not work in general.

The following code:

if (variable == "foo") { /* ... */ }         // Non-Compliant
if (variable != "foo") { /* ... */ }         // Non-Compliant

should be refactored into:

if ("foo".equals(variable)) { /* ... */ }    // Compliant
if (!"foo".equals(variable)) { /* ... */ }   // Compliant

The usual convention for Object.clone() according to Oracle's Javadoc is that:

• x.clone() != x
• x.clone().getClass() == x.getClass()
• x.clone().equals(x)

Obtaining the object that will be returned by calling super.clone() helps to satisfy those invariants:

• super.clone() returns a new object instance
• super.clone() returns an object of the same type as the one clone() was called on
• Object.clone() performs a shallow copy of the object's state

For example, the following code:

class BaseClass implements Cloneable {
  @Override
  public Object clone() throws CloneNotSupportedException {    // Non-Compliant - should return the super.clone() instance
    return new BaseClass();
  }
}

class DerivedClass extends BaseClass implements Cloneable {
  /* Does not override clone() */

  public void sayHello() {
    System.out.println("Hello, world!");
  }
}

class Application {
  public static void main(String[] args) throws Exception {
    DerivedClass instance = new DerivedClass();
    ((DerivedClass) instance.clone()).sayHello();              // Throws a ClassCastException because invariant #2 is violated
  }
}

should be refactored into:

class BaseClass implements Cloneable {
  @Override
  public Object clone() throws CloneNotSupportedException {    // Compliant
    return super.clone();
  }
}

class DerivedClass extends BaseClass implements Cloneable {
  /* Does not override clone() */

  public void sayHello() {
    System.out.println("Hello, world!");
  }
}

class Application {
  public static void main(String[] args) throws Exception {
    DerivedClass instance = new DerivedClass();
    ((DerivedClass) instance.clone()).sayHello();              // Displays "Hello, world!" as expected. Invariant #2 is satisfied
  }
}

Overriding the Object.finalize() method must be done with caution to dispose some system resources. Calling the super.finalize() at the end of this method implementation is highly recommended in case parent implementations must also dispose some system resources.

The following code snippet illustrates this rule:

protected void finalize() {            // Non-Compliant
  releaseSomeResources();
}

protected void finalize() {
  super.finalize();                    // Non-Compliant
  releaseSomeResources();
}

protected void finalize() {
  releaseSomeResources();
  super.finalize();                    // Compliant
}

When the execution is not explicitly terminated at the end of a switch case, it continues to execute the statements of the following case. While this is sometimes intentional, it often is a mistake which leads to unexpected behavior.

This rule doesn't apply to empty cases. Indeed those empty cases allow you to specify the same behavior for a group of cases.

The following code snippet illustrates this rule:

switch (myVariable) {
  case 0:                                // Compliant
  case 1:                                // Compliant
    doSomething();
    break;
  case 2:                                // Compliant
    return;
  case 3:                                // Compliant
    throw new IllegalStateException();
  case 4:                                // Compliant
    continue;
  case 5:                                // Non-Compliant - both 'doSomething()' and 'doSomethingElse()' will be executed
    doSomething();
  default:                               // Non-Compliant
    doSomethingElse();
}

Switch cases should remain small to keep the overall switch compact and readable.

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

switch (myVariable) {
  case 0:                     // Compliant - 5 lines till following case
    System.out.println("");
    System.out.println("");
    System.out.println("");
    break;
  default:                    // Non-Compliant - 6 lines till switch end
    System.out.println("");
    System.out.println("");
    System.out.println("");
    System.out.println("");
    break;
}

The requirement for a final default clause is defensive programming. This clause should either take appropriate action or contain a suitable comment as to why no action is taken.

The following code snippet illustrates this rule:

switch (state) {                       // Non-Compliant - must have a default case
  case 0:
  case 1:
    System.out.println("0 or 1!");
    break;
}

switch (state) {
  default:                             // Non-Compliant - must be last for better readability
    throw new IllegalStateException();
  case 0:
  case 1:
    System.out.println("0 or 1!");
    break;
}

switch (state) {
  case 0:
  case 1:
    System.out.println("0 or 1!");
    break;
  default:                             // Compliant
    throw new IllegalStateException();

Early classes of the Java API, such as Vector, Hashtable and StringBuffer, were synchronized to make them thread-safe. Unfortunately, synchronization has a big negative impact on performance, even when using these collections from a single thread.

It is better to use their new unsynchronized replacements:

• ArrayList or LinkedList instead of Vector
• Deque instead of Stack
• HashMap instead of Hashtable
• StringBuilder instead of StringBuffer

Noncompliant Code Example

Vector cats = new Vector();

Compliant Solution

ArrayList cats = new ArrayList(); 

Exceptions

Use of those synchronized classes is allowed in method signatures when overriding an existing method.

@Override
public Vector getCats() {...} 

Calling System.exit(int status) or Rutime.getRuntime().exit(int status) leads to the shut down of the entire Java virtual machine. It should be used with care, when stopping the whole Java process is meant. For instance, it should not be called from applications running in a J2EE container.

Noncompliant Code Example

System.exit(0);
Runtime.getRuntime().exit(0);

Two important requirements must be fulfilled when logging messages:

• The user must be able to easily retrieve the logs.
• The format of all messages must be uniform to enable users to easily browse them.

If a program directly writes to the standard output, there is absolutely no way to comply with these requirements. That's why defining and using a dedicated logger is highly recommended.

The following code snippet illustrates this rule:

System.out.println("My Message");  // Non-Compliant

logger.log("My Message");          // Compliant

Developers should not need to configure the tab width of their text editors in order to be able to read source code. So the use of tabulation character must be banned.

According to the Java Language Specification:

Unnamed packages are provided by the Java platform principally for convenience when developing small or temporary applications or when just beginning development.

To enforce this best practice, classes located in default package can no longer be access from named ones since Java 1.4.

The following piece of code:

public class MyClass { /* ... */ }

should be refactored into:

package org.example;

public class MyClass{ /* ... */ }

According to the official Java documentation, Object.finalize() is called by the garbage collector on an object when garbage collection determines that there are no more references to the object. Calling this method explicitly breaks this contract and so is misleading.

The following code snippet illustrates this rule:

public void dispose() throws Throwable {
  this.finalize();                       // Non-Compliant
}

This Object.finalize() method is called by the garbage collector on an object when garbage collection determines that there are no more references to the object. But there is absolutely no warranty that this method will be called AS SOON AS the last references to the object are removed. It can be few microseconds to few minutes later. So when some system resources need to be disposed by an object it's better to not rely on this asynchronous mechanism to dispose them.

The following piece of code illustrates this rule:

public class MyClass {

  protected void finalize() {
    releaseSomeResources();     // Non-Compliant
  }

}

Throwable is the superclass of all errors and exceptions in Java. Error is the superclass of all errors which are not meant to be caught by applications.

Catching either Throwable or Error will also catch OutOfMemoryError or InternalError from which an application should not attempt to recover.

Only Exception and its subclasses should be caught.

Noncompliant Code Example

try { /* ... */ } catch (Throwable t) { /* ... */ }
try { /* ... */ } catch (Error e) { /* ... */ } 

Compliant Solution

try { /* ... */ } catch (Exception e) { /* ... */ }  
try { /* ... */ } catch (RuntimeException e) { /* ... */ }  
try { /* ... */ } catch (MyException e) { /* ... */ }  

Throwable.printStackTrace(...) prints a throwable and its stack trace to some stream.

Loggers should be used instead to print throwables, as they have many advantages:

• Users are able to easily retrieve the logs.
• The format of log messages is uniform and allow users to browse the logs easily.

The following code:

try {
  /* ... */
} catch(Exception e) {
  e.printStackTrace();        // Non-Compliant
}

should be refactored into:

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

An exception in a throws declaration in Java is redundant if:

• It is listed multiple times
• It is a subclass of another listed exception
• It is a RuntimeException, or one of its descendants

The following code:

void foo() throws MyException, MyException {}  // Non-Compliant - should be listed once
void bar() throws Throwable, Exception {}      // Non-Compliant - Exception is a subclass of Throwable
void baz() throws RuntimeException {}          // Non-Compliant - RuntimeException can always be thrown

should be refactored into:

void foo() throws MyException {}               // Compliant
void bar() throws Throwable {}                 // Compliant
void baz() {}                                  // Compliant