High cyclomatic complexity on switch statements

[jncraton]

Currently, every case in a switch block increments the cyclomatic complexity regardless of whether or not it falls through.

This has a complexity of 4:

function(someVal) {
    switch (someVal) {
        case 1:
        case 2:
        case 3:
            doSomething();
            break;
        default:
            doSomethingElse();
            break;
    }
}

This has a complexity of 2 while being logically equivalent:

function(someVal) {
    if (someVal === 1 || someVal === 2 || someVal === 3) {
        doSomething();
    } else {
        doSomethingElse();
    }
}

I'm not an expert in cyclomatic complexity by any means, so I'm honestly not sure if this is correct or not. Even if this is the expected behavior, it seems like it would be nice to at least have a flag which could make these two function calculate the same cyclomatic complexity.

[WonderBeat]

Am I right, that both variants are equal? CC for the first case should be 2.

[WolfgangKluge]

I'm not an expert at all, but as I understand cyclomatic complexity, you have to count any individual path of the control flow graph of a function. And || creates a new path (not necessarily individual, but new).
Thus, I think we have to increase the latter functions complexity instead of decreasing the switch ones.

[WonderBeat]

I'm not an expert either. My point is: cyclomatic complexyty counts different flows through a method. And it's usefull, because you can understand, how many tests should be written for this method. So, || should not increase it. I've asked this question to my coleagues =), they agreed. Let's investigate this issue!

[WolfgangKluge]

Hi, I'm still not an expert ;) But I can't agree with the knowledge so far.
As I know, cyclomatic complexity is not always human intuitive.
E.g. a function with a body like

switch(x) {
    case 1: return "a";
    case 2: return "b";
    case 3: return "c";
    case 4: return "d";
    case 5: return "e";
    case 6: return "f";
  }
  return "other";

has a complexity of 7 while a function with this body

var retValues = ["a", "b", "c", "d", "e", "f"];
if (x > 0 && x <= 6) {
    return retValues[x - 1];
}
return "other"

only get's 3 as it's complexity value - even if the first version looks easier to read (simply think of a version with n more entries - while the complexity of the first function is 7+n, the second one remains 3).

---

According to http://gmetrics.sourceforge.net/gmetrics-CyclomaticComplexityMetric.html, the analysis is pretty easy

Start with a initial (default) value of one (1). Add one (1) for each occurrence of each of the following:

• if statement
• while statement
• for statement
• case statement
• catch statement
• && and || boolean operations
• ?: ternary operator ...

If this is true (for Java), I think we should not add more complexity to this system by adding some exceptions.

I only just started to read http://www.literateprogramming.com/mccabe.pdf, but this fits to my understanding of "maximum number of linearly independent circuits" in the control flow graph... Simply try to draw such a graph for your function..

[WolfgangKluge]

According to this list, both of your functions should have a complexity of 4 (what's comparable).

[WolfgangKluge]

I just found another source (http://www.verifysoft.com/en_mccabe_metrics.html) that describes the analysis this way:

Start with 1. Add 1 for each occurence of each of the following:

• if statement
• while statement
• for statement
• case statement, if not empty
• catch statement
• && and || boolean operations (as stated later in the text: "In summary, ...")
• ?: ternary operator

While this sounds quite intuitive in the first place, I can't agree with the explanation: "Case branch does not increase v(G), because it does not increase the number of branches in the control flow". This is simply not true.

Also && and || increases the complexity number, but an empty branch (which behaves the very same as ||) will not. Feels a bit rash.

[WonderBeat]

Thank's. That's an interesting article.
I'm going to check this examples in checkstyle [JAVA] and post results here.

[WonderBeat]

Here are results:
https://gist.github.com/WonderBeat/6078981

So, you were right.
Will send you another pull request ;)

[WonderBeat]

Oh. Strange things =). Last commit.
Tests were implemented. JSHint logic is not modified

[WonderBeat]

Hey. Take a look.
http://www.verifysoft.com/en_mccabe_metrics.html
If there are two or more case ...: parts that have no code in between, the McCabe measure is increased only with one for all those case ...: parts.

[WolfgangKluge]

If there are two or more case ...: parts that have no code in between, the McCabe measure is increased only with one for all those case ...: parts.

As stated above, I can't follow the argumentation of verifysoft. Especially because they write

In summary, the following language constructs increase the cyclomatic number by one: if (...), for (...), while (...), case ...:, catch (...), &&, ||, ?, #if, #ifdef, #ifndef, #elif.

So || will always increase the complexity, but empty case blocks won't (at least not always)... That's incoherent (but that's only my opinion 😉 ).

[WonderBeat]

Ringht. I'm reading McCabe PDF with Fortran examples =). The truth is out there.

[mikesherov]

The original cyclomatic complexity counted the number of decision points, which exclude && and ||. The concept of extended cyclomatic complexity includes Boolean operators to attempt to also convey the complexity of the decisions themselves.

The question here isn't which one is right, but rather which one JSHint is attempting to follow. Perhaps both?

Google "extended cyclomatic complexity" for verification.

[mikesherov]

Just to clarify my previous comment, cases where && and || do truly constitute a decision (e.g. assignment: var x = y || z) should increase cyclomatic complexity, because it can be rewritten as: var x; if(y) {x=y;} else {x=z;}

[WolfgangKluge]

Can you please name one case, where && or || is not part of a decision point?
I think you can rewrite every logical operator to an appropriate if/else statement.

var x = y && z;
// goes to
var x;
if (y == z) {
    x = true;
} else {
    x = false;
}



if (x && y) {
} else {
}
// goes to
if (x) {
    if (y) {
    } else {
        // see below
    }
} else {
   // same as in the above else branch
}



while (x && y) {
}
// goes to
while (true) {
  if (!x) break;
  if (!y) break;
}
// or something similar

[mikesherov]

Fair enough. The language that cyclomatic complexity was original targeted to analyze didn't have a pervasive "default assign" pattern. You can exclude the caveat about && and ||.

In general, original cyclomatic complexity meant the number of branching code paths... the number of blocks that get executed as a result of a conditional split.

[mikesherov]

Well, actually no. I take it back again, I'm confusing myself. The reason I made that caveat is that the assignment that takes place is due to the side effects of the conditional. Original cyclomatic complexity talks about the number of control flow paths: http://testingwarrior.blogspot.com/2011/12/cyclomatic-complexity-with-example.html

If your boolean && and || are used simply for creating a new branch (evaluating the boolean value of a conditional), they don't count towards cyclomatic complexity. If they then implicitly consistute the body of two paths, they do count.

[rwaldron]

I don't see the point in that example, it definitely doesn't do what the console log's imply:

function f(someVal) {
  switch (someVal) {
    case 1 || 2 || 3: // <-- this evaluates to `1`
      console.log("1 to 3")
      break;
    default:
      console.log("Other")
      break;
  }
}

f(1); 
// "1 to 3"

f(2); 
// "Other"

[daemon1981]

Why && doesn't increase cyclomatic complexity count ?

function functionWithCyclomaticComplexityDueToAndOperator_1(a, b) {
  return a && b;
}
// => jshint return complexity 1

function functionWithCyclomaticComplexityDueToOrOperator_2(a, b) {
  return a || b;
}
// => jshint return complexity 2

function functionWithCyclomaticComplexityDueToNegativeAndOperator_1(a, b) {
  return !(!a && !b);
}
// => jshint return complexity 1

Looks strange...

I can increase the complexity of the code without be warned ?

[truhlikfredy]

I don't know how exactly it's working but let me guess it's because the analysis is done on bytecode and not source? So it's after compiling and the last statement got optimized by compiler into the first one.

[rwaldron]

The function with && has only one path, as both sides of && are always evaluated. || has two paths: the left side lone evaluation and the left + right evaluation

[mikesherov]

@rwaldron, sounds like a bug to me. && short circuits when left is false, just like || short circuits when left is true. No?

[rwaldron]

Yes, and for future reference I should avoid commenting or reviewing issues on days that I'm playing video games or otherwise not focusing on the subject at hand.

[mikesherov]

Lol. that's like every day for me.

[truhlikfredy]

So there two different issues?

• The short circuit for && is ignored and gives smaller complexity than it really is
• The code can get more complex looking and the source can get harder to read and easier to make a bug inside it. But if it's somewhere optimized then it will not increase the complexity in report.

BTW:Sorry for reviving old thread.

[mikesherov]

The code can get more complex looking and the source can get harder to read and easier to make a bug inside it. But if it's somewhere optimized then it will not increase the complexity in report.

No, you just made this up!

This is just a bug like any other in the source code. A condition that should be increasing complexity isn't.

[truhlikfredy]

Ok never mind.. I said in beginning I don't know how this one exactly works and the whole thread is a bit confusing.

So for !(!a && !b) and a && b should be given 2.

[paul-wade]

is cyclomatic complexity worthy of its own unit test file? Or should it just be a part of core tests... I could be wrong but I'm not seeing any current tests around it.

[lukeapage]

@paul-wade
The tests are in the options file.

jshint/tests/unit/options.js

Line 2089 in 9d021ee

exports.maxcomplexity = function (test) {

[paul-wade]

Thanks @lukeapage I missed that

[paul-wade]

@lukeapage The original issues is in regards to switch statements. Does your pull request handle just && or both?

[lukeapage]

if you read through this issue carefully i think the conclusion was that either
a. case, & &, || should not increase complexity
b. they all should
e. g. it should be consistent
also i think people preferred b. because we already increase complexity for ||, case and because with b, if you rewrite code to do the same thing, using different syntax, the complexity won't change.
[edit correct a=>b]

[paul-wade]

I'm not sure logical consistency is the right thinking here. Logically sure || and a few fall through case statements should be the same. In terms of maintaining code would you find it easier to read a switch with a few fall through case statements or an if with a bunch of ||. I'd also argue that visual studio, code maid, reshaper, intellij consistently do not increase complexity for an empty case.

Isn't consistency with other languages/analyzers also important?

[lukeapage]

If you search around about cyclomatic complexity you will see some implement OR/empty switch cases as increasing complexity and some don't. But they are always consistent because they are equivalent code.

It is highly likely those c# tools are showing info from the same source. Are you sure they are increasing complexity for || but not for empty case statements?

JavaScript is a different language from c# and I've found code measuring PHP that does count OR - so it may also be a microsoft convention - none of which are reasons for us to go one way or another.

JSHint has included OR and empty switch for a long time, so there isn't really a reason to change that I see.

[lukeapage]

interestingly http://jscomplexity.org/ has the exact same behaviour as jshint at the moment (+1 for ||, +1 for empty switch, +0 for &&) though it is an option.

[gustavorps]

Bountysource decided to update their Terms of Service:

2.13 Bounty Time-Out.
If no Solution is accepted within two years after a Bounty is posted, then the Bounty will be withdrawn and the amount posted for the Bounty will be retained by Bountysource. For Bounties posted before June 30, 2018, the Backer may redeploy their Bounty to a new Issue by contacting support@bountysource.com before July 1, 2020. If the Backer does not redeploy their Bounty by the deadline, the Bounty will be withdrawn and the amount posted for the Bounty will be retained by Bountysource.

[jugglinmike]

Thanks for the heads-up, Gustavo! It would be a shame for that money to go back to Bountysource, especially when Luke contributed a fix almost five years ago. I've merged the fix, resolving the conflicts that have been introduced in a half-decade of development.

@lukeapage: your $25 is waiting.