csharpsquid:S1541 - method complexity should not be 11

[oliver...@mindsatwork.com.br]

Hi,

I have a method that is similar to the method below, which does not make real sense.

CSharpsquid acuses the method of having a complexity of 11 (which is greater than the recomended 10), though the CC should be only 5.

Is this correct?

Is this mailing group the right place to ask for help on CSharp related matters?

Thanks,

Example method: 

public string CiclomaticComplexity(Object obj)

{

Type type = obj.GetType();

if (type != null)

{

return "(null)";

}

switch (type.ToString())

{

case "a":

return "type-a";

case "b":

return "type-b";

case "c":

return "type-c";

default:

return "unknown type";

}

}

[G. Ann Campbell]

Hi,

This is the right place to ask. :-)

The complexity score  starts at 1 for the method and is incremented for each logical branch and early return. So:

public string CiclomaticComplexity(Object obj)  // +1

{

Type type = obj.GetType();

if (type != null)                              // +1

{

return "(null)";                       // +1

}

switch (type.ToString())

{

case "a":                               // +1

return "type-a";             // +1

case "b":                               // +1

return "type-b";             // +1

case "c":                               // +1

return "type-c";             // +1

default:                                 // +1

return "unknown type"; // +1

}

}

HTH,

Ann

On Wednesday, 30 March 2016 12:42:41 UTC-4:

[Francis Galiegue]

Hello,

On Wednesday, March 30, 2016 at 7:48:14 PM UTC+2, G. Ann Campbell wrote:

Hi,

This is the right place to ask. :-)

The complexity score  starts at 1 for the method and is incremented for each logical branch and early return. So:

public string CiclomaticComplexity(Object obj)  // +1

{

Type type = obj.GetType();

if (type != null)                              // +1

{

return "(null)";                       // +1

}

switch (type.ToString())

{

case "a":                               // +1

return "type-a";             // +1

case "b":                               // +1

return "type-b";             // +1

case "c":                               // +1

return "type-c";             // +1

default:                                 // +1

return "unknown type"; // +1

}

}

OK, SonarQube has its definition of complexity, but I disagree on return being part of it.

The definition of complexity, as I see it, is the number of possible code paths within a method.

But "return" is not a code path; it _exits_ the method.

Consider this code, which I see waaaay too often:

----

public void someMethod()

{

    if (someCondition) {

        // big

        // chunk

        // of

        // code

        // here

    }

}

----

Let's say the complexity of this method is n.

The way I would always write such a method instead is as such:

----

public void someMethod()

{

    if (!someCondition)

        return;

    // big

    // chunk

    // of

    // code

    // here

}

----

Yet, due to the way SonarQube's Java plugin calculates the complexity, it would, artificially in this case, increment the complexity by 1... Even though the end code turns out to be more obvious.

Similarly, continue or break do not create other code paths; they just branch. Here as well, I disagree with both of them adding to complexity... Other code sample:

----

while (whatever) {

    if (someCondition) {

        // big

        // chunk

        // of

        // code

        // here

    }

}

----

Complexity n; contrast with:

----

while (whatever) {

    if (!someCondition)

        continue;

    // big

    // chunk

    // of

    // code

    // here

}

----

Again, to my eyes, this is an artificial increase of the complexity by 1.

Regards,

[oliver...@mindsatwork.com.br]

Thanks for the answer G. Ann.

Sorry for the delay answering, I hadn't checked the option to be updated on the topic.

I tested the same code on Java's equivalent rule: squid:MethodCyclomaticComplexity, with the default threshold of 10

The following code did not fail the rule. Is there a difference in the concept for Java and C#? 

public String cyclomaticComplexityTest( final Object obj )

{

final Class clazz = obj.getClass();

if ( clazz == null )

{

return "(null)";

}

switch ( clazz.toString() )

{

case "a" :

return "type-a";

case "b" :

return "type-b";

case "c" :

return "type-c";

default :

return "unknown type";

}

}

Thanks,

[G. Ann Campbell]

This might help: http://docs.sonarqube.org/display/SONAR/Metrics+-+Complexity

The rules should be using the same formulas as the metric calculations.

---

G. Ann CAMPBELL | SonarSource

Product Owner

http://sonarsource.com

--
You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/FVoRAr4QlXU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/7a8e4c94-32a0-4b84-801b-0bfd3d9023ff%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[oliver...@mindsatwork.com.br]

Thanks for the reference. I hadn't read it.

Sorry to be a bit of a bother on this issue, but in the Java section of the provided link it states that the 'default' keyword does not increment the complexity.

Is the complexity for C# and Java calculated differently on this matter?

If the last statement is a return statement it also should not be counted, although this is more of a gray area.

Would the following code result in a lower complexity ?

public String cyclomaticComplexityTest( final Object obj )

{

final Class clazz = obj.getClass();

if ( clazz == null )

{

return "(null)";

}

switch ( clazz.toString() )

{

case "a" :

return "type-a";

case "b" :

return "type-b";

case "c" :

return "type-c";

default :

// does nothing

}

                return "unknown type";

}

What I find weird is that the C# and Java complexity calculators are showing different behaviors on a code segment that is quite similar between the two languages.

[Jean-Louis Letouzey]

Hi all,

It appears that in some languages, the Sonar implementation of Cyclomatic Complexity measure is wrong.

The exact definition of this metric is in a NIST (National Institut of Standards for Technology) document:
            "NIST Special Publication 500-235
            Structured Testing: A Testing Methodology Using the Cyclomatic Complexity Metric"

In short: As Ann mentionned, it starts from 1, it is incremented with each logical branch (if, while, case..) and each "and" and "or" within a decision.
That's the way to count the number of lineary independant paths into the code.
A return statement, a continue .... does not increment the measure

if ( A )                              // +1      in fact 1 conditional jump
if (A && B )                      // +2      in fact 2 conditional jumps
C == A && B                   // +0 !     just a calculation, no jump

You like it or not, that's the definition !


About the continue instruction: a return does not add a testing path. It may impact the calculation of the essential cyclomatic complexity which is another completly different measure)

I hope, this remark will help Sonar to provide more precise and more consistent measure across languages.

Thanks

Regards

Jean-Louis

[G. Ann Campbell]

FYI, I've updated the metric definition to remove the word "cyclomatic".

Ann

[battl...@gmx.de]

Hi G. Ann,

thanks for clarifying the Sonar definitions.

But then, how can I actually check against cyclomatic complexity? After all, that often is considered as the "original complexity measure".
Like Francis and Jean-Louis, in my team the "SonarSource" complexity (including return and throw[Java]) is strongly disagreed with!


So what are my options given the situation:

• we consider cyclomatic complexity as important to check
• "SonarSource complexity" is not appropriate
• the FxCop rule "CA1502: Avoid excessive complexity" also cannot be used, because of fixed limit 25

?


Any suggestion greatly appreciated!

[G. Ann Campbell]

We recognize this is an issue, and we've talked about addressing it. Its just simply not a priority right now.

I know that's not what you want to hear, but it's the truth.

Ann

[oliver...@mindsatwork.com.br]

I understand that the way sonar calculates complexity has it's own standard.

What seems to be an issue is that very similiar code in Java and C# generate different complexity values.

G. Ann, the explanation you gave about the way sonar handles the complexity would not make sense for Java since the Java rule should not increment complexity on the default keyword.

[p3p...@gmail.com]

All,

This topic triggers me because it is something that has left me wondering for quite some time as well.

The complexity metric as it is implemented in the Java and C# plugins for SonarQube is pretty well documented and the source code is available at github for further inspection.

I have always wondered why the SonarQube implementation deviated from the original definition by McCabe.

Thank you Jean-Louis for confirming the issue and the reference to the NIST definition :)

Allow me to add some feedback:

For me as a user it is not desirable at all for code quality tools to defer from well-known and well-documented metrics or invent new variations of the latter.

I sincerely hope SonarQube will keep embracing existing quality standards and definitions (f.e. ISO 25010) and support well known metrics, such as cyclomatic complexity and fan-in (currently no design metrics available in SQ :( but that's a whole other topic). By doing so will allow users to not only use SonarQube to fix the leak but also aid in monitoring compliance of projects to existing code quality standards (f.e. TUViT, IfSQ, A2DAM, etc) that are often referred to in formal contracts with software vendors. Deviating from well understood software metric definitions makes SonarQube less useful for monitoring compliance to these metrics.

Regards,

Pepijn

Op donderdag 28 april 2016 14:20:59 UTC+2 schreef Jean-Louis Letouzey:

[p3p...@gmail.com]

Op vrijdag 29 april 2016 12:02:11 UTC+2 schreef battl...@gmx.de:

So what are my options given the situation:

• we consider cyclomatic complexity as important to check
• "SonarSource complexity" is not appropriate
• the FxCop rule "CA1502: Avoid excessive complexity" also cannot be used, because of fixed limit 25

?


Any suggestion greatly appreciated!

I can suggest the SIGGuidelines.Analyzers nuget package that I recently published, which includes an implementation of the cyclomatic complexity: https://github.com/p3pijn/SIGGuidelines.Analyzers 

The SonarQube Roslyn SDK allows you to generate a SonarQube plugin for this analyzer (https://github.com/SonarSource-VisualStudio/sonarqube-roslyn-sdk).

[Brian Sperlongano]

Hello!

Sorry to revive an old post, but I think this is an important semantic.

I ran into this issue in some code I was working on. (https://sonarqube.com/dashboard/index?id=com.sperlongano%3Aautobuilder%3Asrc%2Fmain%2Fjava%2Fcom%2Fsperlongano%2Fautobuilder%2FBeanInvocationHandler.java), which has a McCabe complexity of 6 and a SonarQube complexity of 12!  Under the McCabe standard, this code is within ideal complexity bounds but is a finding according to SQ.

The SonarQube definition of complexity does not make sense because it double-counts early returns and all throws.

Consider two essentially identical pieces of code:

public int positive(int x) {

  if(x < 0) {

    return -x;

  }

  return x;

}

public int positive(int x) {

  if(x < 0) {

    x = -x;

  }

  return x;

}

The first one has a SQ complexity of 3 and the second has a SQ complexity of 2.  Both have a McCabe complexity of 2 because there are two possible paths this method can take -- which is the thing that complexity measures.

The default threshold of "10" comes from both NIST and McCabe which both recommended 10 as the ideal upper bound for complexity before a method should be refactored.  Refactoring a method with several early returns into the form with a local return value variable can encourage developers to write less-efficient code in order to "get around" the SonarQube definition.

It does not make any sense that SQ uses their own definition that inflates complexity but with the threshold intended for the NIST/McCabe definition!

The fix for this is very straightforward - do not count throws or early returns in the complexity metric.

I realize this may be low on the priority list, but if it's recognized as an issue, could we at least get a JIRA ticket put up for it?

Thanks!

Brian

[Freddy Mallet]

You're right Brian, SQ is currently computing a complexity based on a mix of cyclomatic and essential complexity and this is highly misleading. Here is the relating ticket on this subject : https://jira.sonarsource.com/browse/MMF-151. 

And be sure that this ticket has not a low priority on our side. Our main concern is that even by sticking to the cyclomatic complexity definition, we'll generate some FP/FN (at least perceived as-is by the community) on switch statements, lambdas, exception handling, setter/getter ... So for us this ticket is a good opportunity to think twice about those corner cases.

Thanks again for your feedback

Freddy mallet

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/14e0aece-35dd-4608-bf9d-3db2f050b5bb%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Freddy MALLET | SonarSource

Product Director & Co-Founder
http://sonarsource.com

[battl...@gmx.de]

Great to see that topic gaining a little momentum again!
As written above, at our company we never have been happy with the current mode of calculation. So if there would be revised definitions/implementations, we would love it :-)


If I may add my 2 cents on the corner case discussion at https://jira.sonarsource.com/browse/MMF-151: I would disagree on the Feature Description, and vote for exactly counting "real" conditional branches in the control flow (+1 for default path).
I.e. for Java, count

• 'if', 'for', 'while', 'for'
• '&&', '||', 'ternary operator'
• 'case'
• 'catch' (effectively conditional branch: if (exception) { MyException e = <caught>; ... }   !)

and ignore

• try, switch, do (no branching, only 'syntactic context-setting')
• default, return, break, throw, finally (unconditional 'branches' do not really split up control flow, do they?)
• declarations of new anonymous types/objects (actually only some data being handed over)

I must admit, I am a little unsure about higher order functions like 'forEach', 'filter'...
But I would tend to ignore them as well, because

• strictly speaking a call of 'filter' etc. still is just a call (and no decision / CFG branch)
• one of their major benefits exactly is that: you can delegate branching, so yourMethod() does not have to deal with it

Class/file level complexity of course is another topic.
I would prefer exposing the choice of counting getters+setters (also implicit ones like with C# properties) as a rule parameter,
And then count ALL declared methods in the class/file (no depth limit, i.e. including nested and anonymous local classes, and lambdas).

What do you think?

Cheers,
Klaus

[G. Ann Campbell]

Hi Klaus,

We'll have details for you soon.

Thx,

Ann

---

G. Ann CAMPBELL | SonarSource

Product Owner

http://sonarsource.com

--

You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/FVoRAr4QlXU/unsubscribe.

To unsubscribe from this group and all its topics, send an email to sonarqube+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/3fb15303-9a76-47dc-9f5d-e7eb443046c8%40googlegroups.com.

[Brian Sperlongano]

I took a poke into the code base, at ComplexityVisitor.java and it looks like if we remove 3 lines, we should get very close to the correct branch count.

  @Override

  public List<Tree.Kind> nodesToVisit() {

    return ImmutableList.<Tree.Kind>builder()

        .add(Tree.Kind.METHOD)

        .add(Tree.Kind.CONSTRUCTOR)

        .add(Tree.Kind.IF_STATEMENT)

        .add(Tree.Kind.FOR_STATEMENT)

        .add(Tree.Kind.FOR_EACH_STATEMENT)

        .add(Tree.Kind.DO_STATEMENT)          //Remove this line

        .add(Tree.Kind.WHILE_STATEMENT)

        .add(Tree.Kind.RETURN_STATEMENT)  //Remove this line

        .add(Tree.Kind.THROW_STATEMENT)   //Remove this line

        .add(Tree.Kind.CASE_LABEL)

        .add(Tree.Kind.CATCH)

        .add(Tree.Kind.CONDITIONAL_EXPRESSION)

        .add(Tree.Kind.CONDITIONAL_AND)

        .add(Tree.Kind.CONDITIONAL_OR)

        .build();

  }

[G. Ann Campbell]

Hi Brian,

We're going to address this in all our supported languages. You can watch MMF-151 for progress.

Ann