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

my @files = map { my $s = $_; for ($s) { s/^\s+//; s/\+s$//; } $s }
            <$FILES>;
[download]

my @files = <$FILES>;
for (@files) { s/^\s+//; s/\+s$//; }
[download]

use List::MoreUtils qw( apply );
my @files = apply { s/^\s+//; s/\+s$//; } <$FILES>;
[download]

use Algorithm::Loops qw( Filter );
my @files = Filter { s/^\s+//; s/\+s$//; } <$FILES>;
[download]

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

The Perl::Critic policy in question is Perl::Critic::Policy::ControlStructures::ProhibitMutatingListFunctions. It implements the policy recommendation from page 114 of PBP, which suggests that if you really need to do what you're doing, make a lexical variable.

my @files
    = map {
        my $file = $_;
        chomp $file;
        $file =~ s{ \A \s+ | \s+ \z }{}xmsg;
        $file;
    } <$FILES>;
[download]

It goes on to say that if you really do want a side-effect of your loop, use for instead.

Perhaps merlyn's use of local (as in modifying $_ in map (was Re: A perverse use of grep)) should be allowed by the policy. I don't think so, but I could see that being an option. In any case, rt://Perl::Critic might be a good place to make that case.

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

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

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

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
!!!!!
[download]

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

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.

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.

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

/^\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>;
[download]

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%     --
[download]

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%      --
[download]

---

s$$([},&%#}/&/]+}%&{})*;#$&&s&&$^X.($'^"%]=\&(|?*{%
+.+=%;.#_}\&"^"-+%*).}%:##%}={~=~:.")&e&&s""`$''`"e

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.

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.