Skip to content

Commit

Permalink
Fix npath bugs with ternary
Browse files Browse the repository at this point in the history
  • Loading branch information
@oowekyala
oowekyala committed Aug 8, 2017
1 parent 6abe95d commit b86f0ae
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 24 deletions.
22 changes: 9 additions & 13 deletions pmd-java/src/main/java/net/sourceforge/pmd/lang/java/metrics/impl/CycloMetric.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import net.sourceforge.pmd.lang.java.ast.ASTConditionalOrExpression;
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.ast.JavaParserVisitor;
import net.sourceforge.pmd.lang.java.metrics.JavaMetrics;
import net.sourceforge.pmd.lang.java.metrics.api.JavaOperationMetricKey;
Expand All @@ -29,26 +30,21 @@
*
* <p>The standard version of the metric complies with McCabe's original definition [3]:
*
* <ul>
* <li>+1 for every control flow statement ({@code if, case, catch, throw, do, while, for, break, continue}) and
* <ul> <li>+1 for every control flow statement ({@code if, case, catch, throw, do, while, for, break, continue}) and
* conditional expression ({@code ? : }). Notice switch cases count as one, but not the switch itself: the point is that
* a switch should have the same complexity value as the equivalent series of {@code if} statements.
* <li>{@code else}, {@code finally} and {@code default} don't count;
* <li>+1 for every boolean operator ({@code &&, ||}) in the guard condition of a control flow statement. That's because
* Java has short-circuit evaluation semantics for boolean operators, which makes every boolean operator kind of a
* control flow statement in itself.
* </ul>
* a switch should have the same complexity value as the equivalent series of {@code if} statements. <li>{@code else},
* {@code finally} and {@code default} don't count; <li>+1 for every boolean operator ({@code &&, ||}) in the guard
* condition of a control flow statement. That's because Java has short-circuit evaluation semantics for boolean
* operators, which makes every boolean operator kind of a control flow statement in itself. </ul>
*
* <p>Version {@link CycloVersion#IGNORE_BOOLEAN_PATHS}: Boolean operators are not counted, which means that empty
* fall-through cases in {@code switch} statements are not counted as well.
*
* <p>References:
*
* <ul>
* <li> [1] Lanza, Object-Oriented Metrics in Practice, 2005.
* <li> [2] McCabe, A Complexity Measure, in Proceedings of the 2nd ICSE (1976).
* <li> [3] <a href="https://docs.sonarqube.org/display/SONAR/Metrics+-+Complexity">Sonarqube online documentation</a>
* </ul>
* <ul> <li> [1] Lanza, Object-Oriented Metrics in Practice, 2005. <li> [2] McCabe, A Complexity Measure, in Proceedings
* of the 2nd ICSE (1976). <li> [3] <a href="https://docs.sonarqube.org/display/SONAR/Metrics+-+Complexity">Sonarqube
* online documentation</a> </ul>
*
* @author Clément Fournier
* @since June 2017
Expand Down
11 changes: 10 additions & 1 deletion ...rc/main/java/net/sourceforge/pmd/lang/java/metrics/impl/visitors/DefaultNpathVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,16 @@ public Object visit(ASTSwitchStatement node, Object data) {

@Override
public Object visit(ASTConditionalExpression node, Object data) {
return node.isTernary() ? sumChildrenComplexities(node, data) + 2 : 1;
// bool comp of guard clause + complexity of last two children (= total - 1)

if (node.isTernary()) {
ASTExpression wrapper = new ASTExpression(Integer.MAX_VALUE);
wrapper.jjtAddChild(node.jjtGetChild(0), 0);
int boolCompTernary = CycloMetric.booleanExpressionComplexity(wrapper);

return boolCompTernary + sumChildrenComplexities(node, data) - 1;
}
return 1;
}


Expand Down
170 changes: 163 additions & 7 deletions pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/metrics/impl/xml/NPathTest.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
public class Foo {
public static void bar() {
boolean a, b = true;
try { // total 6
try { // total 8
if (true) { // 2
List buz = new ArrayList();
}
for(int i = 0; i < 19; i++) { // 2
List buz = new ArrayList();
for(int i = 0; i < 19; i++) { // 3
List buz = a ? new ArrayList() : null;
}
} catch(Exception e) {
if (true) { // 2
Expand All @@ -23,8 +23,12 @@ public class Foo {
if (true || a && b) { // 4
j = 10;
return;
} else {
j = 12;
}
j = a || b ? j + 1 : j; // 3
while (j++ < 20) { // 2
List buz = new ArrayList();
}
Expand Down Expand Up @@ -53,7 +57,7 @@ public class Foo {
<description>Full example</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.Foo#bar()' has value 2016.</message>
<message>'.Foo#bar()' has value 8064.</message>
</expected-messages>
<code-ref id="full-example"/>
</test-code>
Expand Down Expand Up @@ -266,11 +270,11 @@ class Bar {
]]></code-fragment>

<test-code>
<description>test case for bug 3484404 (Invalid NPath calculation in return statement)</description>
<description>Test case for bug 3484404 (Invalid NPath calculation in return statement)</description>
<expected-problems>3</expected-problems>
<expected-messages>
<message>'.Bar#x(boolean, boolean)' has value 25.</message>
<message>'.Bar#y(boolean, boolean)' has value 25.</message>
<message>'.Bar#x(boolean, boolean)' has value 4.</message>
<message>'.Bar#y(boolean, boolean)' has value 4.</message>
<message>'.Bar#z(int, int)' has value 1.</message>
</expected-messages>
<code-ref id="bug3484404"/>
Expand Down Expand Up @@ -300,4 +304,156 @@ class Bar {
]]></code>
</test-code>

<test-code>
<description>NPath should count the boolean complexity of ternaries</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.Test#method(Date)' has value 4.</message>
</expected-messages>
<code><![CDATA[
public class Test {
public void method(Date a) {
final long aTime = (a == null && a || b) ? 0L : a.getTime();
}
}
]]></code>
</test-code>

<test-code>
<description>#01 NPath should count ternaries like if - else constructs (https://stackoverflow.com/q/5079923/6245827)</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.Test#method(Date)' has value 2.</message>
</expected-messages>
<code><![CDATA[
public class Test {
private static final long UNKNOWN = -1;
public void method(Date a) {
long aTime;
if (a == null) {
aTime = UNKNOWN;
} else {
aTime = a.getTime();
}
}
}
]]></code>
</test-code>

<test-code>
<description>#02 NPath should count ternaries like if - else constructs (https://stackoverflow.com/q/5079923/6245827)</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.Test#method(Date)' has value 2.</message>
</expected-messages>
<code><![CDATA[
public class Test {
private static final long UNKNOWN = -1;
public void method(Date a) {
final long aTime = a == null ? UNKNOWN : a.getTime();
}
}
]]></code>
</test-code>

<test-code>
<description>#1 NPath should count ternaries like if - else constructs (https://stackoverflow.com/q/5079923/6245827)</description>
<rule-property name="reportLevel">6</rule-property>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.SOFExample#usefulMethod(List)' has value 272.</message>
</expected-messages>
<code><![CDATA[
public class SOFExample {
private final Map<String, Date> magicMap = new HashMap<String, Date>();
protected static final long UNKNOWN = 0L;
private static final class MyCal { long aTime; long bTime; long cTime; long dTime;}
public void usefulMethod(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 ? UNKNOWN : a.getTime();
final long bTime = b == null ? UNKNOWN : b.getTime();
final long cTime = c == null ? UNKNOWN : c.getTime();
final long dTime = d == null ? UNKNOWN : d.getTime();
for (MyCal myCal : myCals) {
if(myCal.aTime == UNKNOWN) myCal.aTime = aTime;
if(myCal.bTime == UNKNOWN) myCal.bTime = bTime;
if(myCal.cTime == UNKNOWN) myCal.cTime = cTime;
if(myCal.dTime == UNKNOWN) myCal.dTime = dTime;
}
}
}
]]></code>
</test-code>

<test-code>
<description>#2 NPath should count ternaries like if - else constructs (https://stackoverflow.com/q/5079923/6245827)</description>
<rule-property name="reportLevel">6</rule-property>
<expected-problems>1</expected-problems>
<expected-messages>
<message>'.SOFExample#usefulMethod(List)' has value 272.</message>
</expected-messages>
<code><![CDATA[
public class SOFExample {
private final Map<String, Date> magicMap = new HashMap<String, Date>();
protected static final long UNKNOWN = 0L;
private static final class MyCal { long aTime; long bTime; long cTime; long dTime;}
public void usefulMethod(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;
if (a == null) {
aTime = a.getTime();
} else {
aTime = UNKNOWN;
}
final long bTime;
if (b == null) {
bTime = b.getTime();
} else {
bTime = UNKNOWN;
}
final long cTime;
if (c == null) {
cTime = c.getTime();
} else {
cTime = UNKNOWN;
}
final long dTime;
if (d == null) {
dTime = d.getTime();
} else {
dTime = UNKNOWN;
}
for (MyCal myCal : myCals) {
if(myCal.aTime == UNKNOWN) myCal.aTime = aTime;
if(myCal.bTime == UNKNOWN) myCal.bTime = bTime;
if(myCal.cTime == UNKNOWN) myCal.cTime = cTime;
if(myCal.dTime == UNKNOWN) myCal.dTime = dTime;
}
}
}
]]></code>
</test-code>
</test-data>
6 changes: 3 additions & 3 deletions ...ava/src/test/resources/net/sourceforge/pmd/lang/java/rule/metrics/xml/NPathComplexity.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,11 @@ class Bar {

<test-code>
<description>test case for bug 3484404 (Invalid NPath calculation in return statement)</description>
<rule-property name="reportLevel">5</rule-property>
<rule-property name="reportLevel">4</rule-property>
<expected-problems>2</expected-problems>
<expected-messages>
<message>The method 'x(boolean, boolean)' has an NPath complexity of 25</message>
<message>The method 'y(boolean, boolean)' has an NPath complexity of 25</message>
<message>The method 'x(boolean, boolean)' has an NPath complexity of 4</message>
<message>The method 'y(boolean, boolean)' has an NPath complexity of 4</message>
</expected-messages>
<code-ref id="bug3484404"/>
</test-code>
Expand Down

0 comments on commit b86f0ae

Please sign in to comment.