Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Early return statements and cyclomatic complexity

I prefer this writing style with early returns:

public static Type classify(int a, int b, int c) {
    if (!isTriangle(a, b, c)) {
        return Type.INVALID;
    }
    if (a == b && b == c) {
        return Type.EQUILATERAL;
    }
    if (b == c || a == b || c == a) {
        return Type.ISOSCELES;
    }
    return Type.SCALENE;
}

Unfortunately, every return statement increases the cyclomatic complexity metric calculated by Sonar. Consider this alternative:

public static Type classify(int a, int b, int c) {
    final Type result;
    if (!isTriangle(a, b, c)) {
        result = Type.INVALID;
    } else if (a == b && b == c) {
        result = Type.EQUILATERAL;
    } else if (b == c || a == b || c == a) {
        result = Type.ISOSCELES;
    } else {
        result = Type.SCALENE;
    }
    return result;
}

The cyclomatic complexity of this latter approach reported by Sonar is lower than the first, by 3. I have been told that this might be the result of a wrong implementation of the CC metrics. Or is Sonar correct, and this is really better? These related questions seem to disagree with that:

https://softwareengineering.stackexchange.com/questions/118703/where-did-the-notion-of-one-return-only-come-from

https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

If I add support for a few more triangle types, the return statements will add up to make a significant difference in the metric and cause a Sonar violation. I don't want to stick a // NOSONAR on the method, as that might mask other problems by new features/bugs added to the method in the future. So I use the second version, even though I don't really like it. Is there a better way to handle the situation?

like image 555
janos Avatar asked Sep 30 '22 21:09

janos


2 Answers

Your question relates to https://jira.codehaus.org/browse/SONAR-4857. For the time being all SonarQube analysers are mixing the cyclomatic complexity and essential complexity. From a theoretical point of view return statement should not increment the cc and this change is going to happen in the SQ ecosystem.

like image 86
Freddy - SonarSource Team Avatar answered Oct 05 '22 07:10

Freddy - SonarSource Team


Not really an answer, but way too long for a comment.

This SONAR rule seems to be thoroughly broken. You could rewrite

b == c || a == b || c == a

as

b == c | a == b | c == a

and gain two points in this strange game (and maybe even some speed as branching is expensive; but this is on the discretion of the JITc, anyway).

The old rule claims, that the cyclomatic complexity is related to the number of tests. The new one doesn't, and that's a good thing as obviously the number of meaningfull tests for your both snippets is exactly the same.

Is there a better way to handle the situation?

Actually, I do have an answer: For each early return use | instead of || once. :D

Now seriously: There is a bug requesting annotations allowing to disable a single rule, which is marked as fixed. I din't look any further.

like image 33
maaartinus Avatar answered Oct 05 '22 08:10

maaartinus