PMD - NPath complexity very high with ternary operator (?
I'm using PMD to generate some code quality report on a project.
I don't understand a result for the NPath complexity inspection. I have created a dull class that is show-casing the result (this is not the real class, but it is using the same pattern):import java.util.*;
public class SOFExample {
private final Map<String, Date> magicMap = new HashMap<String, Date>();
protected static final long UNKNWOWN = 0L;
private static final class MyCal { long aTime; long bTime; long cTime; long dTime;}
public void usefullMethod(final List<MyCal> myCals) {
final Date a = magicMap.get("a");
final Date b = magicMap.get("b");
final Date c = magicMap.get("c");
final Date d = magicMap.get("d");
final long aTime = a == null ? UNKNWOWN : a.getTime();
final long bTime = b == null ? UNKNWOWN : b.getTime();
final long cTime = c == null ? UNKNWOWN : c.getTime();
final long dTime = d == null ? UNKNWOWN : d.getTime();
for (MyCal myCal : myCals) {
if(myCal.aTime == UNKNWOWN) myCal.aTime = aTime;
if(myCal.bTime == UNKNWOWN) myCal.bTime = bTime;
if(myCal.cTime == UNKNWOWN) myCal.cTime = cTime;
if(myCal.dTime == UNKNWOWN) myCal.dTime = dTime;
}
}
}
PMD result:
The method usefullMethod() has an NPath complexity of 10625
If I add a new variable initialized the same way, I got this:
The method usefullMethod() has an NPath complexity of 103125
And if I replace all? With if-else structure, then I got this:
The method usefullMethod() has an NPath complexity of 1056
Why do I got this very high result with the ternary '?' Operator?
What's wrong开发者_StackOverflow中文版 with this code? (In this demo code it is easy to extract a method for getting the default values, but in real code it might not be possible)
Making the example even simpler, this class has a nPath value of 2. It should be pretty apparent why it is two - there are clearly two execution paths through the code.
package test;
import java.util.*;
public class Test {
private static final long UNKNWOWN = -1;
public void method(Date a) {
long aTime;
if (a == null) {
aTime = UNKNWOWN;
} else {
aTime = a.getTime();
}
}
}
And this class has a nPath value of 5. The question is why - there are still two logical paths through the code.
package test;
import java.util.*;
public class Test {
private static final long UNKNWOWN = -1;
public void method(Date a) {
final long aTime = a == null ? UNKNWOWN : a.getTime();
}
}
However, the algorithm used is as follows:
int npath = complexitySumOf(node, 0, data);
npath += 2;
It adds the complexity of all children then adds two for ternary. The minimum complexity returned for simple java nodes is 1. The AbstractSyntaxTree shows there are three children. Hence 3 + 2 is 5.
<ConditionalExpression beginColumn="36" beginLine="11" endColumn="69" endLine="11" ternary="true">
<EqualityExpression beginColumn="36" beginLine="11" endColumn="44" endLine="11" image="==">
<PrimaryExpression beginColumn="36" beginLine="11" endColumn="36" endLine="11">
<PrimaryPrefix beginColumn="36" beginLine="11" endColumn="36" endLine="11">
<Name beginColumn="36" beginLine="11" endColumn="36" endLine="11" image="a"/>
</PrimaryPrefix>
</PrimaryExpression>
<PrimaryExpression beginColumn="41" beginLine="11" endColumn="44" endLine="11">
<PrimaryPrefix beginColumn="41" beginLine="11" endColumn="44" endLine="11">
<Literal beginColumn="41" beginLine="11" charLiteral="false" endColumn="44" endLine="11" floatLiteral="false" intLiteral="false" singleCharacterStringLiteral="false" stringLiteral="false">
<NullLiteral beginColumn="41" beginLine="11" endColumn="44" endLine="11"/>
</Literal>
</PrimaryPrefix>
</PrimaryExpression>
</EqualityExpression>
<Expression beginColumn="48" beginLine="11" endColumn="55" endLine="11">
<PrimaryExpression beginColumn="48" beginLine="11" endColumn="55" endLine="11">
<PrimaryPrefix beginColumn="48" beginLine="11" endColumn="55" endLine="11">
<Name beginColumn="48" beginLine="11" endColumn="55" endLine="11" image="UNKNWOWN"/>
</PrimaryPrefix>
</PrimaryExpression>
</Expression>
<PrimaryExpression beginColumn="59" beginLine="11" endColumn="69" endLine="11">
<PrimaryPrefix beginColumn="59" beginLine="11" endColumn="67" endLine="11">
<Name beginColumn="59" beginLine="11" endColumn="67" endLine="11" image="a.getTime"/>
</PrimaryPrefix>
<PrimarySuffix argumentCount="0" arguments="true" arrayDereference="false" beginColumn="68" beginLine="11" endColumn="69" endLine="11">
<Arguments argumentCount="0" beginColumn="68" beginLine="11" endColumn="69" endLine="11"/>
</PrimarySuffix>
</PrimaryExpression>
</ConditionalExpression>
If you have a complex expression in the the ternary operator, the difference it count would be even more prevalent. As far as what's wrong with the code, it already has 9 branches (8 ternary operators and a loop) which is high even without the whole nPath calculation. I would refactor it regardless.
精彩评论