Cyclomatic Complexity Calculation Options

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Cyclomatic Complexity Calculation Options

James.Watson
I'm trying out Sonar and I'm getting hung up on the how cyclomatic complexity it being calculated.  I think this is a useful metric but I strongly disagree with how it's calculated.

For example, I do not want to count returns (other than the last) towards cyclomatic complexity.  Is there an easy way to modify the way that cyclomatic complexity is calculated or are there other algorithms to choose from?

I believe you can develop your own checks but I'm wondering if there's an quicker way specifically for this check.

thanks,

-Matt
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Patroklos Papapetrou
Hi Matt

Interesting point of view :)
AFAIK you can't modify how complexity is calculated,but why you'd want something like this?
What I mean is that Cyclomatic Complexity ( MaCaBe metric ) is defined years ago and it has a somehow a standard method of computation.



2013/10/17 James.Watson <[hidden email]>
I'm trying out Sonar and I'm getting hung up on the how cyclomatic complexity
it being calculated.  I think this is a useful metric but I strongly
disagree with how it's calculated.

For example, I do not want to count returns (other than the last) towards
cyclomatic complexity.  Is there an easy way to modify the way that
cyclomatic complexity is calculated or are there other algorithms to choose
from?

I believe you can develop your own checks but I'm wondering if there's an
quicker way specifically for this check.

thanks,

-Matt



--
View this message in context: http://sonarqube.15.x6.nabble.com/Cyclomatic-Complexity-Calculation-Options-tp5018064.html
Sent from the SonarQube Users mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email



Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

James.Watson
Patroklos-

  Thanks,  I figured you couldn't easily modify it.

  So based on the wikipedia page (feel free to provide a different reference) cyclomatic complexity is a measure of the number of linearly independent paths through a program's source code.  Consider the following trivial methods:

boolean exampleA(int input)
{
  if (input == 0) {
    return false;
  } else if (input < 0) {
    return check(input * -1);
  } else {
    return check(input);
  }
}
 
boolean exampleB(int input)
{
  boolean retval;
   
  if (input == 0) {
    retval = false;
  } else if (input < 0) {
    retval = check(input * -1);
  } else {
    retval = check(input);
  }
   
  return retval;
}

Unless I've made a mistake, these are equivalent functions both semantically and in execution (aside from B's extraneous variable assignments and accesses).  It's pretty clear that both have exactly three linearly independent paths through the source.

However, based on my understanding of how cyclometric complexity is being calculated in Sonar, the first gets a score of 6 while the second gets a score of 4.  The real problem for me is that I think the first example is far superior.  Mainly because it's shorter and easier to reason about.  For example, if I want to know what the method returns when the input equals 0, I know that simply by looking at the first 2 lines of the method.  The second version requires that I read and comprehend every line of the code of the entire method to be sure I understand how is executes.

The second version is also inferior because it introduces the possibility of subtle bugs.  For example, if the 'else if' were replaced by if, the first version is robust in that the result is unchanged.  The second version is not robust to this change.  Another common problem I find is that people will often (unnecessarily) assign a default value to their return variable this allows for some paths of execution to not have a return value.  If you use the first style, the compiler will not allow this.

The upshot is that extra returns cannot add extra paths to source code.  In practice, treating them as extra paths that should be eliminated encourages what I consider to be bad practice.  The belief that having only one return statement is a good practice is common but I think it is very misguided in contemporary programming languages.   Moreover, a good metric measures only what it claims to measure and not other things.
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

James.Watson
On a side note, in the wiki page Cyclomatic complexity it contains this:

Cyclomatic complexity may be extended to a program with multiple exit points; in this case it is equal to:

π - s + 2

where π is the number of decision points in the program, and s is the number of exit points.[3][4]

[3] a b Belzer, Kent, Holzman and Williams (1992). Encyclopedia of Computer Science and Technology. CRC Press. pp. 367–368.
[4] Harrison (October 1984). "Applying Mccabe's complexity measure to multiple-exit programs". Software: Practice and Experience (J Wiley & Sons).
Based on that definition, extra returns should actually reduce the cyclomatic complexity of the function.

I any event, this is not nearly as cut-and-dried as it might seem and I think it would be nice to be have some options around how this calculation is made given how useful it can be.  I'm not really concerned with forcing my opinions about this on anyone else, I'm just looking to make the tool work for me and my team.
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Keith Barlow
What you point out is an interesting case...  the only thing I could say is that the numbers are merely supposed to be guidelines.  4 vs 6 is really not much of an indication of anything.  20 vs 10 or 50 vs 15 is much more indicative of problematic areas.  I wouldn't read too much into single point discrepancies.  Also, the complexity does not tell the full story by itself.  This number should be evaluated in context of other numbers.  Response for class might be a good one to look at here.  I am curious to know what that picture looks like though, if I understand the way it's calculate, I suspect it might look similar to the complexity discrepancy your observing.

Overall... numbers should only be used as a guideline to identify problematic areas, you still need to use your personal judgement in evaluating the actual application.  The classic example of this is with LCOM4, while high LCOM4s are generally considered bad, there are perfectly acceptable patterns, such as static utility classes, which will have a high LCOM4 rating.

Keith


On Thu, Oct 17, 2013 at 6:08 PM, James.Watson <[hidden email]> wrote:
On a side note, in the wiki page  Cyclomatic complexity
<http://en.wikipedia.org/wiki/Cyclomatic_complexity>   it contains this:


> Cyclomatic complexity may be extended to a program with multiple exit
> points; in this case it is equal to:
>
> π - s + 2
>
> where π is the number of decision points in the program, and s is the
> number of exit points.[3][4]
>
> [3] a b Belzer, Kent, Holzman and Williams (1992). Encyclopedia of
> Computer Science and Technology. CRC Press. pp. 367–368.
> [4] Harrison (October 1984). "Applying Mccabe's complexity measure to
> multiple-exit programs". Software: Practice and Experience (J Wiley &
> Sons).

Based on that definition, extra returns should actually reduce the
cyclomatic complexity of the function.

I any event, this is not nearly as cut-and-dried as it might seem and I
think it would be nice to be have some options around how this calculation
is made given how useful it can be.  I'm not really concerned with forcing
my opinions about this on anyone else, I'm just looking to make the tool
work for me and my team.



--
View this message in context: http://sonarqube.15.x6.nabble.com/Cyclomatic-Complexity-Calculation-Options-tp5018064p5018073.html
Sent from the SonarQube Users mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email





--
Keith Barlow

Software Engineer
Dell Boomi
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

carchi
Well that true, but these are small functions. When we get in to larger function with 40-50 lines of code.

we could be seeing the differences you speak of. An example would be the following.

This should have a CCM of 13, in Sonar i got 25.

iboolean exampleA(int input)
{
  if (input == 0) {
    return false;
  } else if (input < 0) {
    return check(input * -1);
  } else if (input < 1) {
    return check(input * -1);
  } else if (input < 2) {
    return check(input * -1);
  } else if (input < 3) {
    return check(input * -1);
  } else if (input < 4) {
    return check(input * -1);
  } else if (input < 5) {
    return check(input * -1);
  } else if (input < 6) {
    return check(input * -1);
  } else if (input < 7) {
    return check(input * -1);
  } else if (input < 8) {
    return check(input * -1);
  } else if (input < 999999999) {
    return check(input * -1);
  } else if (input < 10) {
    return check(input * -1);
  } else {
    return check(input);
  }
}
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Ann Campbell
Please, please, please tell me that isn't an example pulled from your codebase.



---
G. Ann Campbell
Systems Architect II
Shaw Industries Inc,
201 S. Hamilton St.
Dalton Ga 30720


On Fri, Oct 18, 2013 at 1:19 PM, carchi <[hidden email]> wrote:
Well that true, but these are small functions. When we get in to larger
function with 40-50 lines of code.

we could be seeing the differences you speak of. An example would be the
following.

This should have a CCM of 13, in Sonar i got 25.

iboolean exampleA(int input)
{
  if (input == 0) {
    return false;
  } else if (input < 0) {
    return check(input * -1);
  } else if (input < 1) {
    return check(input * -1);
  } else if (input < 2) {
    return check(input * -1);
  } else if (input < 3) {
    return check(input * -1);
  } else if (input < 4) {
    return check(input * -1);
  } else if (input < 5) {
    return check(input * -1);
  } else if (input < 6) {
    return check(input * -1);
  } else if (input < 7) {
    return check(input * -1);
  } else if (input < 8) {
    return check(input * -1);
  } else if (input < 999999999) {
    return check(input * -1);
  } else if (input < 10) {
    return check(input * -1);
  } else {
    return check(input);
  }
}



--
View this message in context: http://sonarqube.15.x6.nabble.com/Cyclomatic-Complexity-Calculation-Options-tp5018064p5018123.html
Sent from the SonarQube Users mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email




**********************************************************
Privileged and/or confidential information may be contained in this message. If you are not the addressee indicated in this message (or are not responsible for delivery of this message to that person) , you may not copy or deliver this message to anyone. In such case, you should destroy this message and notify the sender by reply e-mail.
If you or your employer do not consent to Internet e-mail for messages of this kind, please advise the sender.
Shaw Industries does not provide or endorse any opinions, conclusions or other information in this message that do not relate to the official business of the company  or its subsidiaries.
**********************************************************

Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

carchi
In reply to this post by carchi
No it the above small example with the else if copied a couple time, and the int incremented.
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Java
CC can be really a pain in the bud. Always giving high scores to switch... case constructions.
However, best practise dictates having only one exit point. So in you example, a high CC seems correct.


-------- Ursprüngliche Nachricht --------
Von: carchi <[hidden email]>
Gesendet: Fri Oct 18 20:02:38 MESZ 2013
An: [hidden email]
Betreff: [sonar-user] Re: Cyclomatic Complexity Calculation Options

No it the above small example with the else if copied a couple time, and the
int incremented.



--
View this message in context: http://sonarqube.15.x6.nabble.com/Cyclomatic-Complexity-Calculation-Options-tp5018064p5018124.html
Sent from the SonarQube Users mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email




---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


tux
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

tux
In reply to this post by carchi
On Fri, 18 Oct 2013 10:19:06 -0700 (PDT), carchi <[hidden email]>
wrote:

> Well that true, but these are small functions. When we get in to larger
> function with 40-50 lines of code.
>
> we could be seeing the differences you speak of. An example would be the
> following.
>
> This should have a CCM of 13, in Sonar i got 25.
>
> iboolean exampleA(int input)
> {
>   if (input == 0) {
>     return false;
>   } else if (input < 0) {

"else" after "return" or "throw" should be a top-level fail
There should *never* be an else after something that immediately exits
the scope. NEVER!

 if (input == 0)
     return false;
 if (input < 0)
     return (…);
 etc.

unrelated to complexity, this should throw the highest level of
unacceptable non-compliant code writing

>     return check(input * -1);
>   } else if (input < 1) {
>     return check(input * -1);
>   } else if (input < 2) {
>     return check(input * -1);
>   } else if (input < 3) {
>     return check(input * -1);
>   } else if (input < 4) {
>     return check(input * -1);
>   } else if (input < 5) {
>     return check(input * -1);
>   } else if (input < 6) {
>     return check(input * -1);
>   } else if (input < 7) {
>     return check(input * -1);
>   } else if (input < 8) {
>     return check(input * -1);
>   } else if (input < 999999999) {
>     return check(input * -1);
>   } else if (input < 10) {
>     return check(input * -1);
>   } else {
>     return check(input);
>   }
> }
--
H.Merijn Brand    [hidden email]   (072) 567 13 51
PROCURA B.V.    http://www.procura.nl    KvK 37140650
Ban de e-mail disclaimers:  http://tinyurl.com/kmne65

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

jbelbute
I've been using the cyclomatic complexity metric for some time.  Like any
metric, when it is a singular focus of your attention it can often drive
the wrong behavior.  It does measure complexity and the algorithm for
calculating it was originally published by Tom McCabe.  Complexity is not
bad, complexity everywhere is though.  So one of the things I encourage
folks to look at is the distribution of the complexity.  If the majority
of the methods in your system have high cyclomatic complexity then that is
cause for concern, but if the complexity is isolated in a relatively small
number of methods then there is a good chance that you are ok.  I remind
folks that if they are too focused on the cyclomatic complexity metric
then among the bad behaviors that you can drive is moving the complexity
out of the code and putting it into the design.  In other words you can
refactor almost any method into a number of smaller methods each having a
cyclomatic complexity <= 2 but that would make the code almost not
understandable.

In additional to the cyclomatic complexity metric I also look at the
essential complexity metric and what McCabe calls the S0 metric which is
the sum of the fan in (the number of places it is called from) and fan out
(the number of calls out) for a method.  The essential complexity metric
tells us how well structured the code is, the S0 tells how central to the
function of the system the method is.  So a very complex (high cyclomatic)
well structured (low essential) method is ok most of the time but if they
are both high then I will bet you will be spending a great deal of time
trying to maintain that method.  In the past I have been able to correlate
this situation with a high number of changes being checked in.

Unfortunately I do not believe Sonar supports either the Essential
complexity metric nor the S0 metric but it does support the SCM activity
so if you are looking at cyclomatic complexity it would be interesting to
look at files with a high volume of change to see if there is a
correlation to poorly structured code

john  

On 10/18/13 2:56 PM, "H.Merijn Brand" <[hidden email]> wrote:

>On Fri, 18 Oct 2013 10:19:06 -0700 (PDT), carchi <[hidden email]>
>wrote:
>
>> Well that true, but these are small functions. When we get in to larger
>> function with 40-50 lines of code.
>>
>> we could be seeing the differences you speak of. An example would be the
>> following.
>>
>> This should have a CCM of 13, in Sonar i got 25.
>>
>> iboolean exampleA(int input)
>> {
>>   if (input == 0) {
>>     return false;
>>   } else if (input < 0) {
>
>"else" after "return" or "throw" should be a top-level fail
>There should *never* be an else after something that immediately exits
>the scope. NEVER!
>
> if (input == 0)
>     return false;
> if (input < 0)
>     return (Š);
> etc.
>
>unrelated to complexity, this should throw the highest level of
>unacceptable non-compliant code writing
>
>>     return check(input * -1);
>>   } else if (input < 1) {
>>     return check(input * -1);
>>   } else if (input < 2) {
>>     return check(input * -1);
>>   } else if (input < 3) {
>>     return check(input * -1);
>>   } else if (input < 4) {
>>     return check(input * -1);
>>   } else if (input < 5) {
>>     return check(input * -1);
>>   } else if (input < 6) {
>>     return check(input * -1);
>>   } else if (input < 7) {
>>     return check(input * -1);
>>   } else if (input < 8) {
>>     return check(input * -1);
>>   } else if (input < 999999999) {
>>     return check(input * -1);
>>   } else if (input < 10) {
>>     return check(input * -1);
>>   } else {
>>     return check(input);
>>   }
>> }
>--
>H.Merijn Brand    [hidden email]   (072) 567 13 51
>PROCURA B.V.    http://www.procura.nl    KvK 37140650
>Ban de e-mail disclaimers:  http://tinyurl.com/kmne65
>
>---------------------------------------------------------------------
>To unsubscribe from this list, please visit:
>
>    http://xircles.codehaus.org/manage_email
>
>


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Keith Barlow
Well said John,  I was about to recommend FanIn/FanOut as well but I wasn't sure sonar had it.  I thought I had seen it at some point.  After a little digging I found that they do report fan in and fan out... it's on the Metrics tab when looking at a class and labeled as afferent and efferent couplings.

Keith


On Fri, Oct 18, 2013 at 6:27 PM, Belbute, John <[hidden email]> wrote:
I've been using the cyclomatic complexity metric for some time.  Like any
metric, when it is a singular focus of your attention it can often drive
the wrong behavior.  It does measure complexity and the algorithm for
calculating it was originally published by Tom McCabe.  Complexity is not
bad, complexity everywhere is though.  So one of the things I encourage
folks to look at is the distribution of the complexity.  If the majority
of the methods in your system have high cyclomatic complexity then that is
cause for concern, but if the complexity is isolated in a relatively small
number of methods then there is a good chance that you are ok.  I remind
folks that if they are too focused on the cyclomatic complexity metric
then among the bad behaviors that you can drive is moving the complexity
out of the code and putting it into the design.  In other words you can
refactor almost any method into a number of smaller methods each having a
cyclomatic complexity <= 2 but that would make the code almost not
understandable.

In additional to the cyclomatic complexity metric I also look at the
essential complexity metric and what McCabe calls the S0 metric which is
the sum of the fan in (the number of places it is called from) and fan out
(the number of calls out) for a method.  The essential complexity metric
tells us how well structured the code is, the S0 tells how central to the
function of the system the method is.  So a very complex (high cyclomatic)
well structured (low essential) method is ok most of the time but if they
are both high then I will bet you will be spending a great deal of time
trying to maintain that method.  In the past I have been able to correlate
this situation with a high number of changes being checked in.

Unfortunately I do not believe Sonar supports either the Essential
complexity metric nor the S0 metric but it does support the SCM activity
so if you are looking at cyclomatic complexity it would be interesting to
look at files with a high volume of change to see if there is a
correlation to poorly structured code

john

On 10/18/13 2:56 PM, "H.Merijn Brand" <[hidden email]> wrote:

>On Fri, 18 Oct 2013 10:19:06 -0700 (PDT), carchi <[hidden email]>
>wrote:
>
>> Well that true, but these are small functions. When we get in to larger
>> function with 40-50 lines of code.
>>
>> we could be seeing the differences you speak of. An example would be the
>> following.
>>
>> This should have a CCM of 13, in Sonar i got 25.
>>
>> iboolean exampleA(int input)
>> {
>>   if (input == 0) {
>>     return false;
>>   } else if (input < 0) {
>
>"else" after "return" or "throw" should be a top-level fail
>There should *never* be an else after something that immediately exits
>the scope. NEVER!
>
> if (input == 0)
>     return false;
> if (input < 0)
>     return (Š);
> etc.
>
>unrelated to complexity, this should throw the highest level of
>unacceptable non-compliant code writing
>
>>     return check(input * -1);
>>   } else if (input < 1) {
>>     return check(input * -1);
>>   } else if (input < 2) {
>>     return check(input * -1);
>>   } else if (input < 3) {
>>     return check(input * -1);
>>   } else if (input < 4) {
>>     return check(input * -1);
>>   } else if (input < 5) {
>>     return check(input * -1);
>>   } else if (input < 6) {
>>     return check(input * -1);
>>   } else if (input < 7) {
>>     return check(input * -1);
>>   } else if (input < 8) {
>>     return check(input * -1);
>>   } else if (input < 999999999) {
>>     return check(input * -1);
>>   } else if (input < 10) {
>>     return check(input * -1);
>>   } else {
>>     return check(input);
>>   }
>> }
>--
>H.Merijn Brand    [hidden email]   (072) 567 13 51
>PROCURA B.V.    http://www.procura.nl    KvK 37140650
>Ban de e-mail disclaimers:  http://tinyurl.com/kmne65
>
>---------------------------------------------------------------------
>To unsubscribe from this list, please visit:
>
>    http://xircles.codehaus.org/manage_email
>
>


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email





--
Keith Barlow

Software Engineer
Dell Boomi
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Keith Barlow
Correction... afferent and efferent couplings are only file level metrics...  not method level.  Not exactly what were looking for, me thinks.



On Fri, Oct 18, 2013 at 6:47 PM, Keith Barlow <[hidden email]> wrote:
Well said John,  I was about to recommend FanIn/FanOut as well but I wasn't sure sonar had it.  I thought I had seen it at some point.  After a little digging I found that they do report fan in and fan out... it's on the Metrics tab when looking at a class and labeled as afferent and efferent couplings.

Keith


On Fri, Oct 18, 2013 at 6:27 PM, Belbute, John <[hidden email]> wrote:
I've been using the cyclomatic complexity metric for some time.  Like any
metric, when it is a singular focus of your attention it can often drive
the wrong behavior.  It does measure complexity and the algorithm for
calculating it was originally published by Tom McCabe.  Complexity is not
bad, complexity everywhere is though.  So one of the things I encourage
folks to look at is the distribution of the complexity.  If the majority
of the methods in your system have high cyclomatic complexity then that is
cause for concern, but if the complexity is isolated in a relatively small
number of methods then there is a good chance that you are ok.  I remind
folks that if they are too focused on the cyclomatic complexity metric
then among the bad behaviors that you can drive is moving the complexity
out of the code and putting it into the design.  In other words you can
refactor almost any method into a number of smaller methods each having a
cyclomatic complexity <= 2 but that would make the code almost not
understandable.

In additional to the cyclomatic complexity metric I also look at the
essential complexity metric and what McCabe calls the S0 metric which is
the sum of the fan in (the number of places it is called from) and fan out
(the number of calls out) for a method.  The essential complexity metric
tells us how well structured the code is, the S0 tells how central to the
function of the system the method is.  So a very complex (high cyclomatic)
well structured (low essential) method is ok most of the time but if they
are both high then I will bet you will be spending a great deal of time
trying to maintain that method.  In the past I have been able to correlate
this situation with a high number of changes being checked in.

Unfortunately I do not believe Sonar supports either the Essential
complexity metric nor the S0 metric but it does support the SCM activity
so if you are looking at cyclomatic complexity it would be interesting to
look at files with a high volume of change to see if there is a
correlation to poorly structured code

john

On 10/18/13 2:56 PM, "H.Merijn Brand" <[hidden email]> wrote:

>On Fri, 18 Oct 2013 10:19:06 -0700 (PDT), carchi <[hidden email]>
>wrote:
>
>> Well that true, but these are small functions. When we get in to larger
>> function with 40-50 lines of code.
>>
>> we could be seeing the differences you speak of. An example would be the
>> following.
>>
>> This should have a CCM of 13, in Sonar i got 25.
>>
>> iboolean exampleA(int input)
>> {
>>   if (input == 0) {
>>     return false;
>>   } else if (input < 0) {
>
>"else" after "return" or "throw" should be a top-level fail
>There should *never* be an else after something that immediately exits
>the scope. NEVER!
>
> if (input == 0)
>     return false;
> if (input < 0)
>     return (Š);
> etc.
>
>unrelated to complexity, this should throw the highest level of
>unacceptable non-compliant code writing
>
>>     return check(input * -1);
>>   } else if (input < 1) {
>>     return check(input * -1);
>>   } else if (input < 2) {
>>     return check(input * -1);
>>   } else if (input < 3) {
>>     return check(input * -1);
>>   } else if (input < 4) {
>>     return check(input * -1);
>>   } else if (input < 5) {
>>     return check(input * -1);
>>   } else if (input < 6) {
>>     return check(input * -1);
>>   } else if (input < 7) {
>>     return check(input * -1);
>>   } else if (input < 8) {
>>     return check(input * -1);
>>   } else if (input < 999999999) {
>>     return check(input * -1);
>>   } else if (input < 10) {
>>     return check(input * -1);
>>   } else {
>>     return check(input);
>>   }
>> }
>--
>H.Merijn Brand    [hidden email]   (072) 567 13 51
>PROCURA B.V.    http://www.procura.nl    KvK 37140650
>Ban de e-mail disclaimers:  http://tinyurl.com/kmne65
>
>---------------------------------------------------------------------
>To unsubscribe from this list, please visit:
>
>    http://xircles.codehaus.org/manage_email
>
>


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email





--
Keith Barlow

Software Engineer
Dell Boomi



--
Keith Barlow

Software Engineer
Dell Boomi
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

James.Watson
In reply to this post by jbelbute
jbelbute wrote
...It does measure complexity and the algorithm for
calculating it was originally published by Tom McCabe...
After I posted this (sorry for the delay, I figured this thread was dead.)  I actually downloaded the original article from McCabe and worked through it.  I came to the conclusion that the issue (as I see it) is that there is a rather reasonable assumption that multiple returns are equivalent to the multiple exits that McCabe talks about in the paper.  Unfortunately this is an incorrect conclusion.

To understand this you need to understand what it means to code (or deal with code) written in an unstructured language.  I have the misfortune of having had to try to make sense of some unstructured COBOL code written by insane bonobos that had long ago fled the scene.  This code had interesting features such as jumping between two independent loops during their execution.  After spending a few weeks, we came to the conclusion that there was no way that what it did was correct but no one knew what it should do instead.  So, to the point, multiple exits in McCabes paper are specifically referring to leaving a subroutine using GOTOs to 'return' to different places in the source.  This is not possible in Java using multiple returns although one could make the case that throw/catch would fit this definition (but that's a different discussion.)  In effect, all Java methods have one implicit exit point.  That is you can't enter a subroutine in method A and exit to method B.  No matter how many return statements there are, they always resume execution at the same 'exit' point.

So I still hold that the calculation is wrong.  I propose that the belief that multiple returns are wrong is simply a dogma based on the misunderstanding of a seminal but (frankly) confusingly written article about programming languages that bear little resemblance to Java.  I don't believe that I will convert anyone so all I am really saying is that there should be options.  I plan to make this happen (for my team at least) if no one else acts.

So to the practical value of this, it seems to come up when implementing rather straightforward equals methods.  The checker complains of things that can't really be made any more clear.  They could be broken down but not made more straightforward.  The only way to get them to avoid the CC warning is to introduce error-prone return variables and if nesting which is pretty obviously more arcane and complex.

Of course, I started this thread before I understood how to flag things as 'false positives' and 'resolved' (not sure which to use here) so I'm probably making a mountain of a mole hill.

thanks for the discussion,

-Matt
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

James.Watson
In reply to this post by tux
tux wrote
"else" after "return" or "throw" should be a top-level fail
There should *never* be an else after something that immediately exits
the scope. NEVER!

 if (input == 0)
     return false;
 if (input < 0)
     return (…);
 etc.
Can you explain this more?  Are you saying that you are OK with multiple returns as long as there are no 'else' statements?  I'm not sure I understand that and my experience tells me the opposite.  Why wouldn't you want to state your intentions as clearly as possible to the compiler and make if more likely that it will let you know when you have made an error?

thanks,

-Matt
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Java
In reply to this post by James.Watson
Hi Matt,

nice interpretation, but IMHO even in java, one return at the end of a method is better then multiple returns throughout the method. May the original article talk about other languages, but I've seen more cobold styled code in java, you can possible imagine. An I damn wicket for their framework, because of constructors, looking like code from 68.

I struggled once with CC. For example equals and hash code sucks. But now I use the commons-lang HashCodeBuilder and EqualsBuilder.

However, if  you ask me, sonarQube is not about having 100% of total quality, but to track your quality of code at all.


-------- Ursprüngliche Nachricht --------
Von: "James.Watson" <[hidden email]>
Gesendet: Tue Oct 22 04:17:08 MESZ 2013
An: [hidden email]
Betreff: [sonar-user] Re: Cyclomatic Complexity Calculation Options

jbelbute wrote
> ...It does measure complexity and the algorithm for
> calculating it was originally published by Tom McCabe...

After I posted this (sorry for the delay, I figured this thread was dead.)
I actually downloaded the original article from  McCabe
<http://www.literateprogramming.com/mccabe.pdf>   and worked through it.  I
came to the conclusion that the issue (as I see it) is that there is a
rather reasonable assumption that multiple returns are equivalent to the
multiple exits that McCabe talks about in the paper.  Unfortunately this is
an incorrect conclusion.

To understand this you need to understand what it means to code (or deal
with code) written in an unstructured language.  I have the misfortune of
having had to try to make sense of some unstructured COBOL code written by
insane bonobos that had long ago fled the scene.  This code had interesting
features such as jumping between two independent loops during their
execution.  After spending a few weeks, we came to the conclusion that there
was no way that what it did was correct but no one knew what it should do
instead.  So, to the point, multiple exits in McCabes paper are specifically
referring to leaving a subroutine using GOTOs to 'return' to different
places in the source.  This is not possible in Java using multiple returns
although one could make the case that throw/catch would fit this definition
(but that's a different discussion.)  In effect, all Java methods have one
implicit exit point.  That is you can't enter a subroutine in method A and
exit to method B.  No matter how many return statements there are, they
always resume execution at the same 'exit' point.

So I still hold that the calculation is wrong.  I propose that the belief
that multiple returns are wrong is simply a dogma based on the
misunderstanding of a seminal but (frankly) confusingly written article
about programming languages that bear little resemblance to Java.  I don't
believe that I will convert anyone so all I am really saying is that there
should be options.  I plan to make this happen (for my team at least) if no
one else acts.

So to the practical value of this, it seems to come up when implementing
rather straightforward equals methods.  The checker complains of things that
can't really be made any more clear.  They could be broken down but not made
more straightforward.  The only way to get them to avoid the CC warning is
to introduce error-prone return variables and if nesting which is pretty
obviously more arcane and complex.

Of course, I started this thread before I understood how to flag things as
'false positives' and 'resolved' (not sure which to use here) so I'm
probably making a mountain of a mole hill.

thanks for the discussion,

-Matt



--
View this message in context: http://sonarqube.15.x6.nabble.com/Cyclomatic-Complexity-Calculation-Options-tp5018064p5018206.html
Sent from the SonarQube Users mailing list archive at Nabble.com.

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email




---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


tux
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

tux
In reply to this post by James.Watson
On Mon, 21 Oct 2013 19:27:23 -0700 (PDT), "James.Watson"
<[hidden email]> wrote:

> tux wrote
> > "else" after "return" or "throw" should be a top-level fail
> > There should *never* be an else after something that immediately exits
> > the scope. NEVER!
> >
> >  if (input == 0)
> >      return false;
> >  if (input < 0)
> >      return (…);
> >  etc.
>
> Can you explain this more?

Yes I can, and I will try

> Are you saying that you are OK with multiple returns as long as there are
> no 'else' statements?  I'm not sure I understand that and my experience
> tells me the opposite.  Why wouldn't you want to state your intentions as
> clearly as possible to the compiler and make if more likely that it will
> let you know when you have made an error?

The «if … else …» construct is there to do two different actions based
on an expression and then continue with common code after that

  wakeup ();
  if (monday ())
      breakfast ("cereal");
  else
      breakfast ("yoghurt");
  commute ();

if the last (unconditional) statement before an «else» exits the
enclosing method, there is no *common* code to run alter the if/else,
and the else is useless, making the code in all read less easily.

Let me take a *simplified* example of code I actually encountered
--8<---
public int foo (int i) throws Exception {

    try {
        if (i > 0)
            return (i - 1);
        else
            return (i + 1);

        return (i);
        }
    catch (Exception e) {
        throw e;
        }
    }
-->8---

Do you see how many errors are in that code?

1. The whole try/catch is useless, as this code cannot throw an
   exception. Removing it to a simplified
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);
    else
        return (i + 1);

    return (i);
    }
-->8---

2. That last return will never ever be called. Impossible
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);
    else
        return (i + 1);
    }
-->8---

3. The if/else has neither common code after it, nor could it be
   reached after a return from the if branch
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);

    return (i + 1);
    }
-->8---

In a similar vein, consider this (imho bad and from real life examples)
--8<---
public void foo (String s) throws MyException {

    if (s != null) {

        if (!s.isEmpty ()) {
            //
            // 150 lines of code
            //
            }
        else
            throw new MyException ("String is empty");

        }
    else
        throw new MyException ("String is null");
    }
-->8---

which could be simplified in a few steps, making the code a lot clearer

first step: revert outer if. The else branch does a throw, so it will
never ever reach common code after the if/else. exit early on errors
and continue coding for what you expect
--8<---
public void foo (String s) throws MyException {

    if (s == null)
        throw new MyException ("String is null");

    if (!s.isEmpty ()) {
        //
        // 150 lines of code
        //
        }
    else
        throw new MyException ("String is empty");
    }
-->8---

second step, revert the inner - now second - if/else
--8<---
    if (s == null)
        throw new MyException ("String is null");

    if (s.isEmpty ())
        throw new MyException ("String is empty");

    //
    // 150 lines of code
    //
-->8---

I really hope that both examples clearly state what I tried to say and
that the examples make sense. IMHO «else» after return/exit/throw/die
should be the highest level of inconformance possible

Note that I said unconditional return earlier. Here I want to note that
the following case obviously does not fall into these rules, as the
last return in either block is a conditional return
--8<---
public int foo (int i) {

    if (i > 0) {
        if (i < 10)
            return (i - 1);
        }
    else
        return (i + 1);

    return (i);
    }
-->8---

--
H.Merijn Brand    [hidden email]   (072) 567 13 51
PROCURA B.V.    http://www.procura.nl    KvK 37140650
Ban de e-mail disclaimers:  http://tinyurl.com/kmne65

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Keith Barlow
@merijn, I think it's good you have a practice you follow and it works for you but I have to say that I think the whole point of using structures such as:

>  if (input == 0)
>      return false;
>  if (input < 0)
>      return (…);

is to tell the reader he doesn't have to continue reading to figure out what it going to happen next...  there are always simple and common mistakes that can happen in code but to call this a failure of the worst kind is pretty extreme.  

Keith


On Tue, Oct 22, 2013 at 2:31 AM, H.Merijn Brand <[hidden email]> wrote:
On Mon, 21 Oct 2013 19:27:23 -0700 (PDT), "James.Watson"
<[hidden email]> wrote:

> tux wrote
> > "else" after "return" or "throw" should be a top-level fail
> > There should *never* be an else after something that immediately exits
> > the scope. NEVER!
> >
> >  if (input == 0)
> >      return false;
> >  if (input < 0)
> >      return (…);
> >  etc.
>
> Can you explain this more?

Yes I can, and I will try

> Are you saying that you are OK with multiple returns as long as there are
> no 'else' statements?  I'm not sure I understand that and my experience
> tells me the opposite.  Why wouldn't you want to state your intentions as
> clearly as possible to the compiler and make if more likely that it will
> let you know when you have made an error?

The «if … else …» construct is there to do two different actions based
on an expression and then continue with common code after that

  wakeup ();
  if (monday ())
      breakfast ("cereal");
  else
      breakfast ("yoghurt");
  commute ();

if the last (unconditional) statement before an «else» exits the
enclosing method, there is no *common* code to run alter the if/else,
and the else is useless, making the code in all read less easily.

Let me take a *simplified* example of code I actually encountered
--8<---
public int foo (int i) throws Exception {

    try {
        if (i > 0)
            return (i - 1);
        else
            return (i + 1);

        return (i);
        }
    catch (Exception e) {
        throw e;
        }
    }
-->8---

Do you see how many errors are in that code?

1. The whole try/catch is useless, as this code cannot throw an
   exception. Removing it to a simplified
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);
    else
        return (i + 1);

    return (i);
    }
-->8---

2. That last return will never ever be called. Impossible
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);
    else
        return (i + 1);
    }
-->8---

3. The if/else has neither common code after it, nor could it be
   reached after a return from the if branch
--8<---
public int foo (int i) {

    if (i > 0)
        return (i - 1);

    return (i + 1);
    }
-->8---

In a similar vein, consider this (imho bad and from real life examples)
--8<---
public void foo (String s) throws MyException {

    if (s != null) {

        if (!s.isEmpty ()) {
            //
            // 150 lines of code
            //
            }
        else
            throw new MyException ("String is empty");

        }
    else
        throw new MyException ("String is null");
    }
-->8---

which could be simplified in a few steps, making the code a lot clearer

first step: revert outer if. The else branch does a throw, so it will
never ever reach common code after the if/else. exit early on errors
and continue coding for what you expect
--8<---
public void foo (String s) throws MyException {

    if (s == null)
        throw new MyException ("String is null");

    if (!s.isEmpty ()) {
        //
        // 150 lines of code
        //
        }
    else
        throw new MyException ("String is empty");
    }
-->8---

second step, revert the inner - now second - if/else
--8<---
    if (s == null)
        throw new MyException ("String is null");

    if (s.isEmpty ())
        throw new MyException ("String is empty");

    //
    // 150 lines of code
    //
-->8---

I really hope that both examples clearly state what I tried to say and
that the examples make sense. IMHO «else» after return/exit/throw/die
should be the highest level of inconformance possible

Note that I said unconditional return earlier. Here I want to note that
the following case obviously does not fall into these rules, as the
last return in either block is a conditional return
--8<---
public int foo (int i) {

    if (i > 0) {
        if (i < 10)
            return (i - 1);
        }
    else
        return (i + 1);

    return (i);
    }
-->8---

--
H.Merijn Brand    [hidden email]   (072) 567 13 51
PROCURA B.V.    http://www.procura.nl    KvK 37140650
Ban de e-mail disclaimers:  http://tinyurl.com/kmne65

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email





--
Keith Barlow

Software Engineer
Dell Boomi
tux
Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

tux
On Tue, 22 Oct 2013 10:50:38 -0400, Keith Barlow <[hidden email]>
wrote:

> @merijn, I think it's good you have a practice you follow and it works for
> you but I have to say that I think the whole point of using structures such
> as:
>
> >  if (input == 0)
> >      return false;
> >  if (input < 0)
> >      return (…);  
>
> is to tell the reader he doesn't have to continue reading to figure out
> what it going to happen next...  there are always simple and common
> mistakes that can happen in code but to call this a failure of the worst
> kind is pretty extreme.

Just to make my point (even more) clear: I do not object to having
multiple return's or throw's in a single method. I would even say that
I am in favor of those: return on failure fast

I am just very opposed to «else» after unconditional return/throw als
it takes away all clarity in the code.

  if (expression) {
      // 40 lines of code
      return;
      }
  else {
      // 160 lines of code
      }

  common code;

is obfuscated code, as the (what reads as) common code is never
executed when expression evaluates to false.

The fact that is also uselessly indents the 160 lines is just an
additional reason not to do it like this.

> Keith



--
H.Merijn Brand    [hidden email]   (072) 567 13 51
PROCURA B.V.    http://www.procura.nl    KvK 37140650
Ban de e-mail disclaimers:  http://tinyurl.com/kmne65

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: Cyclomatic Complexity Calculation Options

Keith Barlow
In this case, the better alternative would be:

  if (expression) {
      return methodA();
      }
  else {
     return methodB();
      }

...

methodA() {
// 40 lines of code
}

methodB() {
      // 160 lines of code
}

If there is common code that needs to be executed after invocation of method A or method B, then you could assign the responses to a variable and evaluate or you could relocate to the method that calls this switching method.  Which decision is better probably depends on what the function's purpose and definition is.

Keith


On Tue, Oct 22, 2013 at 12:13 PM, H.Merijn Brand <[hidden email]> wrote:
On Tue, 22 Oct 2013 10:50:38 -0400, Keith Barlow <[hidden email]>
wrote:

> @merijn, I think it's good you have a practice you follow and it works for
> you but I have to say that I think the whole point of using structures such
> as:
>
> >  if (input == 0)
> >      return false;
> >  if (input < 0)
> >      return (…);
>
> is to tell the reader he doesn't have to continue reading to figure out
> what it going to happen next...  there are always simple and common
> mistakes that can happen in code but to call this a failure of the worst
> kind is pretty extreme.

Just to make my point (even more) clear: I do not object to having
multiple return's or throw's in a single method. I would even say that
I am in favor of those: return on failure fast

I am just very opposed to «else» after unconditional return/throw als
it takes away all clarity in the code.

  if (expression) {
      // 40 lines of code
      return;
      }
  else {
      // 160 lines of code
      }

  common code;

is obfuscated code, as the (what reads as) common code is never
executed when expression evaluates to false.

The fact that is also uselessly indents the 160 lines is just an
additional reason not to do it like this.

> Keith



--
H.Merijn Brand    [hidden email]   (072) 567 13 51
PROCURA B.V.    http://www.procura.nl    KvK 37140650
Ban de e-mail disclaimers:  http://tinyurl.com/kmne65

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email





--
Keith Barlow

Software Engineer
Dell Boomi
12