How to fix the following PMD violations

Question

I am using PMD to analyze code and it produces a few high priority warnings which I do not know how to fix.

1) Avoid if(x!=y)..; else...; But what should I do if I need this logic? That is, I do need to check if x!=y? How can I refactor it?

2) Use explicit scoping instead of the default package private level. But the class is indeed used only within the package. What access modifier should I use?

3) Parameter is not assigned and could be declared final. Should I add final keyword to all the places which PMD pointed out with this warning?

Solution 1

Avoid negation: Instead of if( x!=y ) doThis() else doThat(), check for the positive case first, because people/humans tend to like positive things more than negative. It twists the brain to have to reverse the logic in mind when reading the source code. So instead, write:

 if ( x!=y ) doThis() else doThat()       // Bad - negation first
 if ( x==y ) doThat() else doThis()       // Good - positive first

Explicit scoping: According to PMD website, it's a controversial rule. You may hate it, someone else likes it. What you should do is make all the fields within your classes private. There seems to be a field or method (not a class) with a package visibility, e.g. something like this:

 class Foo {
   /* private missing */ Object bar;
 }

Final parameters: Method parameters should be final to avoid accidental reassignment. That's just a good practice. If you're using Eclipse, the content assist even provides a quickfix called "Change modifiers to final where possible". Just select all code in the editor with Ctrl-a and then press Ctrl-1.

Solution 2

You don't need to enable all rules. Choose some of the rules you agree to and refactor your code until all warnings are cleared.

1 - Refactor it to a if (x == y) ... else ... logic. Just avoid negative conditions in if statments, they make code harder to understand

2 - I wouldn't enable that rule.

3 - A lot of people declare a lot of fields and variables final. Especially when they want to make sure or express that the value of a variable shall not be changed in the method. If you don't like that, disable that rule.

Solution 3

These all seem like minor warnings that could be turned off.

1) It wants you to flip the logic

if(x==y) {
    //old else clause
} else {
    //old if clause
}

2) If package is really the correct access you want, there is no access modifier to add. I am not familiar enough to know if there is a way to suppress that specific warning.

3) A style issue. Some people want final on everything it could be on. Others thinks it adds too much clutter for to little information. If you are in the latter camp, turn that warning off.

Solution 4

Regarding the first item (the inequality) there are two issues:

1) Readability of double negation.

Say you have:

if(x!=y) { false clause } else { true clause }

The second clause is executed if "not x is not equal to y".

This can be rewritten as:

if (x==y) {true clause } else {false clause}.

2) Correctness: if x and y are not-primitives, using if(!x.equals(y)) is safer. This is the equivalent of using == instead of .equals() and can lead to very serious bugs.

Solution 5

You can also use // NOPMD at the end of any line where you don't want PMD rules to be checked.

For example for the above given code you can suppress PMD check by giving,

class Foo {
   /* private missing */ Object bar; // NOPMD
 }

Please be aware that the above comment may silently suppress other warnings in the same line.

View more solutions