The Design Ruleset contains a collection of rules that find questionable designs.
If you have a class that has nothing but static methods, consider making it a Singleton. Note that this doesn't apply to abstract classes, since their subclasses may well include non-static methods. Also, if you want this class to be a Singleton, remember to add a private constructor to prevent instantiation.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.design.UseSingleton
Here's an example of code that would trigger this rule:
public class MaybeASingleton {
public static void foo() {
// etc
}
public static void bar() {
// etc
}
}
Avoid unnecessary if..then..else statements when returning a boolean
This rule is defined by the following Java class: net.sourceforge.pmd.rules.SimplifyBooleanReturns
Here's an example of code that would trigger this rule:
public class Foo {
private int bar =2;
public boolean isBarEqualsTo(int x) {
// this bit of code
if (bar == x) {
return true;
} else {
return false;
}
// can be replaced with a simple
// return bar == x;
}
}
Avoid unnecessary comparisons in boolean expressions - this makes simple code seem complicated.
This rule is defined by the following XPath expression:
//EqualityExpression/PrimaryExpression/PrimaryPrefix/Literal/BooleanLiteral
Here's an example of code that would trigger this rule:
public class Bar {
// can be simplified to
// bar = isFoo();
private boolean bar = (isFoo() == true);
public isFoo() { return false;}
}
Switch statements should have a default label.
This rule is defined by the following XPath expression:
//SwitchStatement[not(SwitchLabel[count(*) = 0])]
Here's an example of code that would trigger this rule:
public class Foo {
public void bar() {
int x = 2;
switch (x) {
case 2: int j = 8;
}
}
}
Deeply nested if..then statements are hard to read.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.AvoidDeeplyNestedIfStmtsRule
Here's an example of code that would trigger this rule:
public class Foo {
public void bar() {
int x=2;
int y=3;
int z=4;
if (x>y) {
if (y>z) {
if (z==x) {
// this is officially out of control now
}
}
}
}
}
Reassigning values to parameters is a questionable practice. Use a temporary local variable instead.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.AvoidReassigningParameters
Here's an example of code that would trigger this rule:
public class Foo {
private void foo(String bar) {
bar = "something else";
}
}
A high ratio of statements to labels in a switch statement implies that the switch statement is doing too much work. Consider moving the statements either into new methods, or creating subclasses based on the switch variable.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.design.SwitchDensityRule
Here's an example of code that would trigger this rule:
public class Foo {
private int x;
public void bar() {
switch (x) {
case 1: {
System.out.println("I am a fish.");
System.out.println("I am a fish.");
System.out.println("I am a fish.");
System.out.println("I am a fish.");
break;
}
case 2: {
System.out.println("I am a cow.");
System.out.println("I am a cow.");
System.out.println("I am a cow.");
System.out.println("I am a cow.");
break;
}
}
}
}
Calling overridable methods during construction poses a risk of invoking methods on an incompletely constructed object. This situation 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(), there's a problem.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.ConstructorCallsOverridableMethod
Here's an example of code that would trigger this rule:
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();
}
}
Instantiation by way of private constructors from outside of the constructor's class often causes the generation of an accessor. A factory method, or non-privitization of the constructor can eliminate this situation. The generated class file is actually an interface. It gives the accessing class the ability to invoke a new hidden package scope constructor that takes the interface as a supplementary parameter. This turns a private constructor effectively into one with package scope, though not visible to the naked eye.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.AccessorClassGeneration
Here's an example of code that would trigger this rule:
public class OuterClass {
void method(){
InnerClass ic = new InnerClass();//Causes generation of accessor
}
public class InnerClass {
private InnerClass(){
}
}
}
public class OuterClass {
void method(){
InnerClass ic = new InnerClass();//OK, due to public constructor
}
public class InnerClass {
public InnerClass(){
}
}
}
public class OuterClass {
void method(){
InnerClass ic = InnerClass.getInnerClass();//OK
}
public static class InnerClass {
private InnerClass(){
}
private static InnerClass getInnerClass(){
return new InnerClass();
}
}
}
public class OuterClass {
private OuterClass(){
}
public class InnerClass {
void method(){
OuterClass oc = new OuterClass();//Causes generation of accessor
}
}
}
If a final field is assigned to a compile-time constant, it could be made static, thus saving overhead in each object
This rule is defined by the following XPath expression:
//FieldDeclaration
[@Final='true' and @Static='false']
[not (../../../../ClassOrInterfaceDeclaration[@Interface='true'])]
/VariableDeclarator/VariableInitializer/Expression
/PrimaryExpression/PrimaryPrefix/Literal
Here's an example of code that would trigger this rule:
public class Foo {
public final int BAR = 42; // this could be static and save some space
}
Ensure that Connection objects are always closed after use
This rule is defined by the following Java class: net.sourceforge.pmd.rules.CloseConnection
Here's an example of code that would trigger this rule:
public void foo() {
Connection c = pool.getConnection();
try {
// do stuff
} catch (SQLException ex) {
// handle exception
} finally {
// oops, should close the connection using 'close'!
// c.close();
}
}
A nonstatic initializer block will be called any time a constructor is invoked (just prior to invoking the constructor). While this is a valid language construct, it is rarely used and is confusing.
This rule is defined by the following XPath expression:
//ClassOrInterfaceBodyDeclaration/Initializer[@Static='false']
Here's an example of code that would trigger this rule:
public class MyClass {
// this block gets run before any call to a constructor
{
System.out.println("I am about to construct myself");
}
}
The default label in a switch statement should be the last label, by convention. Most programmers will expect the default label (if present) to be the last one.
This rule is defined by the following XPath expression:
//SwitchStatement
[not(SwitchLabel[position() = last()][count(*) = 0])]
[SwitchLabel[count(*) = 0]
Here's an example of code that would trigger this rule:
switch (a)
{
case 1:
// do something
break;
default:
// the default case should be last, by convention
break;
case 2:
break;
}
A non-case label (e.g. a named break/continue label) was present in a switch statement. This legal, but confusing. It is easy to mix up the case labels and the non-case labels.
This rule is defined by the following XPath expression:
//SwitchStatement//BlockStatement/Statement/LabeledStatement
Here's an example of code that would trigger this rule:
public class Foo {
void bar(int a) {
switch (a)
{
case 1:
// do something
break;
mylabel: // this is legal, but confusing!
break;
default:
break;
}
}
}
A call to Collection.toArray can use the Collection's size vs an empty Array of the desired type.
This rule is defined by the following XPath expression:
//PrimaryExpression
[PrimaryPrefix/Name[ends-with(@Image, 'toArray')]]
[
PrimarySuffix/Arguments/ArgumentList/Expression
/PrimaryExpression/PrimaryPrefix/AllocationExpression
/ArrayDimsAndInits/Expression/PrimaryExpression/PrimaryPrefix/Literal[@Image='0']
]
Here's an example of code that would trigger this rule:
class Example {
void bar() {
// A bit inefficient, unlike...
x.toArray(new Foo[0]);
// ..this one, which sizes the destination array, avoiding
// a reflection call in some Collection implementations
x.toArray(new Foo[x.size()]);
}
}
Avoid equality comparisons with Double.NaN - these are likely to be logic errors.
This rule is defined by the following XPath expression:
//EqualityExpression[@Image='==']
/PrimaryExpression/PrimaryPrefix
/Name[@Image='Double.NaN' or @Image='Float.NaN']
Here's an example of code that would trigger this rule:
public class Bar {
int x = (y == Double.NaN);
}
Newbie programmers sometimes get the comparison concepts confused and use equals() to compare to null.
This rule is defined by the following XPath expression:
//PrimaryExpression
[PrimaryPrefix/Name[ends-with(@Image, 'equals')]]
[PrimarySuffix/Arguments/ArgumentList
/Expression/PrimaryExpression/PrimaryPrefix
/Literal/NullLiteral]
Here's an example of code that would trigger this rule:
class Bar {
void foo() {
String x = "foo";
if (x.equals(null)) { // bad!
doSomething();
}
}
}
In an "if" expression with an "else" clause, avoid negation in the test. For example, rephrase: if (x != y) diff(); else same(); as: if (x == y) same(); else diff(); Most "if (x != y)" cases without an "else" are often return cases, so consistent use of this rule makes the code easier to read. Also, this resolves trivial ordering problems, such as "does the error case go first?" or "does the common case go first?".
This rule is defined by the following Java class: net.sourceforge.pmd.rules.design.ConfusingTernary
Here's an example of code that would trigger this rule:
return (x != y) ? diff : same;
Avoid instantiating an object just to call getClass() on it; use the .class public member instead
This rule is defined by the following XPath expression:
//PrimarySuffix
[@Image='getClass']
[parent::PrimaryExpression
[PrimaryPrefix/AllocationExpression]
[count(PrimarySuffix) = 2]
]
Here's an example of code that would trigger this rule:
class Foo {
Class c = new String().getClass();
}
Avoid idempotent operations - they are silly.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.IdempotentOperations
Here's an example of code that would trigger this rule:
public class Foo {
public void bar() {
int x = 2;
x = x;
}
}
Be sure to specify a Locale when creating a new instance of SimpleDateFormat.
This rule is defined by the following XPath expression:
//AllocationExpression
[ClassOrInterfaceType[@Image='SimpleDateFormat']]
[Arguments[@ArgumentCount=1]]
Here's an example of code that would trigger this rule:
public class Foo {
// Should specify Locale.US (or whatever)
private SimpleDateFormat sdf = new SimpleDateFormat("pattern");
}
Identifies private fields whose values never change once they are initialized either in the declaration of the field or by a constructor. This aids in converting existing classes to immutable classes.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.design.ImmutableField
Here's an example of code that would trigger this rule:
public class FinalFields {
private int x;
public FinalFields() {
x = 7;
}
public void foo() {
int a = x + 2;
}
}
Identifies x as being eligible for making final.
When doing a String.toLowerCase()/toUpperCase() call, use a Locale. This avoids problems with certain locales, i.e. Turkish.
This rule is defined by the following XPath expression:
//PrimaryExpression
[PrimaryPrefix/Name
[ends-with(@Image, 'toLowerCase') or ends-with(@Image,
'toUpperCase')]
]
[PrimarySuffix/Arguments[@ArgumentCount=0]]
Here's an example of code that would trigger this rule:
class Foo {
// BAD
if (x.toLowerCase().equals("list"))...
/*
This will not match "LIST" when in Turkish locale
The above could be
if (x.toLowerCase(Locale.US).equals("list")) ...
or simply
if (x.equalsIgnoreCase("list")) ...
*/
// GOOD
String z = a.toLowerCase(Locale.EN);
}
Do not use protected fields in final classes since they cannot be subclassed. Clarify your intent by using private or package access modifiers instead.
This rule is defined by the following XPath expression:
//ClassOrInterfaceDeclaration[@Final='true']//FieldDeclaration[@Protected='true']
Here's an example of code that would trigger this rule:
public final class Bar {
private int x;
protected int y; // <-- Bar cannot be subclassed, so is y really private or package visible???
Bar() {}
}
Identifies a possible unsafe usage of a static field.
This rule is defined by the following Java class: net.sourceforge.pmd.rules.design.AssignmentToNonFinalStatic
Here's an example of code that would trigger this rule:
public class StaticField {
static int x;
public FinalFields(int y) {
x = y;
}
}
Identifies the unasignment to x as possibly unsafe.
A class that has private constructors and does not have any static method cannot be used.
This rule is defined by the following XPath expression:
//ClassOrInterfaceDeclaration[
( count(.//ConstructorDeclaration)>0
and count(.//ConstructorDeclaration) = count(.//ConstructorDeclaration[@Private='true']) )
and
count(.//MethodDeclaration[@Static='true'])=0
]
Here's an example of code that would trigger this rule:
/* This class is unusable, since it cannot be
instantiated (private constructor),
and no static method can be called.
*/
public class Foo {
private Foo() {}
void foo() {
}
}
finalize() is called by the garbage collector on an object when garbage collection determines that there are no more references to the object.
This rule is defined by the following XPath expression:
//Statement/StatementExpression/PrimaryExpression/PrimaryPrefix
[
./Name[
@Image='finalize'
or ends-with(@Image,'.finalize')
]
or @Image='finalize'
]
Here's an example of code that would trigger this rule:
void foo() {
Bar b = new Bar();
b.finalize();
}
Method level synchronization can backfire when new code is added to the method. Block-level synchronization helps to ensure that only the code that needs synchronization gets it.
This rule is defined by the following XPath expression:
//MethodDeclaration[@Synchronized='true']
Here's an example of code that would trigger this rule:
// Try to avoid this
synchronized void foo() {
}
// Prefer this:
void bar() {
synchronized(mutex) {
}
}
A switch statement without an enclosed break statement may be a bug.
This rule is defined by the following XPath expression:
//SwitchStatement[count(.//BreakStatement)=0]
Here's an example of code that would trigger this rule:
switch(status) {
case CANCELLED:
doCancelled();
// uncomment the next line if doNew() and doRemoved() should not be executed when status is cancelled
// break;
case NEW:
doNew();
case REMOVED:
doRemoved();
}
notify() awakens a thread monitoring the object. If more than one thread is monitoring, then only one is chosen. The thread chosen is arbitrary; thus it's usually safer to call notifyAll() instead.
This rule is defined by the following XPath expression:
//Statement/StatementExpression/PrimaryExpression/PrimaryPrefix
[
./Name[
@Image='notify'
or ends-with(@Image,'.notify')
]
or @Image='notify'
or ( ./AllocationExpression
and ../PrimarySuffix[@Image='notify'] )
]
Here's an example of code that would trigger this rule:
x.notify();
// If many threads are monitoring x, only one (and you won't know which) will be notified.
// use instead:
x.notifyAll();
Each caught exception type should be handled in its own catch clause.
This rule is defined by the following XPath expression:
//TryStatement/FormalParameter
/following-sibling::Block//InstanceOfExpression/PrimaryExpression/PrimaryPrefix
/Name[
@Image = ./ancestor::Block/preceding-sibling::FormalParameter
/VariableDeclaratorId/@Image
]
Here's an example of code that would trigger this rule:
// Avoid this
try {
...something...
}
catch (Exception ee) {
if (ee instanceof IOException) {
cleanup();
}
}
// Prefer this:
try {
...something...
}
catch (IOException ee) {
cleanup();
}
The abstract class does not contain any abstract methods. An abstract class suggests an incomplete implementation, which is to be completed by subclasses implementing the abstract methods. If the class is intended to be used as a base class only (not to be instantiated direcly) a protected constructor can be provided prevent direct instantiation.
This rule is defined by the following XPath expression:
//ClassOrInterfaceDeclaration
[@Abstract='true'
and count( .//MethodDeclaration[@Abstract='true'] )=0 ]
[count(ImplementsList)=0]
Here's an example of code that would trigger this rule:
public abstract class Foo {
void int method1() { ... }
void int method2() { ... }
// consider using abstract methods or removing
// the abstract modifier and adding protected constructors
}
No need to check for null before an instanceof; the instanceof keyword returns false when given a null argument.
This rule is defined by the following XPath expression:
//Expression
[ConditionalOrExpression
[EqualityExpression[@Image='==']
//NullLiteral
and
UnaryExpressionNotPlusMinus
[@Image='!']//InstanceOfExpression[PrimaryExpression
//Name/@Image = ancestor::ConditionalOrExpression/EqualityExpression
//PrimaryPrefix/Name/@Image]]
or
ConditionalAndExpression
[EqualityExpression[@Image='!=']//NullLiteral
and
InstanceOfExpression
[PrimaryExpression
//Name/@Image = ancestor::ConditionalAndExpression
/EqualityExpression//PrimaryPrefix/Name/@Image]]
Here's an example of code that would trigger this rule:
class Foo {
void bar(Object x) {
if (x != null && x instanceof Bar) {
// just drop the "x != null" check
}
}
}