Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

Perl::Critic says don't modify $_ in list functions and other things

by Lady_Aleena (Curate)
on Jul 09, 2020 at 01:29 UTC ( #11119056=perlquestion: print w/replies, xml ) Need Help??

Lady_Aleena has asked for the wisdom of the Perl Monks concerning the following question:

Hello all. I have been playing around with Per::Critic on the command line and found to my dismay that some of my modules do not pass gentle. I realize that I can ignore the recommendations of Perl critic, but I would love for my modules to pass "gentle" before I begin ignoring things.

The most common issue is that I modify $_ in list functions. The following is a convenient and short one liner to put the lines of files into a list. I thought doing it this way was nice and slim.

my @list = map { chomp($_); $_ } <$fh>; my @uc_list = map { chomp $_; [uc $_] } <$lc_fh>; # used only onc +e my @split_list = map { chomp $_; [ split(/\|/, $_) ] } <$piped_fh>; +# used only once

In one subroutine, I have this three times.

my @array = map { $_ =~ s/^[\*\+-] (.+)/$1/; some_sub($_, $opt); } @another_array;

Also, my idify subroutine is just modifying $_.

my @ids = map { $_ =~ s/<.+?>//g; $_ =~ s/^(\d+([stnrdh]{2}|))/NUMWORDS($1)/e; $_ =~ s/(.)\.\w{2,5}?$/$1/; $_ =~ s/&amp/and/g; $_ =~ s/&/and/g; $_ =~ s//Ae/g; $_ =~ s//C/g; $_ =~ s//U/g; $_ =~ s/(||)/e/g; $_ =~ s/#/No/g; $_ =~ s/ /_/g; $_ =~ s/[^\w:.\-]//g; $_; } grep {defined($_)} @base;

And my Fancy::Map modifies $_ too.

sub fancy_map { my ($opt, $list) = @_; map { if (ref($_)) { fancy_map($opt, $_); } else { my $before = $opt->{'before'} ? $opt->{'before'}.' ' : ''; my $after = $opt->{'after'} ? ' '.$opt->{'after'} : ''; $_ = $before.$_.$after; } } @{$list}; }

If this is something ignored usually, I will ignore it, but I would like to know how to make it better. Should I just assign $_ to a variable and then use the variable?

Another thing that I am confused by is why the expression form of eval is discouraged?

if ($raw_value =~ /\*/) { # multiplication $total_value = eval($raw_value); ($amount, $base_value) = split(/\*/,$raw_value); } elsif ($raw_value =~ /\//) { # division $base_value = eval($raw_value); ($total_value, $amount) = split(/\//, $raw_value); }

The rest of the gentle issues are me being lazy with conditionals. While writing I was thinking my $var = "foo" if 'some condition', but Perl critic does not like it, but it is fixed easily.

NOTE: Please see my update. Most are fixed, but the eval problem remains.

My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.8.8 on web host.

No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
Lady Aleena

Replies are listed 'Best First'.
Re: Perl::Critic says don't modify $_ in list functions and other things
by choroba (Archbishop) on Jul 09, 2020 at 05:37 UTC
    Sometimes, no loop is needed at all:
    chomp( my @list = <$fh> );

    When using readline, it's probably not so serious, as you aren't changing the original array in place at the same time.

    The /r is useful when you want to keep the original:

    my @ids = map s/ /_/gr, grep defined, @base;

    When you want to modify the original array, use "for" instead.

    for (@$list) { if (ref) { ... } else { $_ = ...; } }
    map{substr$_->[0],$_->[1]||0,1}[\*||{},3],[[]],[ref qr-1,-,-1],[{}],[sub{}^*ARGV,3]
      The /r is useful when you want to keep the original

      It is indeed useful and thanks for mentioning it (++). For some reason the fact that it exists never seems to stick with me! However, in Lady_Aleena's case it might not be an option because her sig says

      my perl versions are 5.28.1 local and 5.8.8 on web host.

      and /r only came in at 5.13.2 (according to perlver).

      As an even further aside it's a bit surprising to find anyone offering 5.8.8 as part of a service today. I am not aware of any O/S still shipping that as standard.

        As an even further aside it's a bit surprising to find anyone offering 5.8.8 as part of a service today. I am not aware of any O/S still shipping that as standard.

        That's what I was thinking...
        I'm considering upgrading from shared hosting to VPS purely so I can upgrade from 5.16.003 to something a bit more modern. Being stuck with 5.8.8 seems a bit dire.

Re: Perl::Critic says don't modify $_ in list functions and other things (updated x2)
by AnomalousMonk (Bishop) on Jul 09, 2020 at 02:20 UTC

    For me, the critical thing to realize about a statement like
        my @array = map { $_ =~ s/^[\*\+-] (.+)/$1/; ... } @another_array;
    is that it is operating on the elements of @another_array at the same time it's generating a new array to assign to @array. So the function

    sub fancy_map { my ($opt, $list) = @_; map { if (ref($_)) { fancy_map($opt, $_); } else { my $before = $opt->{'before'} ? $opt->{'before'}.' ' : ''; my $after = $opt->{'after'} ? ' '.$opt->{'after'} : ''; $_ = $before.$_.$after; } } @{$list}; }
    which is passed an array by the reference $list is also operating on the referent of $list!

    Consider:

    c:\@Work\Perl\monks>perl -wMstrict -le "my @list = qw(foo bar baz boff quux); print qq{original: @list}; ;; my @spookified = spooky_aaad(\@list); print qq{returned: @spookified}; print qq{original: @list}; ;; sub spooky_aaad { my ($array_ref) = @_; ;; return map { $_ =~ s{ \A (.) (.*) \z }{\U$2\E$1}xms; $_; } @$array_ref; } " original: foo bar baz boff quux returned: OOf ARb AZb OFFb UUXq original: OOf ARb AZb OFFb UUXq
    (Update: Also, the statement
        my @another_array = map { chomp($_);  $_; } @original_array;
    is equivalent to the statements
        my @another_array = @original_array;
        chomp @another_array;
        chomp @original_array;
    because $_ is aliased to the elements of @original_array. In the case of a statement like
        my @list = map { chomp($_); $_ } <$fh>;
    this doesn't matter: the <$fh> expression creates a temporary list that is fed to map and then vanishes. But in other cases... See also ikegami's reply.)

    Any time I see code structured in this way, my buggy-sense starts to tingle. If you're happy with the operation of your code, that's fine with me; you can just ignore or (I think) turn off the specific critique. Just be aware that pitfalls lurk here.

    Update: Oops... In my haste to answer, I overlooked the latter questions of your post.

    ...why the expression form of eval is discouraged?
    String eval is looked upon askance because it is an open invitation to all kinds of mischief if your data is not absolutely trustworthy — and even then many programmers would avoid it, preferring alternatives that may be slightly more roundabout, but that are a whole bunch safer.
    my $var = "foo" if 'some condition';
    This inadvertantly allowed syntax leaves $var in an undefined state of existence. It used to be used to simulate a persistent, "static" variable within a function; state is used for this purpose now, and the old usage is officially Frowned Upon.


    Give a man a fish:  <%-{-{-{-<

Re: Perl::Critic says don't modify $_ in list functions and other things
by ikegami (Pope) on Jul 09, 2020 at 03:03 UTC

    Here's the problem with your approach:

    my @a = qw( a b c ); my @b = map { $_ = uc($_); $_ } @a; say "@a"; # A B C <-- You clobbered @a!!! say "@b"; # A B C

    Use List::MoreUtils's apply instead of map.

    my @a = qw( a b c ); my @b = apply { $_ = uc($_);} @a; say "@a"; # a b c say "@b"; # A B C

      I hope Im not being obtuse and about to embarrass myself but

      # Why this? my @b = apply { $_ = uc($_);} @a; # Instead of this, or a more verbose version of it? my @b = map uc, @a;

        I'll take the liberty of answering for ikegami and guess that
            my @b = apply { $_ = uc($_);} @a;
        is just a quick example of the use of apply put together to be consistent with the preceding code
            my @b = map { $_ = uc($_); $_ } @a;
        which was itself just a quick example put together to demonstrate a particular problem. I'd be very surprised if ikegami suggested it | either for production code.


        Give a man a fish:  <%-{-{-{-<

        Of course, but the OP isn't actually using $_ = uc($_), and your solution wouldn't help the OP. $_ = uc($_) is just a placeholder for a large block of code that possibly modifies $_.

Re: Perl::Critic says don't modify $_ in list functions and other things
by Marshall (Canon) on Jul 09, 2020 at 05:42 UTC
    Geez. At the first block of code, why not:
    my @list = map { s/\r\n$//; $_} <$fh>; my @uc_list = map { s/\r\n$//; uc $_; $_} <$lc_fh>; my @split_list = map { s/\r\n$//; split(/\|/, $_); [@$_] } <$piped_fh> +;
    UPDATE: I have found some rare issues using chomp() in multi-platform situations. chomp() does a great job when dealing with files generated upon that particular platform. I have written code that is tested on my Windows box and on a Unix box and then a user uses an old Mac to edit the file. chomp() usually works, but if old Mac formatting is allowed, that is not sufficient. To be honest, the last time I got an error report involving this, I told the old Mac user to set his editor to save files as DOS format. I did not change my code to allow old Mac.

    The easiest way to make sure that you delete all of the potential \r \n characters is to remove all white space at the end of the line. I like the suggestion from ikegami of s/\s+\z//. That does have a potential issue with a tab separated line containing a blank field at the end of the line.

      If you have 5.14.0 or higher, why not use s///r?

      my @list = map { s/\r\n$//r } <$fh>; my @uc_list = map { uc s/\r\n$//r } <$lc_fh>; my @split_list = map {[ split m/\|/ => s/\r\n$//r ]} <$piped_fh>;

      Enjoy, Have FUN! H.Merijn

        From OPs signature: 'My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.8.8 on web host.'

      my @list = map { s/\r\n$//; $_} <$fh>;

      Would any line produced by evaluating the <$fh> expression ever end in \r\n? (This might happen if the file handle had been opened in binmode, but then there would only ever be a single "line" read from the handle and the list processing code would seem superfluous.)

      I think s{ $/ \z }{}xms would work, but I also think that's an exact replacement for chomp; no improvement if so. If the /r modifier is available, s{ $/ \z }{}xmsr seems a very attractive alternative.


      Give a man a fish:  <%-{-{-{-<

        Would any line produced by evaluating the <$fh> expression ever end in \r\n?

        On Windows? It would require CR CR LF in the file (assuming default $/ and IO layers). Extremely unlikely.

        Elsewhere? It would require CR LF in the file (assuming default $/ and IO layers). Possible.

        I think s{ $/ \z }{}xms would work

        Well, that should be s{ \Q$/\E \z }{}xms (and /m and /s are useless) to be equivalent to the chomp.

        And if $/ hasn't been changed, s/\n\z// could be used (since $/ defaults to LF on all systems).

        But if I was going with a regex pattern, I'd go with s/\s+\z//. Handles \n, \r\n and other trailing whitespace. (TSV files being an exception.)

      This is a good point.
Re: Perl::Critic says don't modify $_ in list functions and other things
by perlfan (Vicar) on Jul 09, 2020 at 04:38 UTC
    > If this is something ignored usually, I will ignore it, but I would like to know how to make it better. Should I just assign $_ to a variable and then use the variable?

    This is what I do if $_ is a non-reference scalar. Otherwise, you'd have to take some extra steps to ensure a deep copy of whatever it's referencing. I generally only use map occasionally, and it's always in some sort of a transform operation that doesn't actually modify $_. But that's me.

    >Another thing that I am confused by is why the expression form of eval is discouraged?

    I don't do that because it means I am getting clever, and we know that leads to needing to have more than clever debugging skills. Again, that's just me, but I've been bitten by this too many times to not believe in the hidden future cost of cleverness. It's also a security vulnerability waiting to happen.

Re: Perl::Critic says don't modify $_ in list functions and other things
by haukex (Bishop) on Jul 11, 2020 at 06:28 UTC
    My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.8.8 on web host.

    Are you sure your web host doesn't offer newer versions? For example, I was once on a web host that offered multiple Perl versions, and you'd have to point the shebang at the right one, e.g. #!/usr/bin/perl5.28.

      It was only after posting the first message that I decided to check out what was on the new server my host moved me to, and behold, better Perl! My signature is updated.

      My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.16.3 or 5.30.0 on web host depending on the shebang.

      No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
      Lady Aleena
Re: Perl::Critic says don't modify $_ in list functions and other things
by jcb (Vicar) on Jul 09, 2020 at 22:38 UTC

    Other monks have mentioned this, but not in these words: map, grep, and for/foreach arrange for $_ to be an alias to each element of the list in turn. In a map block, $_ is not its own variable and modifying $_ will reach back into the original list. In some cases, $_ can even be aliased to some value slot that is read-only, and attempting to modify $_ will fail; for this reason, subs using $_ should localize *_, which replaces the entire glob and "shadows" any aliases that have been established in outer contexts.

    There are several reasons to avoid eval STRING that do not affect eval BLOCK. An obvious problem is that eval STRING parses Perl code from a string. If that string is untrusted remote user input and not very, very, very strictly checked, you have a remote code execution vulnerability. There is also a performance issue, in that eval STRING must invoke perl's parser each time it is reached at runtime, while the BLOCK in eval BLOCK is compiled along with the rest of the program. This also leads to one of the reasons eval STRING can be needed: because the STRING is not compiled until runtime, eval STRING can also trap compilation errors in STRING, while a compilation error in the BLOCK in eval BLOCK is fatal before execution starts.

      subs using $_ should localize *_, which replaces the entire glob

      Since this only about $_, then local $_; is better, because localizing @_ is unlikely to be in the sub's interest. (Update: Except on Perl versions before 5.14, as per the replies.)

        Using local $_ can fail if $_ is currently aliased to something read-only. The "Localization of special variables" and "Localization of globs" sections in perlsub explained this and recommended using local *_ instead in the affected versions. You can avoid problems with also localizing @_ by unpacking your arguments into lexicals before localizing *_.

        Beginning with perl 5.14, local $_ is a special case that also strips any magic associated with $_, while preserving the other slots in *_. Since Lady_Aleena mentions that her Web host is using a perl older than 5.14, this is a concern for our questioner.

Re: Perl::Critic says don't modify $_ in list functions and other things
by Lady_Aleena (Curate) on Jul 09, 2020 at 19:08 UTC

    I have updated my code to the following based on suggestions in this thread and chatterbox. With so much wondeful help, I didn't know who to reply to.

    my @list = map { chomp($_); $_ } <$fh>; my @uc_list = map { chomp $_; [uc $_] } <$lc_fh>; # used only onc +e my @split_list = map { chomp $_; [ split(/\|/, $_) ] } <$piped_fh>; +# used only once

    ...has become...

    my @list = map { chomp; $_ } <$fh>; my @uc_list = map { chomp; [uc $_] } <$lc_fh>; # used only once my @split_list = map { chomp; [ split(/\|/, $_) ] } <$piped_fh>; # u +sed only once

    It appears that explicitely using chomp($_) is what was the problem.

    The following code is now a lot simpler once I put the regex into the subroutine instead of munging it in the map.

    With the use of /r and a chain of regexen, I was able to change the map.

    And Fancy::Map got a complete overhaul with help.

    My only unfinished business with Perl::Critic set on gentle is my two eval(EXPR). I haven't figured out a fix for that at the moment. Does eval BLOCK act differently than eval EXPR?

    Also, please note my updated signature! 8)

    My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.16.3 or 5.30.0 on my web host depending on the shebang.

    No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
    Lady Aleena
      Does eval BLOCK act differently than eval EXPR?

      Indeed it does. eval BLOCK is parsed at compile time; if there are any syntactic errors, compilation fails. eval EXPR is evaluated at run time; if there are any errors, the error message goes into $@ (see perlvar), the eval returns undef, and the Perl program is free to soldier on.


      Give a man a fish:  <%-{-{-{-<

        I made a few changes to the conditionals around the eval code to possibly keep mistakes from happening when it is used, but I still can't figure out how to convert the eval EXPR into an eval BLOCK. Like converting a map or grep EXPR to a BLOCK, it isn't as easy as putting {} around it instead of () and expecting it to work the same.

        if ($raw_value && $raw_value =~ /\d+\*\d+/) { $total_value = eval($raw_value); # performs multiplication ($amount, $base_value) = split(/\*/,$raw_value); } elsif ($raw_value && $raw_value =~ /\d+\/\d+/) { $base_value = eval($raw_value); # performs division ($total_value, $amount) = split(/\//, $raw_value); } else { $amount = $raw_value; $base_value = $assets->{$lookup_asset} ? $assets->{$lookup_asset} +: 0; $total_value = $amount * $base_value; }

        My OS is Debian 10 (Buster); my perl versions are 5.28.1 local and 5.16.3 or 5.30.0 on web host depending on the shebang.

        No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
        Lady Aleena
Re: Perl::Critic says don't modify $_ in list functions and other things
by Anonymous Monk on Jul 10, 2020 at 14:58 UTC

    I actually started using Perl::Critic lo these many moons ago when the current Perl was 5.8, because of my $var = "foo" if 'some condition'. My experience was that usually it behaved the way you would expect ($var was undefined if the condition was false). But sometimes it retained whatever the previously-assigned value was.

    In general, my feeling about Perl::Critic is that if it flags something, it means someone, somewhere, under some circumstances had trouble with that construct. Maybe those circumstances do not apply to you, and you should not feel guilty about disabling a policy, but you should think first, both about what the policy does in general and whether it applies in your case.

      if it flags something, it means someone, somewhere, under some circumstances had trouble with that construct

      TheDamian in the cellar with the lead piping.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://11119056]
Approved by Marshall
Front-paged by davies
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others having an uproarious good time at the Monastery: (5)
As of 2020-11-26 16:30 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found

    Notices?