SONARJS-862 Rule: Cognitive Complexity of functions should not be too high #449

vilchik-elena · 2017-01-19T11:01:49Z

No description provided.

ivandalbosco

@vilchik-elena Good work! Kindly check my comments, Don't forget to handle the "(a && b) && c" and "a && (b && c)" cases!

ivandalbosco · 2017-01-26T11:06:45Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

+    }
+  }
+
+  private class CognitiveComplexity extends DoubleDispatchVisitor {


@vilchik-elena This class is a valuable asset of the analyzer. It is likely to be reused in the future, e.g., for computing the complexity of a class or a file, or to display the complexity, etc. Therefore I would like to see this class as a top-level one, in project javascript-fronted.

@ivandalbosco I would prefer to extract it as soon as it will be required somewhere else.

@vilchik-elena @pynicolas @inverno Actually we had a long discussion last Friday (unfortunately you weren't present) about this point. Carlo's view point and mine was that whenever a strong business notion is found, it immediately deserves a separate class; waiting for a second usage of the notion is not a good criterion for creating the separate class. Pierre-Yves was of opposite opinion (yours). I really prefer the former.

@ivandalbosco This will require time and efforts now, which are investments in the future, hypothetical future. In fact I've not heard that we will do CG metric, moreover I think CG of class or file contradicts to its notion.

ivandalbosco · 2017-01-26T12:43:49Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+
+    for (i = 0; i < length; i++) {  // +1 (nesting level +1)
+//S ^^^ LOOPS {{+1}}
+        if (condition) {            // +2


@vilchik-elena How about testing a "for" loop within a "for" loop? In my experience it is a common case.

@ivandalbosco done

ivandalbosco · 2017-01-26T13:04:16Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+
+function nested_conditional_expression() { // Noncompliant [[effortToFix=6]]
+    x = condition1 ? (condition2 ? trueValue2 : falseValue2) : falseValue1 ; // +3
+    x = condition1 ? trueValue1 : (condition2 ? trueValue2 : falseValue2)  ; // +3


@vilchik-elena OK. I just suggest that you replace this line with:
x = condition1 ? (condition2 ? trueValue2 : falseValue2) : (condition3 ? trueValue3 : falseValue3); // +5

@ivandalbosco I've added it

ivandalbosco · 2017-01-26T13:17:06Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+//S     ^^ TRY_CATCH
+    }
+}
+


@vilchik-elena I suggest you also add a case of nested try-catch, e.g,:

function nested_try_catch() { // Noncompliant [[effortToFix=3;id=NESTED_TRY_CATCH]]
try {
if (condition) { // +1 (nesting level +1)
//S ^^ NESTED_TRY_CATCH
try {}
catch (someError) { // +2
//S ^^^^^ NESTED_TRY_CATCH
}
}
} finally {
}
}

@ivandalbosco FYI in github comments you can wrap one line code in single backticks and multiline code in triple backticks

@ivandalbosco thanks for the case. Added!

ivandalbosco · 2017-01-26T14:54:49Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+
+  for (var i = a && b; a && b; a && b) { // +1(for) +1(&&)  +1(&&)  +1(&&)
+  }
+}


@vilchik-elena I may be wrong, but I see nowhere in your nesting tests that complexity may increase more than once at a same nesting level. It is definitely worth it, because I know it has been a matter of debate: do we increase complexity once per level or possibly several times (the latter was retained). For example:

function nesting() { // Noncompliant [[effortToFix=5]]
if (condition) { // +1 (+1 for nesting)
if (condition) {} // +2
if (condition) {} // +2 (this one was debated - personally I disagree but this is irrelevant)
}
}

ivandalbosco · 2017-01-26T15:16:14Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

+
+  /* When complexity of nesting function is considered for this check, we ignore all nested in it functions */
+  private Set<Tree> ignoredNestedFunctions = new HashSet<>();
+  private final CognitiveComplexity cognitiveComplexity = new CognitiveComplexity();


@vilchik-elena I am not sure that this implementation is fault-tolerant. Class CognitiveComplexity is stateful. If an exception is raised in CognitiveComplexity#visitFunction(tree), the clean up in method CognitiveComplexity#leaveFunction(tree) will not be performed, so the CognitiveComplexity of the first function of the next file may be polluted. How about moving this initialization to line 85?

@ivandalbosco Agree about the fact that CognitiveComplexity#leaveFunction(tree) might not be done in case of some exception. But in fact if some exception is raised the entire analysis is stopped so we don't care.
But we might change this behaviour at any moment so I will force initialization of CognitiveComplexity for each file.
Thanks!

ivandalbosco · 2017-01-26T15:21:22Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+
+function and_or() {  // Noncompliant [[effortToFix=7;id=LOGICAL_OPS]]
+    foo(1 && 2 && 3 && 4); // +1
+//S                 ^^ LOGICAL_OPS {{+1}}


@vilchik-elena Kindly see Ann's email about
var x = (a && b && c); // +1
var x = ((a && b) && c); // +1, not +2
Thanks!

ivandalbosco · 2017-01-26T15:30:43Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

+      }
+    }
+
+    private void checkComplexity() {


@vilchik-elena This method could be public, named "getComplexity", and not raising issue. I would delegate the threshold and the issue raising to the enclosing Check class.

@ivandalbosco done in 21c4bd5

ivandalbosco · 2017-01-26T15:43:31Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

+        return "+1";
+
+      } else{
+        return String.format("+%s (incl %s for nesting)", complexity, complexity - 1);


@vilchik-elena How about adding a dot at the end of "incl", that is "incl." ?

@ivandalbosco ok

ivandalbosco · 2017-01-26T16:38:55Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

+//                         ^^
+var functionExpression = function(a, b) { return a && b; } // Noncompliant [[effortToFix=1]]
+//                       ^^^^^^^^
+


@vilchik-elena Maybe an example with an IIFE would be nice. How about a nasty little case like this?

(function(a) {
if (cond) {}
return a;
})(function(b) {return b + 1})(0);

I don't know what it is supposed to display... Too tired for today...

@ivandalbosco It's nasty, but not much interesting: one issue on first line (complexity = 1). But ok, I will add it

SonarTech · 2017-01-30T10:37:38Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

+        boolean leftChildOfSameKind = leftChild.is(javaScriptTree.getKind());
+        boolean rightChildOfSameKind = rightChild.is(javaScriptTree.getKind());
+
+        // For expressions with same-kind operators like "a && (b && c)" we want to have secondary location on leftmost operator


This block of commented-out lines of code should be removed.

… high

SonarTech · 2017-01-30T11:02:40Z

SonarQube analysis reported 1 issue

1 major

Watch the comments in this conversation to review them.

ivandalbosco · 2017-01-30T15:01:57Z

javascript-checks/src/test/resources/checks/CognitiveComplexityFunctionCheck/zeroThreshold.js

@@ -207,10 +207,13 @@ function with_complexity_after_nested_function() { // Noncompliant [[effortToFix
   if (condition) {}           // +1
 }

-function and_or() {  // Noncompliant [[effortToFix=7;id=LOGICAL_OPS]]
+function and_or() {  // Noncompliant [[effortToFix=8;id=LOGICAL_OPS]]
    foo(1 && 2 && 3 && 4); // +1
 //S       ^^ LOGICAL_OPS {{+1}}


@vilchik-elena Great! I can see that
foo(1 && 2 && (3 && 4))
is +1, whereas
foo(1 && 2 && !(3 && 4))
is +2. Can you please add one or more test with the "not" operator?

@inverno done

ivandalbosco · 2017-01-30T15:15:57Z

...cript-checks/src/main/java/org/sonar/javascript/checks/CognitiveComplexityFunctionCheck.java

  public void setThreshold(int threshold) {
    this.threshold = threshold;
  }
+
+  private static class ComplexityData {


@vilchik-elena Good design, this class was indeed a must. However, you are in the middle of the river: methods complexity() and aggregatedNestedFunctions() are public (probably for future usage), whereas the class itself is private. So Eclipse correctly tells me that those two methods are "never used locally". I still recommend you move classes CognitiveComplexity and ComplexityData to package org.sonar.javascript.metrics. But this is up to you, I won't argue if you disagree...

… high (#449)

vilchik-elena force-pushed the SONARJS-862 branch 3 times, most recently from 1282a32 to c095cf6 Compare January 26, 2017 11:12

ivandalbosco reviewed Jan 27, 2017

View reviewed changes

SonarTech reviewed Jan 30, 2017

View reviewed changes

vilchik-elena added 7 commits January 30, 2017 11:54

SONARJS-862 Rule: Cognitive Complexity of functions should not be too…

1ed1eda

… high

Move secondary location to first logical operator

6343f84

Fix order of secondary locations

bf81d48

Fix for (a && b) && c

682ed51

Fix for a && (b && c)

115dad4

Fix some flaws and add more test cases

544f2cc

Remove FunctionComplexity check from default profile

eb9ce05

vilchik-elena force-pushed the SONARJS-862 branch 2 times, most recently from 7cbfe22 to 21c4bd5 Compare January 30, 2017 11:00

ivandalbosco reviewed Jan 30, 2017

View reviewed changes

vilchik-elena added 2 commits January 30, 2017 16:22

Separate check logic and complexity computation logic

4aa7222

One more test case

b4f3d50

vilchik-elena force-pushed the SONARJS-862 branch 2 times, most recently from 714ba83 to b4f3d50 Compare January 30, 2017 15:23

vilchik-elena merged commit 149e36e into master Jan 30, 2017

inverno pushed a commit that referenced this pull request Jan 31, 2017

SONARJS-862 Rule: Cognitive Complexity of functions should not be too…

260cd63

… high (#449)

vilchik-elena deleted the SONARJS-862 branch January 31, 2017 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARJS-862 Rule: Cognitive Complexity of functions should not be too high #449

SONARJS-862 Rule: Cognitive Complexity of functions should not be too high #449

vilchik-elena commented Jan 19, 2017

ivandalbosco left a comment

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017

ivandalbosco Jan 30, 2017

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017 •

edited

Loading

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017 •

edited by vilchik-elena

Loading

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017 •

edited

Loading

ivandalbosco Jan 26, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017 •

edited

Loading

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017

ivandalbosco Jan 26, 2017

vilchik-elena Jan 30, 2017

SonarTech Jan 30, 2017

SonarTech commented Jan 30, 2017

ivandalbosco Jan 30, 2017 •

edited

Loading

vilchik-elena Jan 30, 2017

ivandalbosco Jan 30, 2017

SONARJS-862 Rule: Cognitive Complexity of functions should not be too high #449

SONARJS-862 Rule: Cognitive Complexity of functions should not be too high #449

Conversation

vilchik-elena commented Jan 19, 2017

ivandalbosco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilchik-elena Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivandalbosco Jan 26, 2017 • edited by vilchik-elena Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilchik-elena Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilchik-elena Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonarTech commented Jan 30, 2017

ivandalbosco Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilchik-elena Jan 30, 2017 •

edited

Loading

ivandalbosco Jan 26, 2017 •

edited by vilchik-elena

Loading

vilchik-elena Jan 30, 2017 •

edited

Loading

vilchik-elena Jan 30, 2017 •

edited

Loading

ivandalbosco Jan 30, 2017 •

edited

Loading