-
Notifications
You must be signed in to change notification settings - Fork 0
PmdRulesCodecop
Peter Kofler edited this page Feb 10, 2020
·
22 revisions
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.
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, '.'))
)
]
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
}
}
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))) ) ]
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:
| Name | Default value | Description |
|---|---|---|
| prefix | 'p' | Mandatory prefix to parameter names |
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' ]
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
}
}
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
)
]
class BadTestCase extends TestCase {
protected void setUp() throws Exception {
// super.setUp(); - is missing
prepareSomething();
}
protected void tearDown() throws Exception {
releaseSomething();
// super.tearDown(); - is missing
}
}
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))]
public interface SomeInterface { } // nok
public interface SomeInterfaceF { } // nok
public interface SomeInterfaceIF { } // ok
public class SomeClass { } // ok
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| suffix | 'IF' | Mandatory suffix to interface names |
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))]
public interface SomeInterface { } // nok
public interface SomeInterface { } // nok
public interface ISomeInterface { } // ok
public class SomeClass { } // ok
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| prefix | 'I' | Mandatory prefix to interface names |
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'
)
]
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
}
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' ) ]
public class CharInstantiationExample {
Character char1 = new Character('x'); // bad
Character char2 = Character.valueOf('x'); // ok
}
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, $suffix)] ]
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);
}
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| suffix | 'Exception' | Suffix of Exception names (to identify) |
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
(count(descendant::Expression)=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/descendant::*)=0) and
(count(descendant::Expression)=0) and
(count(descendant::AllocationExpression)=0)
] and
count(Block/BlockStatement/Statement/ReturnStatement)=1
)
)
]
class AvoidPrivateGetterAndSetterExample {
String myVariable;
private void setMyVariable(String pMyVariable) { // useless
myVarialbe = pMyVariable;
}
...
myVariable = "asdfasdf"; // good, use the member variable instead of
}
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')]
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
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
public void methodThrowingException() throws Exception { }
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| ignoreTests | 'false' | Ignore test methods |
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 upper case only.
This rule is defined by the following Java class: org.codecop.pmd.rule.ClassNamingConventions
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:
| Name | Default value | Description |
|---|---|---|
| upperCaseLen | '3' | Allowed length of upper case only names |
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)) ]
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:
| Name | Default value | Description |
|---|---|---|
| logFactory | 'Logger.getLogger' | Invocation of logger factory |
Avoid throwing checked Exceptions - it's considered noise.
This rule is defined by the following Java class: org.codecop.pmd.rule.AvoidThrowingCheckedException
public class Foo {
void bar() throws IOException {
throw new IOException();
}
}
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))]
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:
| Name | Default value | Description |
|---|---|---|
| minLen | '3' | Minimal length of variable names |
Do not use wild card imports such as java.net.*.
This rule is defined by the following XPath expression:
/ImportDeclaration[@PackageName=@ImportedName]
import java.net.*; // is bad import java.net.URL; // is better
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')]
@Inject public String badParameter; // is bad @Inject private String goodParameter; // is better
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| inject | 'Inject' | Name of Inject annotation |
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
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();
}
}
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
class BadObject {
public void badVoidMethod(String a, String b) { };
public String badMixedMethod(Date when) { };
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| allowObject | 'true' | Allow plain java.lang.Object |
| checkConstructors | 'false' | Check public constructors for more than one primitive |
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, $suffix) or ends-with(@Image, 'Error')]]
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
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| suffix | 'Exception' | Suffix of Exception names (to identify) |
Immutable data structures are easier to reason about and have no concurrency issues. Only final fields are allowed. (Mutable field references should be copied when passed in or out, but that is not checked.)
This rule is defined by the following XPath expression:
//FieldDeclaration[@Final='false']
public class SomeClass {
private final String channel; // ok
private boolean showIfSearchedInRegion; // not ok
}
Use only a maximum of one level of intention per method. You cannot nest if, loops or trys- This enforces SRP split on method level. This is rule 1 of Object Calisthenics.
This rule is defined by the following Java class: org.codecop.pmd.rule.LevelOfIntentation
public class Foo {
public void okBecauseOneBlock() {
if (true) {
// ok, only one block
}
}
public void failBecauseNested() {
if (true) {
while(true) { } // nesting 2
}
}
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| problemDepth | '1' | Maximum allowed nesting depth. |
Use only one level of intention per method. You cannot use if, loops or try 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 (strict) 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
]
]
public class Foo {
public void okBecauseOneBlock() {
if (true) {
// ok, only one block
}
}
public void failBecauseNested() {
if (true) {
while(true) { }
}
}
}
Each collection gets wrapped in its own class, so now behaviour 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
]
public class FooMap {
private Map map; // ok
}
public class Mixup {
private Map map;
private int i; // yuk
}
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)]
public class SomeImpl { } // nok
public class SomeClass { } // ok
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| suffix | 'Impl' | Forbidden suffix to class names |
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=$classSuffix])
and
(not(starts-with(MethodDeclarator/@Image, $methodPrefix)))
]
public class GoodNamedTest {
@Test
public void shouldRun() { // ok
}
}
public class BadNamedTest {
@Test
public void testRun() { // nok
}
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| classSuffix | 'Test' | Suffix to test class names (to identify) |
| methodPrefix | 'should' | Mandatory prefix to test methods |
Type casting an object to a subclass' type is not object oriented. Use polymorphy to access the behaviour you need.
This rule is defined by the following XPath expression:
//CastExpression/Type/ReferenceType[ not (ancestor::MethodDeclaration/@MethodName='equals') ]
public class OneCast {
private void run(Object param) {
Integer var = (Integer) param; // Don't do that
}
private void run(long param) {
int var = (int) param; // truncating is ok
}
}
A class with too many public methods is probably a good suspect for refactoring, to reduce its complexity and find a way to have more fine grained objects.
This rule is defined by the following XPath expression:
//ClassOrInterfaceDeclaration/ClassOrInterfaceBody[
count(descendant::MethodDeclaration[@Public = 'true']) > $maxMethods
]
public class Foo {
public void one() {}
public void two() {}
public void three() {}
public void four() {} // too many
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| maxMethods | '3' | The method count reporting threshold |
An interface with too many methods is an ISP (SOLID) violation and needs to be refactored, to reduce its complexity and find a way to have more fine grained objects.
This rule is defined by the following XPath expression:
//ClassOrInterfaceDeclaration[
(@Interface='true') and
(ClassOrInterfaceBody[ count(descendant::MethodDeclaration) > $maxMethods ])
]
public interface Foo {
void one();
void two();
void three();
void four(); // too many
}
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| maxMethods | '3' | The method count reporting threshold |
Limit the number of classes per package. Keep packages small and focused.
This rule is defined by the following Java class: org.codecop.pmd.rule.ExcessiveClassCount
This rule has the following properties:
| Name | Default value | Description |
|---|---|---|
| maxClasses | '20' | Maximum allowed classes in package. |