This code generates a random signed integer and then computes the remainder of that value modulo another value. Since the random number can be negative, the result of the remainder operation can also be negative. Be sure this is intended, and strongly consider using the Random.nextInt(int) method instead.

This code computes a hashCode, and then computes the remainder of that value modulo another value. Since the hashCode can be negative, the result of the remainder operation can also be negative.

Assuming you want to ensure that the result of your computation is nonnegative, you may need to change your code. If you know the divisor is a power of 2, you can use a bitwise and operator instead (i.e., instead of using x.hashCode()%n, use x.hashCode()&(n-1). This is probably faster than computing the remainder as well. If you don't know that the divisor is a power of 2, take the absolute value of the result of the remainder operation (i.e., use Math.abs(x.hashCode()%n)

This code performs integer multiply and then converts the result to a long, as in:

 
	long convertDaysToMilliseconds(int days) { return 1000*3600*24*days; } 

 
	long convertDaysToMilliseconds(int days) { return 1000L*3600*24*days; } 

 
	static final long MILLISECONDS_PER_DAY = 24L*3600*1000;
	long convertDaysToMilliseconds(int days) { return days * MILLISECONDS_PER_DAY; } 

This method contains a self assignment of a local variable; e.g.

  public void foo() {
    int x = 3;
    x = x;
  }

Such assignments are useless, and may indicate a logic error or typo.

This operation compares two floating point values for equality. Because floating point calculations may involve rounding, calculated float and double values may not be accurate. For values that must be precise, such as monetary values, consider using a fixed-precision type such as BigDecimal. For values that need not be precise, consider comparing for equality within some range, for example: if ( Math.abs(x - y) < .0000001 ). See the Java Language Specification, section 4.2.4.

A Thread object is passed as a parameter to a method where a Runnable is expected. This is rather unusual, and may indicate a logic error or cause unexpected behavior.

The field is marked as transient, but the class isn't Serializable, so marking it as transient has absolutely no effect. This may be leftover marking from a previous version of the code in which the class was transient, or it may indicate a misunderstanding of how serialization works.

This cast is unchecked, and not all instances of the type casted from can be cast to the type it is being cast to. Ensure that your program logic ensures that this cast will not fail.

The code performs an unsigned right shift, whose result is then cast to a short or byte, which discards the upper bits of the result. Since the upper bits are discarded, there may be no difference between a signed and unsigned right shift (depending upon the size of the shift).

This class doesn't do any of the patterns we recognize for checking that the type of the argument is compatible with the type of the this object. There might not be anything wrong with this code, but it is worth reviewing.

This method contains a useless control flow statement, where control flow continues onto the same place regardless of whether or not the branch is taken. For example, this is caused by having an empty statement block for an if statement:

    if (argv.length == 0) {
	// TODO: handle this case
	}

This is an integer bit operation (and, or, or exclusive or) that doesn't do any useful work (e.g., v & 0xffffffff).

There is an integer comparison that always returns the same value (e.g., x <= Integer.MAX_VALUE).

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

The entrySet() method is allowed to return a view of the underlying Map in which an Iterator and Map.Entry. This clever idea was used in several Map implementations, but introduces the possibility of nasty coding mistakes. If a map m returns such an iterator for an entrySet, then c.addAll(m.entrySet()) will go badly wrong. All of the Map implementations in OpenJDK 1.7 have been rewritten to avoid this, you should to.

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
}