Quality profiles / java / Sonar way

Checks for long anonymous inner classes.

Instead of copying data between two arrays, use System.arrayCopy method

Finds all places 'assert' is used as an identifier is used.

Object.finalize() is called by the garbage collector on an object when garbage collection determines that there are no more references to the object.

Code should never throw NPE under normal circumstances. A catch block may hide the original error, causing other more subtle errors in its wake.

This is dangerous because it casts too wide a net; it can catch things like OutOfMemoryError.

Here are the main reasons why commented code is a code smell :

• It always raises more questions than it gives answers
• Everybody will forget very quickly how relevant the commented code is
• This is distraction when going down the code as it stops the flow of eyes
• It is a bad SCM engine : Subversion, CVS and Git are really more trustworthy !
• The simple fact of understanding why code was commented out in the first place can take a lot of time

When several packages are involved in a cycle (package A > package B > package C > package A where ">" means "depends upon"), that means that those packages are highly coupled and that there is no way to reuse/extract one of those packages without importing all the other packages. Such cycle could quickly increase the effort required to maintain an application and to embrace business change. Sonar not only detect cycles between packages but also determines what is the minimum effort to break those cycles. This rule log a violation on each source file having an outgoing dependency to be but in order to break a cycle.

One might assume that new BigDecimal(.1) is exactly equal to .1, but it is actually equal to .1000000000000000055511151231257827021181583404541015625. This is so because .1 cannot be represented exactly as a double (or, for that matter, as a binary fraction of any finite length). Thus, the long value that is being passed in to the constructor is not exactly equal to .1, appearances notwithstanding. The (String) constructor, on the other hand, is perfectly predictable: 'new BigDecimal(.1)' is exactly equal to .1, as one would expect. Therefore, it is generally recommended that the (String) constructor be used in preference to this one.

Code containing duplicate String literals can usually be improved by declaring the String as a constant field. Example :

public class Foo {
 private void bar() {
    buz("Howdy");
    buz("Howdy");
    buz("Howdy");
    buz("Howdy");
 }
 private void buz(String x) {}
}

Finds all places 'enum' is used as an identifier is used.

Each caught exception type should be handled in its own catch clause.

Avoid printStackTrace(); use a logger call instead.

Catch blocks that merely rethrow a caught exception only add to code size and runtime complexity.

Avoid throwing a NullPointerException - it's confusing because most people will assume that the virtual machine threw it. Consider using an IllegalArgumentException instead; this will be clearly seen as a programmer-initiated exception.

Avoid throwing certain exception types. Rather than throw a raw RuntimeException, Throwable, Exception, or Error, use a subclassed exception or error instead.

Don't create instances of already existing BigInteger (BigInteger.ZERO, BigInteger.ONE) and for 1.5 on, BigInteger.TEN and BigDecimal (BigDecimal.ZERO, BigDecimal.ONE, BigDecimal.TEN)

Restricts nested boolean operators (&&, || and ^) to a specified depth (default = 3).

Avoid instantiating Boolean objects; you can reference Boolean.TRUE, Boolean.FALSE, or call Boolean.valueOf() instead.

The null check is broken since it will throw a Nullpointer itself. The reason is that a method is called on the object when it is null. It is likely that you used || instead of && or vice versa.

if you need to get an array of a class from your Collection, you should pass an array of the desidered class as the parameter of the toArray method. Otherwise you will get a ClassCastException.

Checks that class parameter names conform to the specified format

The following code snippet illustrates this rule for format "^[A-Z]$":

class Something<type> { // Non-compliant
}

class Something<T> { // Compliant
}

The method clone() should only be implemented if the class implements the Cloneable interface with the exception of a final method that only throws CloneNotSupportedException. This version uses PMD's type resolution facilities, and can detect if the class implements or extends a Cloneable class

The method clone() should throw a CloneNotSupportedException.

Ensure that resources (like Connection, Statement, and ResultSet objects) are always closed after use. It does this by looking for code patterned like :

Connection c = openConnection();
try {
  // do stuff, and maybe catch something
} finally {
  c.close();
}

Sometimes two 'if' statements can be consolidated by separating their conditions with a boolean short-circuit operator.

Use equals() to compare object references; avoid comparing them with ==.

Checks that constant names conform to the specified format

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

Checks cyclomatic complexity of methods against a specified limit. The complexity is measured by the number of if, while, do, for, ?:, catch, switch, case statements, and operators && and || (plus one) in the body of a constructor, method, static initializer, or instance initializer. It is a measure of the minimum number of possible paths through the source and therefore the number of required tests. Generally 1-4 is considered good, 5-7 ok, 8-10 consider re-factoring, and 11+ re-factor now !

Check that the default is after all the cases in a switch statement.

Avoid importing anything from the package 'java.lang'. These classes are automatically imported (JLS 7.5.3).

Avoid importing anything from the 'sun.*' packages. These packages are not portable and are likely to change.

Detect the double-checked locking idiom, a technique that tries to avoid synchronization overhead but is incorrect because of subtle artifacts of the java memory model.

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

Avoid empty finally blocks - these can be deleted.

Empty If Statement finds instances where a condition is checked but nothing is done about it.

Detects empty statements (standalone ';').

An empty static initializer was found.

Avoid empty switch statements.

Avoid empty synchronized blocks - they're useless.

Avoid empty try blocks - what's the point?

Empty While Statement finds all instances where a while statement does nothing. If it is a timing loop, then you should use Thread.sleep() for it; if it's a while loop that does a lot in the exit expression, rewrite it to make it clearer.

Checks that classes that override equals() also override hashCode().

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

Using Exceptions as flow control leads to GOTOish code and obscures true exceptions when debugging.

Checks that class which has only private constructors is declared as final.

If a final field is assigned to a compile-time constant, it could be made static, thus saving overhead in each object at runtime.

If the finalize() is implemented, its last action should be to call super.finalize.

Methods named finalize() should not have parameters. It is confusing and probably a bug to overload finalize(). It will not be called by the VM.

Avoid using 'for' statements without using curly braces, like for (int i=0; i<42;i++) foo();

Checks that a local variable or a parameter does not shadow a field that is defined in the same class.

Make sure that utility classes (classes that contain only static methods) do not have a public constructor.

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

Avoid using if..else statements without using curly braces.

Avoid using if statements without using curly braces.

Throwing java.lang.Error or java.lang.RuntimeException is almost never acceptable.

Avoid concatenating non literals in a StringBuffer constructor or append().

Checks for assignments in subexpressions, such as in String s = Integer.toString(i = 2);.

Avoid instantiating an object just to call getClass() on it; use the .class public member instead. Example : replace Class c = new String().getClass(); with Class c = String.class;

In JDK 1.5, calling new Integer() causes memory allocation. Integer.valueOf() is more memory friendly.

Checks that local final variable names, including catch parameters, conform to the specified format

Checks that local, non-final variable names conform to the specified format

Avoid using implementation types (i.e., HashSet); use the interface (i.e, Set) instead

Checks for magic numbers.

Checks that name of non-static fields conform to the specified format

Checks that method names conform to the specified format

Checks that method type parameter names conform to the specified format

The following code snippet illustrates this rule for format "^[A-Z]$":

public <type> boolean containsAll(Collection<type> c) { // Non-compliant
  return null;
}

public <T> boolean containsAll(Collection<T> c) { // Compliant
}

A class that has private constructors and does not have any static methods or fields cannot be used.

Checks that the order of modifiers conforms to the suggestions in the Java Language specification, sections 8.1.1, 8.3.1 and 8.4.3. The correct order is : public, protected, private, abstract, static, final, transient, volatile, synchronized, native, strictfp.

Avoid using dollar signs in variable/method/class/interface names.

Class names should always begin with an upper case character.

Non-constructor methods should not have the same name as the enclosing class. Example :

public class MyClass {
  // this is bad because it is a method
  public void MyClass() {}
  // this is OK because it is a constructor
  public MyClass() {}
}

A field name is all in uppercase characters, which in Sun's Java naming conventions indicate a constant. However, the field is not final. Example :

public class Foo {
  // this is bad, since someone could accidentally
  // do PI = 2.71828; which is actualy e
  // final double PI = 3.16; is ok
  double PI = 3.16;
}

The method name and parameter number are suspiciously close to equals(Object), which may mean you are intending to override the equals(Object) method. Example :

public class Foo {
  public int equals(Object o) {
  // oops, this probably was supposed to be boolean equals
  }
  public boolean equals(String s) {
  // oops, this probably was supposed to be equals(Object)
  }
}

The method name and return type are suspiciously close to hashCode(), which may mean you are intending to override the hashCode() method. Example :

public class Foo {
  public int hashcode() {
  // oops, this probably was supposed to be hashCode
  }
}

This rule uses the NCSS (Non Commenting Source Statements) algorithm to determine the number of lines of code for a given method. NCSS ignores comments, and counts actual statements. Using this algorithm, lines of code that are split are counted as one.

This rule uses the NCSS (Non Commenting Source Statements) algorithm to determine the number of lines of code for a given type. NCSS ignores comments, and counts actual statements. Using this algorithm, lines of code that are split are counted as one.

Checks that package names conform to the specified format. The default value of format has been chosen to match the requirements in the Java Language specification and the Sun coding conventions. However both underscores and uppercase letters are rather uncommon, so most configurations should probably assign value ^[a-z]+(\.[a-z][a-z0-9]*)*$ to format

Disallow assignment of parameters.

Checks that parameter names conform to the specified format

Throwing a new exception from a catch block without passing the original exception into the new Exception will cause the true stack trace to be lost, and can make it difficult to debug effectively.

Checks for redundant modifiers in interface and annotation definitions.

Checks for redundant exceptions declared in throws clause such as duplicates, unchecked exceptions or subclasses of another declared exception.

Consider replacing this Enumeration with the newer java.util.Iterator

Consider replacing this Hashtable with the newer java.util.Map

Consider replacing Vector usages with the newer java.util.ArrayList if expensive threadsafe operation is not required.

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

It is unclear which exceptions that can be thrown from the methods. It might be difficult to document and understand the vague interfaces. Use either a class derived from RuntimeException or a checked exception.

Checks for overly complicated boolean expressions.

Checks for overly complicated boolean return statements.

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.

Checks that static, non-final fields conform to the specified format

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

Avoid instantiating String objects; this is usually unnecessary.

Checks that string literals are not used with == or !=.

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

System.(out|err).print is used, consider using a logger.

The check to ensure that requires that comments be the only thing on a line. For the case of // comments that means that the only thing that should precede it is whitespace. It doesn't check comments if they do not end line, i.e. it accept the following: Thread.sleep( 10 <some comment here> ); Format property is intended to deal with the "} // while" example.

Rationale: Steve McConnel in "Code Complete" suggests that endline comments are a bad practice. An end line comment would be one that is on the same line as actual code. For example:

  
    a = b + c; // Some insightful comment
    d = e / f; // Another comment for this line
  

Quoting "Code Complete" for the justfication:

• "The comments have to be aligned so that they do not interfere with the visual structure of the code. If you don't align them neatly, they'll make your listing look like it's been through a washing machine."
• "Endline comments tend to be hard to format...It takes time to align them. Such time is not spent learning more about the code; it's dedicated solely to the tedious task of pressing the spacebar or tab key."
• "Endline comments are also hard to maintain. If the code on any line containing an endline comment grows, it bumps the comment farther out, and all the other endline comments will have to bumped out to match. Styles that are hard to maintain aren't maintained...."
• "Endline comments also tend to be cryptic. The right side of the line doesn't offer much room and the desire to keep the comment on one line means the comment must be short. Work then goes into making the line as short as possible instead of as clear as possible. The comment usually ends up as cryptic as possible...."
• "A systemic problem with endline comments is that it's hard to write a meaningful comment for one line of code. Most endline comments just repeat the line of code, which hurts more than it helps."

His comments on being hard to maintain when the size of the line changes are even more important in the age of automated refactorings.

Do not use if statements that are always true or always false.

Using equalsIgnoreCase() is faster than using toUpperCase/toLowerCase().equals()

Avoid unnecessarily creating local variables

Avoid passing parameters to methods or constructors and then not using those parameters.

Checks for unused import statements.

Detects when a local variable is declared and/or assigned, but not used.

Fields in interfaces are automatically public static final, and methods are public abstract. Classes or interfaces nested in an interface are automatically public and static (all nested interfaces are automatically static). For historical reasons, modifiers which are implied by the context are accepted by the compiler, but are superfluous.

After checking an object reference for null, you should invoke equals() on that object rather than passing it to another object's equals() method.

Detects when a private field is declared and/or assigned a value, but not used.

Unused Private Method detects when a private method is declared but is unused. This PMD rule should be switched off and replaced by its equivalent from Squid that is more effective : it generates less false-positives and detects more dead code.

ArrayList is a much better Collection implementation than Vector.

The class java.util.Arrays has a asList method that should be use when you want to create a new List from an array of objects. It is faster than executing a loop to cpy all the elements of the array one by one

To make sure the full stacktrace is printed out, use the logging statement with 2 arguments: a String and a Throwable.

Use String.indexOf(char) when checking for the index of a single character; it executes faster.

Use StringBuffer.length() to determine StringBuffer length rather than using StringBuffer.toString().equals() or StringBuffer.toString().length() ==.

An operation on an Immutable object (BigDecimal or BigInteger) won't change the object itself. The result of the operation is a new object. Therefore, ignoring the operation result is an error.

The overriding method merely calls the same method defined in a superclass

No need to call String.valueOf to append to a string; just use the valueOf() argument directly.

Checks visibility of class members. Only static final members may be public; other class members must be private unless property protectedAllowed or packageAllowed is set.

Avoid using 'while' statements without using curly braces.