Wednesday, September 28, 2011

PMD - Basic Checks

Tried to Google to figure out what each PMD rule means with examples, but did not find any, possibly because it is embedded in the PMD plugin itself. So I thought it may be worthwhile putting these rules across in a document form. So here is the first one of Basic Checks

Basic Rules

The Basic Ruleset contains a collection of good practices which everyone should follow.

EmptyCatchBlock

Empty Catch Block finds instances where an exception is caught, but nothing is done. In most circumstances, this swallows an exception which should either be acted on or reported. The standard practice should be to throw any Exceptions that can be raised from within the code. The only places where the Exception should be caught are
1.     If you know that the exception can be ignored. But in this scenario we should log the exception indicating that the exception occurred.
2.     At the point where you cannot throw an exception any further. E.g. in a servlet if throw an exception then the user will see some message that will not be understood by her. Instead one should handle the exception at this level and provide the user a relevant message.
In any case we cannot have an empty catch block. At the minimum we should have a log statement.

Example

public void doSomething() {
  try {
    FileInputStream fis = new FileInputStream("/tmp/bugger");
  } catch (IOException ioe) {
      // not good
  }
}

EmptyIfStmt

Empty If Statement finds instances where a condition is checked but nothing is done about it. It would either be a redundant statement or there will be an else block. If there is an else clause negate the if condition and move the else block to the if block.

Example

public class Foo {
void bar(int x) {
  if (x == 0) {
   // empty!
  }
}
}

EmptyWhileStmt

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. An empty while statement will hog the entire CPU time as long as the condition is true. This kind of bug will be difficult to find visually. There should be no reason to have an empty while loop.

Example

public class Foo {
void bar(int a, int b) {
  while (a == b) {
   // empty!
  }
}
}

EmptyTryBlock

Avoid empty try blocks - what's the point? If there is no code in the try block then just remove them.

Example

public class Foo {
public void bar() {
  try {
  } catch (Exception e) {
    e.printStackTrace();
  }
}
}

EmptyFinallyBlock

Avoid empty finally blocks - these can be deleted. “Finally” blocks should have code which “must” be executed before exiting the method. There is no point in putting a “finally” block with nothing in it.

Example

public class Foo {
public void bar() {
  try {
    int x=2;
   } finally {
    // empty!
   }
}
}

EmptySwitchStatements

There is no reason to have empty switch statements.

Example

public class Foo {
public void bar() {
  int x = 2;
  switch (x) {
   // once there was code here
   // but it's been commented out or something
  }
}
}

JumbledIncrementer

Avoid jumbled loop incrementers - it's usually a mistake, and it's confusing even if it's what's intended. Note that in both the loops the variable “I” is being incremented. This is in all likelihood a mistake and if it is not then it is very confusing. Use a different construct to achieve the same result if it is really required.

Example

public class JumbledIncrementerRule1 {
  public void foo() {
   for (int i = 0; i < 10; i++) {
    for (int k = 0; k < 20; i++) {
     System.out.println("Hello");
    }
   }
  }
}

ForLoopShouldBeWhileLoop

Some for loops can be simplified to while loops - this makes them more concise. In scenarios where there are no variables to be incremented or decremented in each execution of the loop use a while loop instead of a “for” loop.

Example

public class Foo {
void bar() {
  for (;true;) true; // No Init or Update part, may as well be: while (true)
}
}

UnnecessaryConversionTemporary

Avoid unnecessary temporaries when converting primitives to Strings. Unnecessary objects put strain on the Garbage Collection by generation of redundant objects.

Example

public String convert(int x) {
  // this wastes an object
  String foo = new Integer(x).toString();
  // this is better
  return Integer.toString(x);
}

OverrideBothEqualsAndHashcode

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. This is required because whenever equals between two objects returns true then the hashCode values returned by the two objects should also be same. If one has the requirement to override the standard equals method then it is very likely that one will need to override the hashCode method too. See http://www.technofundo.com/tech/java/equalhash.html for details of how equals and hashCode are related and how they are to be written.

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
  }
}

DoubleCheckedLocking

Partially created objects can be returned by the Double Checked Locking pattern when used in Java. An optimizing JRE may assign a reference to the baz variable before it creates the object the reference is intended to point to. For more details see http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html.

Example

public class Foo {
  static Object baz;
 Object bar() {
    if(baz == null) { //baz may be non-null yet not fully created
      synchronized(this){
        if(baz == null){
          baz = new Object();
        }
      }
    }
    return baz;
  }
}
One solution to avoid this problem is as follows:
public class Foo {
  static Object baz = new Object();
  Object bar() {
    return baz;
  }
}
i.e. do not use lazy initialization but eager static initialization. The other option is to make the whole method synchronized.
public class Foo {
  static Object baz;
  synchronized Object bar() {
    if(baz == null) { //baz may be non-null yet not fully created
          baz = new Object();
    }
    return baz;
  }
}

ReturnFromFinallyBlock

Avoid returning from a finally block - this can discard exceptions. Finally block is executed irrespective of the occurrence of an exception. We should not be returning anything from the finally block. We should return values only for places where we are sure no exception has occurred in the method. In the finally block we should have only code that does the clean up after the method execution.

Example

public class Bar {
public String foo() {
  try {
   throw new Exception( "My Exception" );
  } catch (Exception e) {
   throw e;
  } finally {
   return "A. O. K."; // Very bad.
  }
}
}

EmptySynchronizedBlock

Avoid empty synchronized blocks - they're useless and they cause a performance problem as they go about trying to get and release unnecessary locks.

Example

public class Foo {
public void bar() {
  synchronized (this) {
   // empty!
  }
}
}

UnnecessaryReturn

Avoid unnecessary return statements. It is obvious that the method is returning from these points and there is no need to specifically indicate them.

Example

public class Foo {
public void bar() {
  int x = 42;
  return;
}
}

EmptyStaticInitializer

An empty static initializer was found. Static initialize blocks are executed when the object is loaded for the first time by the class loader. It makes no sense to have an empty static block and we should remove it.

Example

public class Foo {
static {
  // empty
}
}

UnconditionalIfStatement

Do not use "if" statements that are always true or always false. These can cause performance issues as these statements will be evaluated unnecessarily. It will also be confusing for the other developers who look at this code.

Example

public class Foo {
public void close() {
  if (true) {
       // ...
   }
}
}

EmptyStatementNotInLoop

An empty statement (aka a semicolon by itself) that is not used as the sole body of a for loop or while loop is probably a bug. It could also be a double semicolon, which is useless and should be removed.

Example

public class MyClass {
   public void doit() {
      // this is probably not what you meant to do
      ;
      // the extra semicolon here this is not necessary
      System.out.println("look at the extra semicolon");;
   }
}

BooleanInstantiation

Avoid instantiating Boolean objects; you can reference Boolean.TRUE, Boolean.FALSE, or call Boolean.valueOf() instead. If we create new Boolean(“true”) it creates an object. Instead if we use Boolean.TRUE and Boolean.FALSE it reuses the same Boolean objects and whole JVM will have only two Boolean class instances. This will reduce the load on Garbage collection. We should use the same strategy for other objects as well. E.g. instead of creating new Integer(0) every time we should defined a constant public static final ZERO = new Integer(0) and use this wherever we wish to initialize an Integer object to Zero.

Example

public class Foo {
Boolean bar = new Boolean("true"); // just do a Boolean bar = Boolean.TRUE;
Boolean buz = Boolean.valueOf(false); // just do a Boolean buz = Boolean.FALSE;
}

UnnecessaryFinalModifier

When a class has the final modifier, all the methods are automatically final.

Example

public final class Foo {
    // This final modifier is not necessary, since the class is final
    // and thus, all methods are final
    private final void foo() {
    }
}

CollapsibleIfStatements

Sometimes two 'if' statements can be consolidated by separating their conditions with a boolean short-circuit operator. In this below scenario we could think of if (x && y) instead of the check below. This will make the code more readable.

Example

public class Foo {
void bar() {
  if (x) {
   if (y) {
    // do stuff
   }
  }
}
}

UselessOverridingMethod

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

Example

public void foo(String bar) {
    super.foo(bar);      //Why bother overriding?
}
public String foo() {
    return super.foo();  //Why bother overriding?
}
@Id
public Long getId() {
    return super.getId();  //OK if 'ignoreAnnotations' is false, which is the default behavior
}

ClassCastExceptionWithToArray

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

Example

import java.util.ArrayList;
import java.util.Collection;
 
public class Test {
 
    public static void main(String[] args) {
        Collection c=new ArrayList();
        Integer obj=new Integer(1);
        c.add(obj);
 
        // this would trigger the rule (and throw a ClassCastException
if executed)
        Integer[] a=(Integer [])c.toArray();
 
        // this wouldn't trigger the rule
        Integer[] b=(Integer [])c.toArray(new Integer[c.size()]);
    }
}

AvoidDecimalLiteralsInBigDecimalConstructor

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.

Example

import java.math.BigDecimal;
public class Test {
 
    public static void main(String[] args) {
      // this would trigger the rule
     BigDecimal bd=new BigDecimal(1.123);
      // this wouldn't trigger the rule
     BigDecimal bd=new BigDecimal("1.123");
      // this wouldn't trigger the rule
     BigDecimal bd=new BigDecimal(12);
    }
}

UselessOperationOnImmutable

An operation on an Immutable object (String, BigDecimal or BigInteger) won't change the object itself. The result of the operation is a new object which needs to be assigned to some other variable so that the result of the operation is available. Otherwise, ignoring the operation result is an error or the operation is redundant.

Example

import java.math.*;
class Test {
void method1() {
  BigDecimal bd=new BigDecimal(10);
  bd.add(new BigDecimal(5)); // this will trigger the rule
}
void method2() {
  BigDecimal bd=new BigDecimal(10);
  bd = bd.add(new BigDecimal(5)); // this won't trigger the rule
}
}

MisplacedNullCheck

The null check here is misplaced. if the variable is null you'll get a NullPointerException. Either the check is useless (the variable will never be "null") or it's incorrect. The null check should be before any other check to avoid NullPointerExceptions.

Example

public class Foo {
void bar() {
  if (a.equals(baz) && a != null) {}
}
}public class Foo {
void bar() {
  if (a.equals(baz) || a == null) {}
}
}

UnusedNullCheckInEquals

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

Example

public class Test {
 
public String method1() { return "ok";}
public String method2() { return null;}
 
public void method(String a) {
    String b;
    /*
      I don't know it method1() can be "null"
      but I know "a" is not null..
      I'd better write a.equals(method1())
    */
    if (a!=null && method1().equals(a)) { // will trigger the rule
        //whatever
    }
    if (method1().equals(a) && a != null) { //won't trigger the rule
        //whatever
    }
    if (a!=null && method1().equals(b)) { // won't trigger the rule
        //whatever
    }
    if (a!=null && "LITERAL".equals(a)) { // won't trigger the rule
        //whatever
    }
    if (a!=null && !a.equals("go")) { // won't trigger the rule
        a=method2();
        if (method1().equals(a)) {
            //whatever
        }
    }
}

AvoidThreadGroup

Avoid using ThreadGroup; although it is intended to be used in a threaded environment it contains methods that are not thread safe.

Example

public class Bar {
     void buz() {
      ThreadGroup tg = new ThreadGroup("My threadgroup") ;
      tg = new ThreadGroup(tg, "my thread group");
      tg = Thread.currentThread().getThreadGroup();
      tg = System.getSecurityManager().getThreadGroup();
     }
    }

BrokenNullCheck

The null check is broken since it will throw a NullPointerException itself. It is likely that you used || instead of && or vice versa.

Example

class Foo {
String bar(String string) {
  // should be &&
  //If string is null then the second check will throw a NullPointerException
  if (string!=null || !string.equals(""))
    return string;
  // should be ||
  //If string is null then the second check will throw a NullPointerException
  if (string==null && string.equals(""))
    return string;
}
}

BigIntegerInstantiation

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). This is again to avoid creation of unnecessary objects.

Example

public class Test {
 
public static void main(String[] args) {
   BigInteger bi=new BigInteger(1);
   BigInteger bi2=new BigInteger("0");
   BigInteger bi3=new BigInteger(0.0);
   BigInteger bi4;
   bi4=new BigInteger(0);
}
}

AvoidUsingOctalValues

Integer literals should not start with zero. Zero means that the rest of literal will be interpreted as an octal value. This is confusing and unclear.

Example

public class Foo {
                 int i = 012; // set i with 10 not 12
                 int j = 010; // set j with 8 not 10
                 k = i * j; // set k with 80 not 120
               }

AvoidUsingHardCodedIP

An application with hard coded IP may become impossible to deploy in some case. It never hurts to externalize IP adresses. IP Addresses should always be parameterized as these will change based on the environment in which the code is executed.

Example

public class Foo {
          String ip = "127.0.0.1"; // This is a really bad idea !
        }

CheckResultSet

Always check the return of one of the navigation method (next,previous,first,last) of a ResultSet. Indeed, if the value return is 'false', the developer should deal with it!

Example

// This is NOT appropriate !
            Statement stat = conn.createStatement();
            ResultSet rst = stat.executeQuery("SELECT name FROM person");
            rst.next(); // what if it returns a 'false' ?
            String firstName = rst.getString(1);
 
            // This is appropriate...
            Statement stat = conn.createStatement();
            ResultSet rst = stat.executeQuery("SELECT name FROM person");
            if (rst.next())
            {
                String firstName = rst.getString(1);
            }
            else
           {
                // here you deal with the error ( at least log it)
            }

AvoidMultipleUnaryOperators

Using multiple unary operators may be a bug, and/or is confusing. Check the usage is not a bug, or consider simplifying the expression.

Example

// These are typo bugs, or at best needlessly complex and confusing:
            int i = - -1;
            int j = + - +1;
            int z = ~~2;
            boolean b = !!true;
            boolean c = !!!true;
 
            // These are better:
            int i = 1;
            int j = -1;
            int z = 2;
            boolean b = true;
            boolean c = false;
 
            // And these just make your brain hurt:
            int i = ~-2;
            int j = -~7;

EmptyInitializer

An empty initializer was found.

Example

public class Foo {
 
   static {} // Why ?
 
   {} // Again, why ?
 
}
Posted by at
Labels: , ,

No comments:

Post a Comment

Subscribe to: Post Comments (Atom)