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

Corion,
A couple of links to help reinforce your response:
• Categorized Damian Modules
• Implementing Dispatch Tables

And while you know the difference, it is worth pointing out that perl 5.10.0 introduced a given/when which is mostly analogous to a switch statement. This is considered safe and a good practice (perlsyn).

Cheers - L~R

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 :-))

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)

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.

-Jeff

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};
    }
}
[download]

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};

/filename\s*=\s*(\S+)/x
[download]

/filename\s*=\s*(\S+)/
[download]

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 :-))

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=();
}
[download]

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

given ($_) {
    when (/filename\s*=\s*(\S+)/) {$file = basename($1)}
    when (/go/ && exists $files{$file}) {
        push @PLACE, @batch;
        delete $files{$file}
}
@batch = ();
[download]

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+).