Beefy Boxes and Bandwidth Generously Provided by pair Networks
"be consistent"

Climbing Mt. Perlcritic -- Switch statements

by chexmix (Hermit)
on Mar 25, 2009 at 17:44 UTC ( #753188=perlquestion: print w/replies, xml ) Need Help??
chexmix has asked for the wisdom of the Perl Monks concerning the following question:

Salutations, Monks -

A few weeks ago I posted a node about an issue with the map function I had encountered while attempting to make a script PerlCritic-compliant.

I have hit another problem. In an effort to satisfy PerlCritic's complaint about undue complexity, I am attempting to turn the following nested if:

if ( /filename\s*=\s*(\S+)/x ) { $file = basename($1); } if ( /go/ ) { if ( exists $files{$file} ) { push @PLACE, @batch; delete $files{$file}; } @batch = (); }

... into a switch statement. I am doing this because in another instance, it knocked a couple of points off the complexity score. So I'm hoping for the same result here. Anyway, here is the current version of the replacement:

switch ($_) { case qr /filename\s*=\s*(\S+)/ { $file = basename($1);} case qr /go/ { if (exists $files{$file}) { push @PLACE, @batch; delete $files{$file}}} } @batch = ();

... note this is inside a foreach loop. Anyhoo, here is the error this is throwing:

fileparse(): need a valid pathname at ./script line xxx

Thing is, I think I understand what is happening here but not why: essentially, the call to basename() is not finding a good value in $1, which should have been returned from the match by the (\S+). So:

  1. I recall that it is not good to rely on these numbered variables -- what if there really is nothing there? e.g. what if the match was unsuccessful? BUT
  2. It works when the thing is constructed as two IFs, so why doesn't it work here?

I suspect it will sound overweening of me to say so, but I would prefer this not turn into a thread (IF it turns into a thread ... this could be a stupid question!) about the merits and/or drawbacks of PBP and why I should or should not be trying to satisfy the damned thing's complaints. Its use has been mandated, and I'm trying to use it as a learning tool. It's actually kind of fun - like a puzzle.

Besides, all the PBP anti- and for stuff has been well hashed out here, I think. Just my 2c.

Replies are listed 'Best First'.
Re: Climbing Mt. Perlcritic -- Switch statements
by Corion (Pope) on Mar 25, 2009 at 17:48 UTC

    It's always a bad idea to use It leads to hard to track down errors. I doubt that PerlCritic recommends - I'm not so sure about PBP though.

      AFAICT, PBP appears to recommend a lookup table based approach (Chapter 6 Control Structures - pp 118-121).

      A user level that continues to overstate my experience :-))
Re: Climbing Mt. Perlcritic -- Switch statements
by graff (Chancellor) on Mar 25, 2009 at 22:59 UTC
    First, I don't see the original version and the "switch" version in the OP code as being equivalent -- they don't do the same thing. Maybe you've tested and confirmed that your usage of the "switch" statement gives the same result as the non-switch version, in the sense that both case blocks can operate on the same string (if it happens to match both "filename=..." and "go"). (If I were using "switch", which I don't, that's something I'd want to test.) But it's still true that the non-switch version will only do @batch = () when you match /go/, whereas the switch version does that in every instance, no matter what; I don't know if that's a problem, but I would worry about it.

    Apart from that, I would strongly advise against modifying code logic just to get a few points of improvement on someone else's idea of a "complexity/simplicity" score. (And as others have said, I would avoid using the switch module.)

    You only should modify your coding logic or syntax if it's doing something wrong or if, when you try to read it, you find yourself struggling to understand what it's really doing (and/or what it's supposed to be doing). But if you can read it and it makes sense -- and if it works as intended -- it ain't broke, so don't "fix" it.

    (updated first and last paragraphs to try making them clearer)

Re: Climbing Mt. Perlcritic -- Switch statements
by jthalhammer (Friar) on Mar 26, 2009 at 03:04 UTC

    I agree that using Switch is a bad idea. And unfortunately, PPI and Perl-Critic don't yet understand the given/when syntax of Perl 5.10.

    As for complexity scores, the general idea is to count the number of possible paths through the code. Perl-Critic approximates the McCabe score by adding one point for each logical operator (&&, ||, and, or, etc.) and each "if", "elsif", "while", "until", "unless", "for", and "foreach" keyword. When we add support for the given/when syntax, each "when" clause will also add one point.

    So converting a long if/elsif/elsif chain into a given/when structure doesn't help your complexity score. Neither does consolidating nested "if" conditions with && and || operators. The best way to reduce complexity is to use dispatch tables as others have suggested, or to extract parts of the code into other subroutines.


      I would go exactly towards subroutines. Switch-statements are cluttered and at worst difficult to read and maintain. Given-when is similar although it does make the code a bit more readable. IMO the problem with switch/given structures is that too easily you insert multiple things into a clause making your logic/function "too long". It is also too easy to use overly complex conditions with given/when. In order to get cleaner and more testable code I would go for longer code. It eases the maintenance in the long run.

      Without knowing the full code it is difficult to properly refactor it as there are too many variables whose context and details are unknown (eg. @PLACE, @batch, %files, is the code inside a function or not). But here is my humble try. I did make some assumptions about the code though. Correct me if I'm wrong.

      # Introduce a new subs that can be tested. # New subs also separates the two functionalities # within the foreach loop. my @filenames = parse_file_names(@file_name_list); foreach my $file ( @filenames ) { if( $file =~ /go/ ) { populatePlaceIfExists(\%files, $file); @batch = (); } } sub parse_file_names { my @list = (); for( @_ ) { if ( /filename\s*=\s*(\S+)/x ) { push @list, $1; } } return @list; } sub populatePlaceIfExists { my $files_hash = shift; my $file = shift; if ( exists $files_hash->{$file} ) { push @PLACE, @batch; delete $files_hash->{$file}; } }

      update: one function was missing, copy/paste error..

      update2: I know I generated lots of code, the purpose is that most of the code is easily unit tested. Subs that are strongly decoupled from the data makes it easy to write unit tests. I'd like to know the context and the purpose of @batch and @PLACE so that I could see the associations between them and code and refactor even more.

      seek $her, $from, $everywhere if exists $true{love};
Re: Climbing Mt. Perlcritic -- Switch statements
by Bloodnok (Vicar) on Mar 25, 2009 at 17:50 UTC
    I nearly didn't spot that the if condition
    & the (supposedly) equivalent switch condition
    aren't the same - the if version has the x modifier.

    A user level that continues to overstate my experience :-))
      Not supposedly. They are equivalent.
        ... but surely they're equivalent only because there's no extended RE at play ?

        A user level that continues to overstate my experience :-))
Re: Climbing Mt. Perlcritic -- Switch statements
by Marshall (Abbot) on Mar 25, 2009 at 19:07 UTC
    I'm not sure how these various algorithms would score this code. But I would suspect highly that "flat code" is an objective, ie reducing the number of indent levels.

    Here you have an if and then another if than can only be executed if the first if is true.

    if ( /go/ && exists $files{$file}) { push @PLACE, @batch; delete $files{$file}; } if (/go/) { @batch=(); }
    reduces the "if" indentation level by one. And moves this action that happens on /go/ "no matter what" up a level at the expense of testing it again. I'm curious as to how that would be "scored"?
      Thanks for this ... I plugged it in and now the script is exhibiting some other weird behavior. I won't go into it deeply here b/c it would require discussing the script as a whole which I don't really need to do as yet ... in short, the output from the script has changed drastically, and not in the right way ;). So I have some kind of logic problem in that whole area of code ... I am tracking it down. Thanks for kicking my thinking in a different direction! G
Re: Climbing Mt. Perlcritic -- Switch statements
by JavaFan (Canon) on Mar 25, 2009 at 23:46 UTC
    Assuming you're not hanging on to old versions of Perl, I'd write it as:
    given ($_) { when (/filename\s*=\s*(\S+)/) {$file = basename($1)} when (/go/ && exists $files{$file}) { push @PLACE, @batch; delete $files{$file} } @batch = ();
    If it's in a for loop, and $_ is your loop variant, you can even omit the given.
    Thing is, I think I understand what is happening here but not why: essentially, the call to basename() is not finding a good value in $1, which should have been returned from the match by the (\S+).
    But you're not matching. qr// creates a pattern, it does not perform a match. qr/filename\s*=\s*(\S+)/ is always true. If you want to match, use m// instead.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://753188]
Approved by Corion
and monks are getting baked in the sun...

How do I use this? | Other CB clients
Other Users?
Others musing on the Monastery: (11)
As of 2017-03-23 18:55 GMT
Find Nodes?
    Voting Booth?
    Should Pluto Get Its Planethood Back?

    Results (294 votes). Check out past polls.