High cyclomatic complexity on switch statements #840

Open
jncraton opened this Issue Jan 31, 2013 · 35 comments

Comments

Projects
None yet
9 participants

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.

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

Contributor

WolfgangKluge commented Jul 23, 2013

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.

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!

Contributor

WolfgangKluge commented Jul 23, 2013

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

Contributor

WolfgangKluge commented Jul 23, 2013

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

Contributor

WolfgangKluge commented Jul 23, 2013

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.

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

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

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

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

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.

Contributor

WolfgangKluge commented Jul 26, 2013

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

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

Contributor

mikesherov commented Aug 9, 2013

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.

Contributor

mikesherov commented Aug 9, 2013

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;}

Contributor

WolfgangKluge commented Aug 9, 2013

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
Contributor

mikesherov commented Aug 9, 2013

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.

Contributor

mikesherov commented Aug 9, 2013

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.

Owner

rwaldron commented Dec 4, 2014

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"

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 ?

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.

Owner

rwaldron commented Feb 14, 2015

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

Contributor

mikesherov commented Feb 14, 2015

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

Owner

rwaldron commented Feb 14, 2015

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.

Contributor

mikesherov commented Feb 14, 2015

Lol. that's like every day for me.

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.

Contributor

mikesherov commented Feb 15, 2015

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.

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.

@lukeapage lukeapage added P3 Bug labels Jun 15, 2015

lukeapage removed the rule Issue label Jun 28, 2015

Contributor

paul-wade commented Jul 28, 2015

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.

Contributor

paul-wade commented Jul 29, 2015

Thanks @lukeapage I missed that

@lukeapage lukeapage added a commit to lukeapage/jshint that referenced this issue Jul 29, 2015

@lukeapage lukeapage [[FIX]] maxcomplexity doesn't take into account `&&`
Fixes #840
047d5af

lukeapage added the Has PR label Jul 29, 2015

@paul-wade paul-wade pushed a commit to paul-wade/jshint that referenced this issue Aug 8, 2015

paul-wade [[fix]] case statments with no code should not increase complexity #840 3f0f807
Contributor

paul-wade commented Aug 8, 2015

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

@paul-wade paul-wade pushed a commit to paul-wade/jshint that referenced this issue Aug 8, 2015

paul-wade [[fix]] case statments with no code should not increase complexity #840 8582d11

@paul-wade paul-wade pushed a commit to paul-wade/jshint that referenced this issue Aug 8, 2015

paul-wade [[fix]] case statments with no code should not increase complexity #840 df0bddc

@paul-wade paul-wade pushed a commit to paul-wade/jshint that referenced this issue Aug 8, 2015

paul-wade [[fix]] case statments with no code should not increase complexity #840 184476b

@paul-wade paul-wade pushed a commit to paul-wade/jshint that referenced this issue Aug 8, 2015

paul-wade [[fix]] case statments with no code should not increase complexity #840 6e74b0c
Member

lukeapage commented Aug 8, 2015

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]

Contributor

paul-wade commented Aug 8, 2015

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?

Member

lukeapage commented Aug 8, 2015

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.

Member

lukeapage commented Aug 8, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment