Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed. When a block contains a comment, this block is not considered to be empty.

The following code snippet illustrates this rule:

void doSomething() {
  for (int i = 0; i < 42; i++)        // Non-Compliant
  {
  }
  for (int i = 0; i < 42; i++);       // Compliant

  if (myVar == 4)                     // Compliant - contains a comment
  {
    // Do nothing because of X and Y
  }
  else                                // Compliant
  {
    doSomething();
  }

  try                                 // Non-Compliant
  {
  }
  catch (Exception e)                 // Compliant
  {
    // Ignore
  }
}

Nested code blocks can be used to create a new scope and restrict the visibility of the variables defined within. Using this feature in a method typically indicates that it takes on too many responsibilities, and should be refactored into smaller ones.

The following code:

public void evaluate(int operator) {
  switch (operator) {
    /* ... */
    case ADD: {                                // Non-Compliant - nested code block '{' ... '}'
        int a = stack.pop();
        int b = stack.pop();
        int result = a + b;
        stack.push(result);
        break;
      }
    /* ... */
  }
}

should be refactored into:

public void evaluate(int operator) {
  switch (operator) {
    /* ... */
    case ADD:                                  // Compliant
      evaluateAdd();
      break;
    /* ... */
  }
}

private void evaluateAdd() {
  int a = stack.pop();
  int b = stack.pop();
  int result = a + b;
  stack.push(result);
}

Non-static initializers are rarely used, and can be confusing for most developers. When possible, they should be refactored into standard constructors or field initializers.

The following code:

class MyClass {
  private static final Map MY_MAP = new HashMap() {

    // Non-Compliant - HashMap should be extended only to add behavior, not for initialization
    {
      put("a", "b");
    }

  };
}

could be refactored using Guava into:

class MyClass {
  // Compliant
  private static final Map MY_MAP = ImmutableMap.of("a", "b");
}

Object.finalize() is called by the Garbage Collector sometime after the object became unreferenced.

Overloading this method is misleading:

• The overloaded method will not be called by the Garbage Collector.
• Users are not expected to call Object.finalize() and will get confused.

Another name should be picked for the method.

The following code:

public void finalize(int someParameter) {        // Non-Compliant
  /* ... */
}

should be refactored into:

public void someBetterName(int someParameter) {  // Compliant
  /* ... */
}

The contract of the Object.finalize() method is clear: only the Garbage Collector is supposed to call this method. Making this method public is misleading, because it implies that any caller can use it.

The following code snippet illustrates this rule:

public class MyClass {

  @Override
  public void finalize() {    // Non-Compliant
    /* ... */
  }
}

Override both public boolean Object.equals(Object other), and public int Object.hashCode(), or override neither. Even if you are inheriting a hashCode() from a parent class, consider implementing hashCode and explicitly delegating to your superclass. Example :

// this is bad
public class Bar {
  public boolean equals(Object o) {
      // do some comparison
  }
}

// and so is this
public class Baz {
  public int hashCode() {
      // return some hash value
  }
}

// this is OK
public class Foo {
  public boolean equals(Object other) {
      // do some comparison
  }
  public int hashCode() {
      // return some hash value
  }
}

This rule is deprecated, use squid:S1206 instead.

Overriding a method just to call the same method from the super class without performing any other actions is useless and misleading.

The following code snippet illustrates this rule:

public void doSomething() {                     // Non-Compliant
  super.doSomething();
}

@Override
public boolean isLegal(Action action) {         // Non-Compliant
  return super.isLegal(action);
}

@Override
public boolean isLegal(Action action) {         // Compliant - not simply forwarding the call
  return super.isLegal(new Action(/* ... */));
}

@Id
@Override
public int getId() {                            // Compliant - there is annotation different from @Override
  return super.getId();
}

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

The following code snippet illustrates this rule when the regular expression value is "^[a-z]+(\\.[a-z][a-z0-9]*)*$":

package org.Example; // Non-Compliant

package org.example; // Compliant

Creating temporary primitive wrapper objects only for String conversion is inefficient. Instead, the static toString() method of those primitive wrapper classes should be used.

For example, the following code:

new Integer(myInteger).toString();  // Non-Compliant

should be refactored into:

Integer.toString(myInteger);      // Compliant

Using checked exceptions forces method callers to deal with errors, either by propagating them or by handling them. This makes those exceptions fully part of the API of the method.

To keep the complexity for callers reasonable, methods should not throw more than one kind of checked exception.

The following code:

public void delete() throws IOException, SQLException {      // Non-Compliant
  /* ... */
}

should be refactored into:

public void delete() throws SomeApplicationLevelException {  // Compliant
  /* ... */
}

Overriding methods are not checked by this rule and are allowed to throw several checked exceptions.

Return of boolean literal statements wrapped into if-then-else ones should be simplified.

For example, the following code:

if (someBooleanMethod()) {    // Non-Compliant
  return true;
} else {
  return false;
}

should be refactored into:

return someBooleanMethod();   // Compliant

and, the following code:

if (someBooleanMethod()) {    // Non-Compliant
  return false;
} else {
  return true;
}

should be refactored into:

return !someBooleanMethod();  // Compliant

Returning from a finally block suppresses the propagation of any unhandled Throwable which was thrown in the try or catch block.

The following code snippet illustrates this rule. The developer expects to get "ERROR" in the console whereas "OK" is displayed.

public static void main(String[] args) {
  try {
    doSomethingWhichThrowsException();
    System.out.println("OK");
  } catch (RuntimeException e) {
    System.out.println("ERROR");
  }
}

public static void doSomethingWhichThrowsException() {
  try {
    throw new RuntimeException();
  } finally {
    /* ... */
    return;                                        // Non-Compliant - prevents the RuntimeException from being propagated
  }
}

Sharing some coding conventions is a key point to make it possible for a team to efficiently collaborate. This rule make it mandatory to place closing curly braces on the same line that next else, catch or finally keywords.

The following code snippet illustrates this rule:

public void myMethod() {
  if(something) {
    executeTask();
  } else if (somethingElse) {          // Compliant
    doSomethingElse();
  }
  else {                               // Non-Compliant
     generateError();
  }

  try {
    generateOrder();
  } catch (Exception e) {              // Compliant
    log(e);
  }
  finally {                            // Non-Compliant
    closeConnection();
  }
}

Sharing some coding conventions is a key point to make it possible for a team to efficiently collaborate. This rule make it mandatory to place right curly braces at the beginning of lines of code.

The following code snippet illustrates this rule:

public void myMethod() {
  if(something) {
    executeTask();}                   // Non-Compliant
  else if (somethingElse) {
    doSomethingElse();
  }                                   // Compliant

  try {
    generateOrder();
  } finally {                         // Compliant
    closeConnection();
  }                                   // Compliant

  try {
    generateOrder();
  } finally {                         // Compliant
    closeConnection(); }              // Non-Compliant
}

Constructors and methods receiving arrays should clone objects and store the copy. This prevents that future changes from the user affect the internal functionality.

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