Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 50727
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: Christopher.Dunn [...] amd.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: McCabe calcuation is too high (in PerlCritic)
Date: Wed, 21 Oct 2009 21:02:28 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Christopher Dunn <Christopher.Dunn [...] amd.com>
http://search.cpan.org/~elliotjs/Perl-Critic-1.080/lib/Perl/Critic/Policy/Subroutines/ProhibitExcessComplexity.pm

Cyclomatic complexity should not count the 'else' portion of the flow. elsif yes, but not else.  That is probably not obvious from the description, e.g.
    http://en.wikipedia.org/wiki/Cyclomatic_complexity#Implications_for_Software_Testing

The example is pretty clear:
Show quoted text
if( c1() )
   f1();
else
   f2();
 
if( c2() )
   f3();
else
   f4();
  
The complexity is not the number of paths (1 x 2 x 2 == 4).  Instead, it is the number decision points (plus 1 for deciding to start the program).  An else block can be thought of as code that would have executed anyway if the if-true block were not present, i.e. the default flow of the program.  Else-if, however, represents another decision.  while/for/etc. are decisions too, which is obvious if you replace them with only ifs and gotos.  Because of short-circuiting, booleans have to be counted as well.  (That's not true in all languages.)  Whether to count just 'switch' or all 'cases' is hotly debated, but to me, a switch is a single decision and evidence of good coding.

If someone writes a short function with 5 if-blocks, his complexity is 6 (a good score) not 11 (considered high).  People will ignore this altogether if the scores are excessive.

Here is a clearer description:
    http://www.aivosto.com/project/help/pm-complexity.html

The fix is easy.

 33 Readonly::Hash my %LOGIC_KEYWORDS =>
 34     hashify( qw( if else elsif unless until while for foreach ) );

should be
 34     hashify( qw( if elsif unless until while for foreach switch ) );


This is a very useful measure, so making it a bit more accurate would be a worthwhile improvement.


~Chris
Subject: Re: [rt.cpan.org #50727] McCabe calcuation is too high (in PerlCritic)
Date: Wed, 21 Oct 2009 23:08:57 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 290b
Christopher.Dunn@amd.com via RT wrote: Show quoted text
> The fix is easy. > > 33 Readonly::Hash my %LOGIC_KEYWORDS => > 34 hashify( qw( if else elsif unless until while for foreach ) ); > > should be > 34 hashify( qw( if elsif unless until while for foreach switch ) );
What is "switch" supposed to be?
Subject: Re: [rt.cpan.org #50727] McCabe calcuation is too high (in PerlCritic)
Date: Thu, 22 Oct 2009 00:29:11 -0500
To: "bug-Perl-Critic [...] rt.cpan.org" <bug-Perl-Critic [...] rt.cpan.org>
From: Christopher Dunn <Christopher.Dunn [...] amd.com>
Elliot Shank via RT wrote: Show quoted text
<URL: http://rt.cpan.org/Ticket/Display.html?id=50727 >

Christopher.Dunn@amd.com via RT wrote:
  
The fix is easy.

33 Readonly::Hash my %LOGIC_KEYWORDS =>
34 hashify( qw( if else elsif unless until while for foreach ) );

should be
34 hashify( qw( if elsif unless until while for foreach switch ) );
    
What is "switch" supposed to be?
  

Oh.  In Perl5.10 that should be 'given'.  C uses 'switch' for 'given', and 'case' for 'when'.

  http://en.wikipedia.org/wiki/Switch_statement


Here is a better discussion on CC:

  http://www.eli.sdsu.edu/courses/spring97/cs696/notes/metrics2/metrics2.html

~Chris


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.