Beefy Boxes and Bandwidth Generously Provided by pair Networks
XP is just a number

PerlCritic, $_ and map

by chexmix (Hermit)
on Feb 12, 2009 at 20:03 UTC ( #743445=perlquestion: print w/ replies, xml ) Need Help??
chexmix has asked for the wisdom of the Perl Monks concerning the following question:

Good Day Monks,

I'm working on refactoring some scripts / code to bring it more in line with Damian Conway's _PBP_, and am using the commandline perlcritic tool to find trouble spots. One of the complaints perlcritic is spitting out (I am running it at the fairly low "stern" setting) is this:

"Don't modify $_ in list functions ..."

... and the line it is complaining about looks like this:

my @files = map {chomp;s/^\s+//; s/\+s$//; $_;} <$FILES>;

I read through the relevant material in _PBP_ & understand the drawbacks of this construction, but was fuzzy on how to best fix it. Knowing that I'd find good info at the Monastery, I went looking around. I found the following node:

And on the strength of merlyn's recommendation there, I changed the offending line to

my @files = map { local $_ = $_; chomp; s/^\s+//; s/\+s$//; $_; } <$FILES>;

Now, I did this pretty mindlessly / robotically / whatever, but I thought I understood what the rationale was for changing this in this way, so I was hopeful this edit would satisfy perlcritic.

But it didn't - I get the same complaint, on the same line. Is perlcritic in error by complaining about the _revised_ use of map, or have I done something else wrong? I admit the latter is by far the more likely ...


Comment on PerlCritic, $_ and map
Replies are listed 'Best First'.
Re: PerlCritic, $_ and map
by ikegami (Pope) on Feb 12, 2009 at 20:25 UTC

    I get the same complaint, on the same line.

    False positive. It apparently doesn't check for local $_. I tend to avoid local $_ anyway. What follows is a list of alternatives.

    (Note that chomp is probably redundant with s/\+s$//;, so I omitted it.)

    my @files = map { my $s = $_; $s =~ s/^\s+//; $s =~ s/\+s$//; $s } <$FILES>;
    my @files = map { my $s = $_; for ($s) { s/^\s+//; s/\+s$//; } $s } <$FILES>;
    my @files = <$FILES>; for (@files) { s/^\s+//; s/\+s$//; }
    use List::MoreUtils qw( apply ); my @files = apply { s/^\s+//; s/\+s$//; } <$FILES>;
    use Algorithm::Loops qw( Filter ); my @files = Filter { s/^\s+//; s/\+s$//; } <$FILES>;

    Or turn off disable the critic for that instance, since you're only inadvertently modifying anonymous elements on the stack.

    Do you need an array in the first place?

    while (<$FILES>) { s/^\s+//; s/\+s$//; ... }
Re: PerlCritic, $_ and map
by kyle (Abbot) on Feb 12, 2009 at 20:27 UTC
Re: PerlCritic, $_ and map
by Fletch (Chancellor) on Feb 12, 2009 at 20:22 UTC

    Probably PC's just not smart enough to tell that you've explicitly localized $_ there. If you absolutely have to get rid of the complaint you could always change it to use a lexical and explicit binds (I sometimes use something along the lines of map { ( my $t = $_ ) =~ s/foo/bar/; $t } @xs).

    The cake is a lie.
    The cake is a lie.
    The cake is a lie.

Re: PerlCritic, $_ and map
by shmem (Canon) on Feb 12, 2009 at 21:49 UTC
    my @files = map {chomp;s/^\s+//; s/\+s$//; $_;} <$FILES>;

    As always, I'd take the PBP advisories with a grain of salt. The list you are passing to map results from an exhaustive read from the file-handle <$FILES>, which happens to be built only there and now; how could "modifying the input list" have any side-effects here?

    We now could ask ourselves about modifying the arguments to map by altering $_: "is it useful?" and "is it documented?" and "can it be avoided?". The answer is "yes" for all three questions. Here's the relevant snippet form perlfunc:

    Note that $_ is an alias to the list value, so it can be used to modify the elements of the LIST. While this is useful and supported, it can cause bizarre results if the elements of LIST are not variables.

    Then we could proceed to discuss whether that behavior is a feature of map or a bug by implementation glitch, labeled as a feature (which could be the case), and whether perl's behavior is consistent (which here is, I think, looking at for); but hasn't that been done many times before?

    I can see no bad practice in rewriting a hash and a sorted list of its keys in one pass:

    @sorted = sort keys %hash; %hash = map { my $v = $hash{$_}; s/ /_/g; $_, $v } @sorted;

    But maybe that's just me. Maybe Best Practice would be programming perl as if it where python... ;-)

      how could "modifying the input list" have any side-effects here?

      You'd be surprised in some instances.

      $ perl -le' for (1..2) { print map { my $x=$_; $_="!"; $x } 1..5 } ' 12345 !!!!!

      But that's not the case here.

      I can see no bad practice in rewriting a hash and a sorted list of its keys in one pass:

      %hash = map { my $v = $hash{$_}; s/ /_/g; $_, $v } @sorted;

      That was actually hard to understand, but I suppose a comment would solve that.

      Keep in mind that perlcritic will err on the side of false positive over false negative. Otherwise, it would be redundant with -w. Even if the rule is sound doesn't mean there aren't exceptions.

Re: PerlCritic, $_ and map
by moritz (Cardinal) on Feb 12, 2009 at 23:52 UTC
    I hope the technical side has already been explained well enough.

    So let me just add that PBP fulfilled its purpose - it made you think about your code, and if it's good or not.

    I know there's a part of the Perl community (and I count myself among them) that complain about brainlessly applied PBP rules. But each time somebody asks here if they make sense, or why perlcritic complains, it actually did something good.

    PBP isn't primarily about rules; it's about knowing (or finding out) what you do.

Re: PerlCritic, $_ and map
by Skeeve (Vicar) on Feb 12, 2009 at 22:54 UTC

    I never thought about these side effects and I'm glad I looked into this node. Each time I'm here I learn something new & valuable. Thanks to all of you!

    But to say something in regards to the code example: Now that I know of the problems using this kind of trimming in map, what do you think about this alternative?

    my @files = map { /^\s*(\S.*?)\s*$/ && $1 || "" } <$FILES>;


      /^\s*(\S.*?)\s*$/ && $1 || ""
      is an odd way of writing
      /^\s*(\S.*?)\s*$/ ? $1 : ""

      If you remove the unnecessary \S, you get a pattern that always matches so
      /^\s*(.*?)\s*$/ ? $1 : ""
      can be simplified to
      /^\s*(.*?)\s*$/; $1

      what do you think about this alternative?
      my @files = map { /^\s*(\S.*?)\s*$/ && $1 || "" } <$FILES>;
      Quite bad actually. s/^\s+//; s/\s+$//; is common idiom enough that people instantly see what is being done (it is for instance mentioned in the perlfaq) - your solution requires more thinking to see what it does.

      And then there's performance.

      use Benchmark 'cmpthese'; our @data = ('foo', 'foo bar', ' foo', 'foo ', ' foo ', ' foo bar '); + cmpthese -1, { faq => 'for my $x (@::data) {$_ = $x; s/^\s+//; s/\s+$//}', Skeeve => 'for my $x (@::data) {$_ = $x; $_ = /^\s*(\S.*?)\s*$/ && $ +1 || ""}' }; __END__ Rate Skeeve faq Skeeve 19139/s -- -68% faq 59077/s 209% --

        While I agree about the benchmark result, I don't agree with what you call "Skeeve's". Try this:

        use Benchmark 'cmpthese'; our @data = ('foo', 'foo bar', ' foo', 'foo ', ' foo ', ' foo bar '); + cmpthese -1, { faq => 'my @f= map {local $_ = $_; s/^\s+//; s/\s+$//; $_ } @::da +ta', Skeeve => 'my @f= map { /^\s*(\S.*?)\s*$/ && $1 || "" } @::data', ikegami => 'my @f= map { /^\s*(.*?)\s*$/; $1; } @::data' }; __END__ Rate Skeeve ikegami faq Skeeve 24561/s -- -8% -28% ikegami 26614/s 8% -- -22% faq 33998/s 38% 28% --

Re: PerlCritic, $_ and map
by andreas1234567 (Vicar) on Feb 13, 2009 at 07:43 UTC
    If you know what you are doing, then you should not fear ## No Critic. See BENDING_THE_RULES.
    No matter how great and destructive your problems may seem now, remember, you've probably only seen the tip of them. [1]
      In my case that "if" clause is a big 'un! ;^)

      I hope one day to summit the far-off peak of marginal competence, and to enjoy that feeling (alien to me) humans term "confidence."

      A big thank you to all who replied - this is a great discussion, and I'm absorbing everything as best I can.

Re: PerlCritic, $_ and map
by JavaFan (Canon) on Feb 12, 2009 at 23:40 UTC
    Silly PerlCritic. It complaints about a modifying $_ that isn't harmful, but is silent about trying to remove whitespace from the end of the string twice.

    You do realize that 'chomp' does a subset of what s/\s+$// does (I assume your s/\+s$// is a typo) and is hence redundant?

      Usually, not always, as I've already mentioned.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://743445]
Approved by sweetblood
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (3)
As of 2015-11-28 22:52 GMT
Find Nodes?
    Voting Booth?

    What would be the most significant thing to happen if a rope (or wire) tied the Earth and the Moon together?

    Results (746 votes), past polls