sebthom/jstuff

Just because you can do something, doesn't mean you should, and that's the case with nested ternary operations. Nesting ternary operators results in the kind of code that may seem clear as day when you write it, but six months later will leave maintainers (or worse - future you) scratching their heads and cursing.

Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement.

Noncompliant Code Example

public String getReadableStatus(Job j) {
  return j.isRunning() ? "Running" : j.hasErrors() ? "Failed" : "Succeeded";  // Noncompliant
}

Compliant Solution

public String getReadableStatus(Job j) {
  if (j.isRunning()) {
    return "Running";
  }
  return j.hasErrors() ? "Failed" : "Succeeded";
}

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Noncompliant Code Example

With the default threshold of 3:

public void run() {
  prepare("action1");                              // Noncompliant - "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
}

Compliant Solution

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

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

Exceptions

To prevent generating some false-positives, literals having less than 5 characters are excluded.

A dead store happens when a local variable is assigned a value that is not read by any subsequent instruction. Calculating or retrieving a value only to then overwrite it or throw it away, could indicate a serious error in the code. Even if it's not an error, it is at best a waste of resources. Therefore all calculated values should be used.

Noncompliant Code Example

i = a + b; // Noncompliant; calculation result not used before value is overwritten
i = compute();

Compliant Solution

i = a + b;
i += compute();

Exceptions

This rule ignores initializations to -1, 0, 1, null, true, false and "".

See

• MITRE, CWE-563 - Assignment to Variable without Use ('Unused Variable')
• CERT, MSC13-C. - Detect and remove unused values
• CERT, MSC56-J. - Detect and remove superfluous code and values

Using such generic exceptions as Error, RuntimeException, Throwable, and Exception prevents calling methods from handling true, system-generated exceptions differently than application-generated errors.

Noncompliant Code Example

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

Compliant Solution

public void foo(String bar) {
  throw new MyOwnRuntimeException("My Message");
}

Exceptions

Generic exceptions in the signatures of overriding methods are ignored, because overriding method has to follow signature of the throw declaration in the superclass. The issue will be raised on superclass declaration of the method (or won't be raised at all if superclass is not part of the analysis).

@Override
public void myMethod() throws Exception {...}

Generic exceptions are also ignored in the signatures of methods that make calls to methods that throw generic exceptions.

public void myOtherMethod throws Exception {
  doTheThing();  // this method throws Exception
}

See

• MITRE, CWE-397 - Declaration of Throws for Generic Exception
• CERT, ERR07-J. - Do not throw RuntimeException, Exception, or Throwable

Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of code. Further, it could lead maintainers to introduce bugs because they think they're using one variable but are really using another.

Noncompliant Code Example

class Foo {
  public int myField;

  public void doSomething() {
    int myField = 0;
    ...
  }
}

See

• CERT, DCL01-C. - Do not reuse variable names in subscopes
• CERT, DCL51-J. - Do not shadow or obscure identifiers in subscopes

Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.

See

• Cognitive Complexity

Before Java 8, the only way to partially support closures in Java was by using anonymous inner classes. But the syntax of anonymous classes may seem unwieldy and unclear.

With Java 8, most uses of anonymous inner classes should be replaced by lambdas to highly increase the readability of the source code.

Note that this rule is automatically disabled when the project's sonar.java.source is lower than 8.

Noncompliant Code Example

myCollection.stream().map(new Mapper<String,String>() {
  public String map(String input) {
    return new StringBuilder(input).reverse().toString();
  }
});

Predicate<String> isEmpty = new Predicate<String> {
    boolean test(String myString) {
        return myString.isEmpty();
    }
}

Compliant Solution

myCollection.stream().map(input -> new StringBuilder(input).reverse().toString());

Predicate<String> isEmpty = myString -> myString.isEmpty();

Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.

Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.

Noncompliant Code Example

class StringUtils { // Noncompliant

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Compliant Solution

class StringUtils { // Compliant

  private StringUtils() {
    throw new IllegalStateException("Utility class");
  }

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Exceptions

When class contains public static void main(String[] args) method it is not considered as utility class and will be ignored by this rule.

It is highly recommended not to use wildcard types as return types. Because the type inference rules are fairly complex it is unlikely the user of that API will know how to use it correctly.

Let's take the example of method returning a "List<? extends Animal>". Is it possible on this list to add a Dog, a Cat, ... we simply don't know. And neither does the compiler, which is why it will not allow such a direct use. The use of wildcard types should be limited to method parameters.

This rule raises an issue when a method returns a wildcard type.

Noncompliant Code Example

List<? extends Animal> getAnimals(){...}

Compliant Solution

List<Animal> getAnimals(){...}

or

List<Dog> getAnimals(){...}

Using such generic exceptions as Error, RuntimeException, Throwable, and Exception prevents calling methods from handling true, system-generated exceptions differently than application-generated errors.

Noncompliant Code Example

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

Compliant Solution

public void foo(String bar) {
  throw new MyOwnRuntimeException("My Message");
}

Exceptions

Generic exceptions in the signatures of overriding methods are ignored, because overriding method has to follow signature of the throw declaration in the superclass. The issue will be raised on superclass declaration of the method (or won't be raised at all if superclass is not part of the analysis).

@Override
public void myMethod() throws Exception {...}

Generic exceptions are also ignored in the signatures of methods that make calls to methods that throw generic exceptions.

public void myOtherMethod throws Exception {
  doTheThing();  // this method throws Exception
}

See

• MITRE, CWE-397 - Declaration of Throws for Generic Exception
• CERT, ERR07-J. - Do not throw RuntimeException, Exception, or Throwable

Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.

See

• Cognitive Complexity

Fields in a Serializable class must themselves be either Serializable or transient even if the class is never explicitly serialized or deserialized. For instance, under load, most J2EE application frameworks flush objects to disk, and an allegedly Serializable object with non-transient, non-serializable data members could cause program crashes, and open the door to attackers. In general a Serializable class is expected to fulfil its contract and not have an unexpected behaviour when an instance is serialized.

This rule raises an issue on non-Serializable fields, and on collection fields when they are not private (because they could be assigned non-Serializable values externally), and when they are assigned non-Serializable types within the class.

Noncompliant Code Example

public class Address {
  //...
}

public class Person implements Serializable {
  private static final long serialVersionUID = 1905122041950251207L;

  private String name;
  private Address address;  // Noncompliant; Address isn't serializable
}

Compliant Solution

public class Address implements Serializable {
  private static final long serialVersionUID = 2405172041950251807L;
}

public class Person implements Serializable {
  private static final long serialVersionUID = 1905122041950251207L;

  private String name;
  private Address address;
}

Exceptions

The alternative to making all members serializable or transient is to implement special methods which take on the responsibility of properly serializing and de-serializing the object. This rule ignores classes which implement the following methods:

 private void writeObject(java.io.ObjectOutputStream out)
     throws IOException
 private void readObject(java.io.ObjectInputStream in)
     throws IOException, ClassNotFoundException;

See

• MITRE, CWE-594 - Saving Unserializable Objects to Disk
• Oracle Java 6, Serializable
• Oracle Java 7, Serializable

Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.

Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.

Noncompliant Code Example

class StringUtils { // Noncompliant

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Compliant Solution

class StringUtils { // Compliant

  private StringUtils() {
    throw new IllegalStateException("Utility class");
  }

  public static String concatenate(String s1, String s2) {
    return s1 + s2;
  }

}

Exceptions

When class contains public static void main(String[] args) method it is not considered as utility class and will be ignored by this rule.

Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of code. Further, it could lead maintainers to introduce bugs because they think they're using one variable but are really using another.

Noncompliant Code Example

class Foo {
  public int myField;

  public void doSomething() {
    int myField = 0;
    ...
  }
}

See

• CERT, DCL01-C. - Do not reuse variable names in subscopes
• CERT, DCL51-J. - Do not shadow or obscure identifiers in subscopes

Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.

See

• Cognitive Complexity

When @Overrides of synchronized methods are not themselves synchronized, the result can be improper synchronization as callers rely on the thread-safety promised by the parent class.

Noncompliant Code Example

public class Parent {

  synchronized void foo() {
    //...
  }
}

public class Child extends Parent {

 @Override
  public void foo () {  // Noncompliant
    // ...
    super.foo();
  }
}

Compliant Solution

public class Parent {

  synchronized void foo() {
    //...
  }
}

public class Child extends Parent {

  @Override
  synchronized void foo () {
    // ...
    super.foo();
  }
}

See

• CERT, TSM00-J - Do not override thread-safe methods with methods that are not thread-safe

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Noncompliant Code Example

With the default threshold of 3:

public void run() {
  prepare("action1");                              // Noncompliant - "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
}

Compliant Solution

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

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

Exceptions

To prevent generating some false-positives, literals having less than 5 characters are excluded.

ThreadLocal variables are supposed to be garbage collected once the holding thread is no longer alive. Memory leaks can occur when holding threads are re-used which is the case on application servers using pool of threads.

To avoid such problems, it is recommended to always clean up ThreadLocal variables using the remove() method to remove the current thread’s value for the ThreadLocal variable.

In addition, calling set(null) to remove the value might keep the reference to this pointer in the map, which can cause memory leak in some scenarios. Using remove is safer to avoid this issue.

Noncompliant Code Example

public class ThreadLocalUserSession implements UserSession {

  private static final ThreadLocal<UserSession> DELEGATE = new ThreadLocal<>();

  public UserSession get() {
    UserSession session = DELEGATE.get();
    if (session != null) {
      return session;
    }
    throw new UnauthorizedException("User is not authenticated");
  }

  public void set(UserSession session) {
    DELEGATE.set(session);
  }

   public void incorrectCleanup() {
     DELEGATE.set(null); // Noncompliant
   }

  // some other methods without a call to DELEGATE.remove()
}

Compliant Solution

public class ThreadLocalUserSession implements UserSession {

  private static final ThreadLocal<UserSession> DELEGATE = new ThreadLocal<>();

  public UserSession get() {
    UserSession session = DELEGATE.get();
    if (session != null) {
      return session;
    }
    throw new UnauthorizedException("User is not authenticated");
  }

  public void set(UserSession session) {
    DELEGATE.set(session);
  }

  public void unload() {
    DELEGATE.remove(); // Compliant
  }

  // ...
}

Exceptions

Rule will not detect non-private ThreadLocal variables, because remove() can be called from another class.

See

• Understanding Memory Leaks in Java

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?
• ...

Like for System.gc(), there is no reason to manually call runFinalization() to force the call of finalization methods of any objects pending finalization.

An application relying on these unpredictable methods is also unpredictable and therefore broken. The task of running the garbage collector and calling finalize() methods should be left exclusively to the JVM.

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Noncompliant Code Example

With the default threshold of 3:

public void run() {
  prepare("action1");                              // Noncompliant - "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
}

Compliant Solution

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

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

Exceptions

To prevent generating some false-positives, literals having less than 5 characters are excluded.