Sonar cyclomatic complexity rule issue - discourages multiple return statements

Question

For the following piece of code, sonarqube computes the method cyclomatic complexity as 9

String foo() {
    if (cond1) return a;
    if (cond2) return b;
    if (cond3) return c;
    if (cond4) return d;
    return e;
}

I understand as per the rules for computation http://docs.sonarqube.org/display/SONAR/Metrics+-+Complexity the complexity of 9 is correct. So complexity of the method is = 4 (if) + 4 (return) + 1 (method) = 9

This complexity can be reduced, if I have a single exit point.

String foo() {
    String temp;
    if (cond1) { 
        temp = a;
    } else if (cond2) {
        temp = b;
    } else if (cond3) { 
        temp = c;
    } else if (cond4) {
        temp = d;
    } else {
        temp = e;
    }
    return temp;
}

I believe this code is more cluttered and unreadable than the previous version and I feel having methods with return on guard conditions is a better programming practice. So is there a good reason why return statement is considered for computation of cyclomatic complexity? Can the logic for computation be changed so that it doesn't promote single exit point.

Solution 1

I agree you should use some common sense and go with the code which you believe is simplest.

BTW You can simplify you code and have just one return if you use ? :

String foo() {
    return cond1 ? a :
           cond2 ? b :
           cond3 ? c :
           cond4 ? d : e;
}

Solution 2

"So is there a good reason why return statement is considered for computation of cyclomatic complexity? Can the logic for computation be changed so that it doesn't promote single exit point."

In your example having multiple returns doesn't add to the complexity and as @Peter Lawrey says you should employ common sense.

Does this mean that all examples of multiple return statements do not to complexity and it should be removed? I don't think so. If would be very easy to come up with an example of a method which is hard-to-read because of multiple return statements. Just imagine a 100 line method with 4 different return statement sprinkled throughout. That is the kind of issue this rules tries to catch.

Solution 3

Other answers have made good points about the computation involved.

I'd like to point out that your assertion that the code is less readable is false, because in one instance you have braces, and in the other you don't.

String foo() {
  String output = e;
  if (cond1) output = a;
  else if (cond2) output = b;
  else if (cond3) output = c;
  else if (cond4) output = d;

  return output;
}

This is as readable as the example you gave with return statements. Whether or not you allow braceless if statements is a question of style that you should probably be consistent with across all your code.

The more important issue that cyclomatic complexity does address is that if computing the value of cond1, cond2 etc have side effects, i.e. if they were a stateful method rather than a field in this case, then the conceptual complexity of the code is much higher if you might return early compared to if you can't.

Solution 4

This is a known problem with cyclomatic complexity.

Also there is good reason to think that cyclomatic complexity is useless. It correlates strongly with SLOC and only weakly with actual bugs. In fact SLOC is just as good a predictor of defects as cyclomatic complexity. The same goes for most other complexity metrics.

See http://www.leshatton.org/Documents/TAIC2008-29-08-2008.pdf, starting around slide 16.

View more solutions

Comments

[Henrik Kjus Alstad]

Correct me if im wrong, but this isn't equivialent. Both of the original examples would stop comparing if cond1 == true. Your example would compare cond2,cond3, and cond4, even if cond1 == true

[perfectionist]

@HenrikKjusAlstad - good catch, I forgot the "else"s. Fixed.

[Greg Pelcha]

This doesn't solve the issue of complexity, this is only code aesthetics.

[Hari Krishna Gaddipati]

4 return statements in 100 lines of code is a problem for sure. But 100 lines of code is the first thing I would sort instead of 4 return statements.

[matt freake]

@Hari you are 100% right. 6 years ago I worked on a legacy code base where 100 line methods were not uncommon. Now I'd be refactoring the hell out of then