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.

This rule is defined by the following XPath expression:

    
    //TryStatement
    [@Catch='true']
    [FormalParameter/Type/ReferenceType/ClassOrInterfaceType
     [@Image != 'InterruptedException' and @Image != 'CloneNotSupportedException']
    ]
    /Block[position() > 1]
    [count(*) = 0]
    [../@Finally='false' or following-sibling::Block]
 
              

Here's an example of code that would trigger this rule:

			
  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.

This rule is defined by the following XPath expression:

                  //IfStatement/Statement/Block[count(*) = 0]
              

Here's an example of code that would trigger this rule:

			
    if (foo == 0) {
       // why bother checking up on foo?
    }
       
		

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.

This rule is defined by the following XPath expression:

                  
                  //WhileStatement/Statement[./Block[count(*) = 0]  or ./EmptyStatement]
                  
              

Here's an example of code that would trigger this rule:

			
  while (a == b) {
    // not good
  }
       
		

EmptyTryBlock

Avoid empty try blocks - what's the point?

This rule is defined by the following XPath expression:

                  //TryStatement/Block[1][count(*) = 0]
              

Here's an example of code that would trigger this rule:

			
  // this is bad
  public void bar() {
      try {
      } catch (Exception e) {
          e.printStackTrace();
      }
  }
      
		

EmptyFinallyBlock

Avoid empty finally blocks - these can be deleted.

This rule is defined by the following XPath expression:

                  //TryStatement[@Finally='true']/Block[position() = last()]
      [count(*) = 0]
              

Here's an example of code that would trigger this rule:

			
  // this is bad
  public void bar() {
      try {
          int x=2;
      } finally {
      }
  }
      
		

EmptySwitchStatements

Avoid empty switch statements.

This rule is defined by the following XPath expression:

                  //SwitchStatement[count(*) = 1]
              

Here's an example of code that would trigger this rule:

			
  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.

This rule is defined by the following XPath expression:

 //ForStatement
 [
  ForUpdate/StatementExpressionList/StatementExpression/PostfixExpression/PrimaryExpression/PrimaryPrefix/Name/@Image
  =
  ancestor::ForStatement/ForInit//VariableDeclaratorId/@Image
 ]
             

Here's an example of code that would trigger this rule:

			
 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.

This rule is defined by the following XPath expression:

                
 //ForStatement
  [count(*) > 1]
  [not(ForInit)]
  [not(ForUpdate)]
  [not(Type and Expression and Statement)]
 
            

Here's an example of code that would trigger this rule:

			
  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

This rule is defined by the following Java class: net.sourceforge.pmd.rules.UnnecessaryConversionTemporary

Here's an example of code that would trigger this rule:

			
  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 rule is defined by the following XPath expression:


//ClassOrInterfaceDeclaration[@Interface='false']//MethodDeclarator
[(@Image = 'equals' and count(FormalParameters/*) = 1
and not(//MethodDeclarator[count(FormalParameters/*) = 0][@Image = 'hashCode']))
or
(@Image='hashCode' and count(FormalParameters/*) = 0
and
not
(//MethodDeclarator
 [count(
   FormalParameters//Type/ReferenceType/ClassOrInterfaceType
    [@Image = 'Object' or @Image = 'java.lang.Object']) = 1]
    [@Image = 'equals']))]
 
              

Here's an example of code that would trigger this rule:

			
  // 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.

This rule is defined by the following Java class: net.sourceforge.pmd.rules.DoubleCheckedLocking

Here's an example of code that would trigger this rule:

			
  public class Foo {
      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;
      }
  }
      
		

ReturnFromFinallyBlock

Avoid returning from a finally block - this can discard exceptions.

This rule is defined by the following XPath expression:

                  //TryStatement[@Finally='true']/Block[position() = last()]//ReturnStatement
              

Here's an example of code that would trigger this rule:

			
  public class Bar {
 public String bugga() {
  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.

This rule is defined by the following XPath expression:

                  //SynchronizedStatement/Block[1][count(*) = 0]
              

Here's an example of code that would trigger this rule:

			
  // this is bad
  public void bar() {
      synchronized (this) {}
  }
      
		

UnnecessaryReturn

Avoid unnecessary return statements

This rule is defined by the following XPath expression:

 //ReturnStatement
 [parent::Statement
  [parent::BlockStatement
   [parent::Block
    [parent::MethodDeclaration/ResultType[@Void='true']
    ]
   ]
  ] 
              

Here's an example of code that would trigger this rule:

			
  // this is bad
  public void bar() {
      int x = 42;
      return;
  }
      
		

EmptyStaticInitializer

An empty static initializer was found.

This rule is defined by the following XPath expression:

//ClassOrInterfaceBodyDeclaration/Initializer[@Static='true']/Block[count(*)=0]
                 

Here's an example of code that would trigger this rule:

			
   public class Foo {
  // why are there no statements in this static block?
  static {}
 }
       
		

UnconditionalIfStatement

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

This rule is defined by the following XPath expression:

 //IfStatement/Expression
 [count(PrimaryExpression)=1]
 /PrimaryExpression/PrimaryPrefix/Literal/BooleanLiteral
                

Here's an example of code that would trigger this rule:

			
  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.

This rule is defined by the following XPath expression:


//Statement/EmptyStatement
 [not(
       ../../../ForStatement
       or ../../../WhileStatement
       or ../../../BlockStatement/ClassOrInterfaceDeclaration
       or ../../../../../../ForStatement/Statement[1]
        /Block[1]/BlockStatement[1]/Statement/EmptyStatement
       or ../../../../../../WhileStatement/Statement[1]
        /Block[1]/BlockStatement[1]/Statement/EmptyStatement)
 ]

                

Here's an example of code that would trigger this rule:

			
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, instead use Boolean.TRUE or Boolean.FALSE.

This rule is defined by the following XPath expression:


//PrimaryExpression
[
PrimaryPrefix/AllocationExpression[not (ArrayDimsAndInits) and (ClassOrInterfaceType/@Image='Boolean' or ClassOrInterfaceType/@Image='java.lang.Boolean')]
or
(
PrimaryPrefix/Name[@Image='Boolean.valueOf']
and
PrimarySuffix/Arguments//BooleanLiteral
)
]

              

Here's an example of code that would trigger this rule:

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

UnnecessaryFinalModifier

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

This rule is defined by the following XPath expression:

    
//ClassOrInterfaceDeclaration[@Final='true' and @Interface='false']
/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration[@Final='true']
    
              

Here's an example of code that would trigger this rule:

			

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.

This rule is defined by the following XPath expression:

                
//IfStatement[@Else='false']/Statement
 /IfStatement[@Else='false']
 |
//IfStatement[@Else='false']/Statement
 /Block[count(BlockStatement)=1]/BlockStatement
  /Statement/IfStatement[@Else='false']
            

Here's an example of code that would trigger this rule:

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