The Basic Ruleset contains a collection of good practices which everyone should follow.
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 } }
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? }
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 }
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(); } }
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 { } }
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 } } }
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"); } } } }
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) } }
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); }
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 } }
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; } }
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. } } }
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) {} }
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; }
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 {} }
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) { // ... } } }
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");; } }
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; }
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() { } }
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 } } } }