Clone wiki

pmd-rules / PmdRulesCodecop

Codecop

Mix of rules for various bugs and conventions. Some of these should really be in the core of PMD. Others are likely to be controversial.


JumbledIterator

The used Iterator for the 'exit expression' in a for loop does not match the 'declaration expression', most likely this is not intended.

This rule is defined by the following XPath expression:

//ForStatement[
  ( ForInit//ClassOrInterfaceType/@Image='Iterator' or
    ForInit//ClassOrInterfaceType/@Image='Enumeration'
  ) and
  ( ends-with(Expression//Name/@Image, '.hasNext') or
    ends-with(Expression//Name/@Image, '.hasMoreElements')
  ) and not (
    starts-with(Expression//Name/@Image, concat(ForInit//VariableDeclaratorId/@Image, '.'))
  )
]

Example:

public class JumbledIteratorExample {
   public void someMethod() {
      for (int i = 0; i < ab.size(); i++) { } // ok
      for (Enumeration en = getEnum(); en2.hasMoreElements(); ) { } // maybe wrong
      for (Enumeration en2 = getEnum(); en.hasMoreElements(); ) { } // maybe wrong
      List al = new ArrayList();
      for (Iterator it2 = al.iterator(); it.hasNext(); ) { } // maybe wrong
      for (Iterator it = al.iterator(); it2.hasNext(); ) { } // maybe wrong
      for (int i = 0; i < MAX && clauses.hasNext(); i++) { } // ok
   }
}

ParameterNameWithP

All parameters of methods and constructors are required to be named like pXyz. Use Ctrl-1 to local rename it.

This rule is defined by the following XPath expression:

//FormalParameters/FormalParameter/VariableDeclaratorId[
  (not (starts-with(@Image, $prefix))) or
  (not (substring(@Image, 2, 1)=upper-case(substring(@Image, 2, 1))) )
]

Example:

class ParameterNameWithPExample {
   public SomeClass(String newNo) { } // wrong
   public SomeClass(int pYes) { }
   public void setNo(String newNo) { // wrong
      no = newNo;
   }
   public void setYes(String pYes) {
      yes = pYes;
   }
}

This rule has the following properties:

Heading NameHeading Default valueHeading Description
prefix'p'Mandatory prefix to parameter names

UnintendedEnvUsage

The methods getBoolean, getInteger and getLong read from environment (System Properties) and do not parse. Use Integer.parseInt() instead.

This rule is defined by the following XPath expression:

//PrimaryExpression/PrimaryPrefix/Name[
  @Image='Integer.getInteger' or
  @Image='Boolean.getBoolean' or
  @Image='Long.getLong'
]

Example:

class UnintendedEnvUsageExample {
   public void someMethod() {
      Boolean a = Boolean.getBoolean("true"); // does not work
      Boolean b = new Boolean("true"); // ok
      Long.getLong("3"); // does not work
      Long.parseLong("3"); // ok
   }
}

JunitSetupDoesNotCallSuper

The framework methods setUp() and tearDown() of JUnit's Testcase must always call super.setUp() and super.tearDown() to enable proper preparing and cleaning of resources.

This rule is defined by the following XPath expression:

//MethodDeclarator[
  ( @Image='setUp' and count(FormalParameters/*)=0 and
    count(../Block//PrimarySuffix[@Image='setUp'])=0
  ) or
  ( @Image='tearDown' and count(FormalParameters/*)=0 and
    count(../Block//PrimarySuffix[@Image='tearDown'])=0
  )  or
  ( @Image='onSetUp' and count(FormalParameters/*)=0 and
    count(../Block//PrimarySuffix[@Image='onSetUp'])=0
  )  or
  ( @Image='onTearDown' and count(FormalParameters/*)=0 and
    count(../Block//PrimarySuffix[@Image='onTearDown'])=0
  )
]

Example:

class BadTestCase extends TestCase {
   protected void setUp() throws Exception {
      // super.setUp(); - is missing
      prepareSomething();
   }
   protected void tearDown() throws Exception {
      releaseSomething();
      // super.tearDown(); - is missing
   }
}

InterfaceNamesEndWithIF

For better recognition interface names should end with 'IF'. Rename the interface!

This rule is defined by the following XPath expression:

//ClassOrInterfaceDeclaration[@Interface='true' and
                              not (ends-with(@Image, $suffix))]

Example:

public interface SomeInterface { }     // nok
public interface SomeInterfaceF { }    // nok
public interface SomeInterfaceIF { }   // ok
public class SomeClass { }             // ok

This rule has the following properties:

Heading NameHeading Default valueHeading Description
suffix'IF'Mandatory suffix to interface names

InterfaceNamesStartWithI

For better recognition interface names should start with 'I'. Rename the interface!

This rule is defined by the following XPath expression:

//ClassOrInterfaceDeclaration[@Interface='true' and not (starts-with(@Image, $prefix))]

Example:

public interface SomeInterface { }     // nok
public interface SomeInterface { }     // nok
public interface ISomeInterface { }    // ok
public class SomeClass { }             // ok

This rule has the following properties:

Heading NameHeading Default valueHeading Description
prefix'I'Mandatory prefix to interface names

AtomWrapperInstantiation

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

This rule is defined by the following XPath expression:

//PrimaryPrefix/AllocationExpression[
  not (ArrayDimsAndInits) and
  (
    ClassOrInterfaceType/@Image='Byte' or ClassOrInterfaceType/@Image='java.lang.Byte' or
    ClassOrInterfaceType/@Image='Short' or ClassOrInterfaceType/@Image='java.lang.Short' or
    ClassOrInterfaceType/@Image='Integer' or ClassOrInterfaceType/@Image='java.lang.Integer' or
    ClassOrInterfaceType/@Image='Long' or ClassOrInterfaceType/@Image='java.lang.Long' or
    ClassOrInterfaceType/@Image='Character' or ClassOrInterfaceType/@Image='java.lang.Character'
  )
]

Example:

public class AtomWrapperInstantiationExample {
    Byte myByte = new Byte(1);    // bad
    Byte byte2 = Byte.valueOf(1); // ok
    Short myshort = new Short(20);       // bad
    Short myshort2 = Short.valueOf(20);  // ok
    Integer integer = new Integer(4);      // bad
    Integer integer2 = Integer.valueOf(4); // ok
    Long myLong = new Long(10000000);       // bad
    Long myLong2 = Long.valueOf(10000000);  // ok
    Character myChar = new Character('x');    // bad
    Character char2 = Character.valueOf('x'); // ok
}

CharInstantiation

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

This rule is defined by the following XPath expression:

//PrimaryPrefix/AllocationExpression[
  not (ArrayDimsAndInits) and
  ( ClassOrInterfaceType/@Image='Character' or ClassOrInterfaceType/@Image='java.lang.Character' )
]

Example:

public class CharInstantiationExample {
    Character char1 = new Character('x');     // bad
    Character char2 = Character.valueOf('x'); // ok
}

NonFinalFieldInException

Exceptions must be immutable, so the fields of an Exception must be declared as final.

This rule is defined by the following XPath expression:

//FieldDeclaration[ @Final='false' and
                    ../../../../ClassOrInterfaceDeclaration[ends-with(@Image, 'Exception')] ]

Example:

public class BadException extends Exception {
    private static long serialVersionUID = 1900926677490660714L; // Not ok - must be final!
    private String state;                                        // Not ok - must be final!
    public BadException(String message) {
        super(message);
    }
}

AvoidPrivateGetterAndSetter

Instead of using private getter or setter, we use the member variable instead. It's easier to read and saves lines of code.

This rule is defined by the following XPath expression:

//MethodDeclaration [
  @Private='true' and
  @Synchronized='false' and
  count(Block/BlockStatement)=1 and
  (
     (
        MethodDeclarator[starts-with(@Image,'set')] and
        MethodDeclarator/FormalParameters[count(FormalParameter)=1] and
        ResultType[count(Type)=0] and
        Block/BlockStatement/Statement/StatementExpression/Expression/PrimaryExpression[
           count(PrimarySuffix/Arguments)=0
        ] and //AssignmentOperator[@Image='=']
     ) or (
        MethodDeclarator[starts-with(@Image,'get') or starts-with(@Image,'is')] and
        MethodDeclarator/FormalParameters[count(FormalParameter)=0] and
        ResultType/Type[count(ReferenceType)=1] and
        Block/BlockStatement/Statement/ReturnStatement/Expression/PrimaryExpression[count(PrimarySuffix)=0] and
        count(Block/BlockStatement/Statement/ReturnStatement)=1
     )
  )
]

Example:

class AvoidPrivateGetterAndSetterExample {
   String myVariable;
   private void setMyVariable(String pMyVariable) {    // useless
      myVarialbe = pMyVariable;
   }
   ...
   myVariable = "asdfasdf";      // good, use the member variable instead of
}

MembersMustBePrivate

Members must be private, except static final constants. Change Member to private and create getter and/or setter to access value/s.

This rule is defined by the following XPath expression:

//FieldDeclaration[@Private='false' and (@Static='false' or @Final='false')]

Example:

public class MembersMustBePrivateExample {
   public int z1 = 1;      // WRONG must be private
   private int z2 = 2;     // OK its private (without static and final)
   protected int z3;       // WRONG must be private

   public static int z4 = 1;     // WRONG final is missing
   private static int z5 = 2;    // OK its private (with static)
   protected static int z6 = 3;  // WRONG final is missing

   public static final int z7 = 1;     // OK because static + final
   private static final int z8 = 2;    // OK its private (with static + final)
   protected static final int z9 = 3;  // OK because static + final

SignatureDeclareThrowsException

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.

This rule is defined by the following Java class: org.codecop.pmd.rule.ExceptionSignatureDeclaration

Example:

public void methodThrowingException() throws Exception { }

This rule has the following properties:

Heading NameHeading Default valueHeading Description
ignoreTests'false'Ignore test methods

ClassNamingConventions

Class names should always begin with an upper case character, and should not contain underscores (but _Stub or _Core) or dollar signs. Class names should not be uppercase only.

This rule is defined by the following Java class: org.codecop.pmd.rule.ClassNamingConventions

Example:

public class FOOBAR {}   // bad
public class Foo_Bar {}  // bad
public class Foo$Bar {}  // bad

public class URI {}      // ok - short abbrev
public class Foo_Stub {} // ok - created by RMI compiler
public class Foo_Core {} // ok - Generation Gap Pattern

This rule has the following properties:

Heading NameHeading Default valueHeading Description
upperCaseLen'3'Allowed length of upper case only names

LoggerHasWrongCategory

Logger is created for another category than the enclosing class. This is a typical copy-paste error.

This rule is defined by the following XPath expression:

//PrimaryExpression
[
  (PrimaryPrefix/Name/@Image=$logFactory) and
  (not (PrimarySuffix//ClassOrInterfaceType/@Image=ancestor::ClassOrInterfaceDeclaration/@Image))
]

Example:

class GoodExample {
   private final Logger log = Logger.getLogger(GoodExample.class);
}
class BadExample {
   private final Logger log = Logger.getLogger(GoodExample.class);
}

This rule has the following properties:

Heading NameHeading Default valueHeading Description
logFactory'Logger.getLogger'Invocation of logger factory

AvoidThrowingCheckedException

Avoid throwing checked Exceptions - it's considered noise.

This rule is defined by the following Java class: org.codecop.pmd.rule.AvoidThrowingCheckedException

Example:

public class Foo {
  void bar() throws IOException {
    throw new IOException();
  }
}

ShortVariableCustom

Detects when a field, local, or parameter has a very short name.

This rule is defined by the following XPath expression:

//VariableDeclaratorId[string-length(@Image) < $minLen]
  [not(ancestor::ForInit)]
  [not((ancestor::FormalParameter) and (ancestor::TryStatement))]

Example:

public class Something {
  private int q = 15; // VIOLATION - Field
  public static void main(String[] as) {  // VIOLATION - Formal
    int r = 20 + q; // VIOLATION - Local
    for (int i = 0; i < 10; i++) { // Not a Violation (inside FOR)
      r += q;
    }
  }
}

This rule has the following properties:

Heading NameHeading Default valueHeading Description
minLen'3'Minimal length of variable names

DontImportWild

Do not use wildcard imports such as java.net.*.

This rule is defined by the following XPath expression:

/ImportDeclaration[@PackageName=@ImportedName]

Example:

import java.net.*; // is bad
import java.net.URL; // is better

PrivateInjections

Set all injected fields to private.

This rule is defined by the following XPath expression:

//ClassOrInterfaceBodyDeclaration[contains(Annotation//Name/@Image, 'Inject') and
                                  contains(FieldDeclaration/@Private, 'false')]

Example:

@Inject
public String badParameter; // is bad

@Inject
private String goodParameter; // is better

JUnitTestsShouldIncludeAssertOrVerify

JUnit tests should include at least one assertion. This makes the tests more robust, and using assert with messages provide the developer a clearer idea of what the test does.

This rule is defined by the following Java class: org.codecop.pmd.rule.JUnitTestsShouldIncludeAssertOrVerify

Example:

public class Foo extends TestCase {
  public void testSomething() {
    Bar b = findBar();
    // This is better than having a NullPointerException
    // assertNotNull("bar not found", b);
    b.work();
  }
}

PrimitiveObsession

You are likely using primitive data types to represent domain ideas. For example, a String to represent a message, an Integer to represent an amount of money, or a Struct/Dictionary/Hash to represent a specific object. You should introduce a `ValueObject` in place of the primitive data.

This rule is defined by the following Java class: org.codecop.pmd.rule.PrimitiveObsession

Example:

class BadObject {
   public void badVoidMethod(String a, String b) { };
   public String badMixedMethod(Date when) { };
}

This rule has the following properties:

Heading NameHeading Default valueHeading Description
allowObject'true'Allow plain java.lang.Object
checkConstructors'false'Check public constructors for more than one primitive

MutableException

Exception instances should represent an error condition. Having non final fields allows the state to be modified by accident and therefore mask the original condition.

This rule is defined by the following XPath expression:

//FieldDeclaration[@Final='false' and
                   ../../..[ends-with(@Image,'Exception') or ends-with(@Image,'Error')]]

Example:

public class ChannelException {
   private final String channel; // ok
   private boolean showIfSearchedInRegion; // not ok
}
public class ChannelPolicyError {
   private final String channel;  // ok
   private final boolean showIfSearchedInRegion; // ok
}

OneLevelOfIntention

Use only one level of intention per method. You cannot use if, loops or trys more than once in a method. If used they need to be the first/last/outer statement of the method. This enforces SRP split on method level. This is rule 1 of Object Calisthenics.

This rule is defined by the following XPath expression:

//MethodDeclaration[
        count( descendant::IfStatement | descendant::SwitchStatement | descendant::TryStatement |
               descendant::ForStatement | descendant::WhileStatement | descendant::DoStatement ) > 1
        or Block/BlockStatement[
            count( Statement[IfStatement | SwitchStatement | TryStatement | ForStatement | WhileStatement | DoStatement] ) > 0 and
            count( parent::Block/BlockStatement ) > 1
                ]
    ]

Example:

public class Foo {
    public void okBecauseOneBlock() {
        if (true) {
            // ok, only one block
        }
    }
    public void failBecauseNested() {
        if (true) {
            while(true) {  }
        }
    }
}

FirstClassCollections

Each collection gets wrapped in its own class, so now behaviors related to the collection have a home. Constants do not count. This is rule 4 of Object Calisthenics.

This rule is defined by the following XPath expression:

//FieldDeclaration[
        ( @Array='true' or
          Type[@TypeImage='List' or @TypeImage='Set' or @TypeImage='SortedSet' or @TypeImage='NavigableSet' or
               @TypeImage='Map' or @TypeImage='SortedMap' or @TypeImage='NavigableMap' or
               @TypeImage='Collection' or @TypeImage='Deque' or @TypeImage='Queue' ]
        ) and
        count( //FieldDeclaration[@Static='false' or @Final='false'] ) > 1
    ]

Example:

public class FooMap {
    private Map map; // ok
}
public class Mixup {
    private Map map;
    private int i; // yuk
}

AvoidClassNamingImpl

For better naming implementation classes should not end with 'Impl'. Rename the class!

This rule is defined by the following XPath expression:

//ClassOrInterfaceDeclaration[ends-with(@Image, $suffix)]

Example:

public class SomeImpl { }     // nok
public class SomeClass { }    // ok

This rule has the following properties:

Heading NameHeading Default valueHeading Description
suffix'Impl'Mandatory forbidden suffix to class names

TestNameShouldStartWithShould

Write your tests in the behaviour driven development style of testing, as it encourages you to write tests that read as specifications, and are more aligned with the behaviour of the class or system that you are testing.

This rule is defined by the following XPath expression:

//MethodDeclaration[(../Annotation/MarkerAnnotation/Name[@Image='Test']) and (not(starts-with(MethodDeclarator/@Image, 'should')))]

Example:

public class GoodNamedTest {
  @Test
  public void shouldRun() { // ok
  }
}

public class BadNamedTest {
  @Test
  public void testRun() { // nok
  }
}

Updated