14

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 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)

Guillaume
  • 5,488
  • 11
  • 47
  • 83

1 Answers1

20

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.

Jeanne Boyarsky
  • 12,156
  • 2
  • 49
  • 59
  • Thanks for your explanation. In fact I have already looked in the code to try to understand why the complexity was going so high with my code. What I don't understand is why this algo is used for the ternary operator, giving the npath a logarithmic value (100, 1000, 10000, 1000000, ...) That is no sounding very fair to the real complexity... About refactoring, the fact that your have to deal with potential null value leads to code like that, difficult to refactor, except by using complex pattern... So it is not always the good solution... – Guillaume Feb 28 '11 at 08:37
  • 3
    I'm not passing judgement on what's good or bad. Just saying what is. (And I agree they are way overcounting in the case of a ternary operator.) – Jeanne Boyarsky Mar 01 '11 at 00:47