8

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.

schnatterer
  • 7,525
  • 7
  • 61
  • 80
gopalsaob
  • 81
  • 1
  • 1
  • 4
  • 1
    I would have to agree with you. There really isn't anything inherently wrong with multiple returns. As you can see [here](https://programmers.stackexchange.com/questions/118703/where-did-the-notion-of-one-return-only-come-from), there are historical reasons behind the idea, but there shouldn't be anything wrong with their use in a more modern language. However, I don't know how to change the calculation method, so this will remain a comment. – awksp May 22 '14 at 09:38

4 Answers4

9

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;
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
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.

matt freake
  • 4,877
  • 4
  • 27
  • 56
  • 1
    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. – Hari Krishna Gaddipati Dec 18 '19 at 14:52
  • @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 – matt freake Dec 24 '19 at 01:14
2

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.

Paul Johnson
  • 17,438
  • 3
  • 42
  • 59
2

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.

perfectionist
  • 4,256
  • 1
  • 23
  • 34