Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

foreach argument modification inside loop, followed by return from loop

by vsespb (Hermit)
on Jul 10, 2013 at 10:15 UTC ( #1043435=perlquestion: print w/ replies, xml ) Need Help??
vsespb has asked for the wisdom of the Perl Monks concerning the following question:

from perldoc:
If any part of LIST is an array, foreach will get very confused if you add or remove elements within the loop body, for example with splice. So don't do that.
So we know that one should not modify foreach lists in loop: Is it safe to append to the array you are iterating over

However I wondering is that considered a bad code, when you modify "for" variables, but exit from loop immediately after that.
use warnings; my @a = (1,2,3); for (@a) { if ($_ == 2) { pop @a; last; } } print @a;
Such code works fine, but who knows maybe it can cause segfaults or other problems in some (future versions) of perl in rare conditions etc.

Comment on foreach argument modification inside loop, followed by return from loop
Download Code
Re: foreach argument modification inside loop, followed by return from loop
by rjt (Deacon) on Jul 10, 2013 at 10:31 UTC

    Such code works fine,

    Maybe. Have you tested this with every significant code path in the Perl interpreter on multiple platforms? Do you even know the significant code paths? Can you guarantee it will remain stable with tied variables? XS subs? Refs? Resized arrays? There are a lot of permutations you would need to go through before I'd be convinced this array modification "works fine".

    However I wondering is that considered a bad code

    I think the documentation is pretty clear: "don't do that". Just because the implementation happens to (seem to) work for whatever case you've come up with, directly going against the documented limitations of a fundamental Perl loop keyword is just begging for trouble, either now, or (possibly with greater consequences), later on down the road when everyone has forgotten the stupid unnecessary risk you took, with a for loop or two buried in a large program.

    Your code:

    my @a = (1,2,3); for (@a) { if ($_ == 2) { pop @a; last; } } print @a;

    What's wrong with print grep { $_ != 2 } @a;1, anyway? More efficient, and guaranteed not to get you fired from your job.

    Sorry if this seems a bit doom and gloom, but I really just don't get what you hope to gain, here. Perhaps you could explain?

    ________
    1. Well, if your goal is obfuscation, you got me. I just realized your code is actually along the lines of pop @a if grep { $_ == 3 } @a;. Cute, but I still don't see the point?

      Maybe. Have you tested this with every significant code path in the Perl interpreter on multiple platforms?
      that's what I actually ask in my question.
      What's wrong with print grep { $_ != 3 } @a;, anyway?
      that was just proof-of-concept code
        that was just proof-of-concept code

        Understood, and thanks for posting your real code farther down the thread. Based on that, the relevant logic you have is this:

        my $idx = 0; for my $j (@a) { if ($j == $target) { splice(@a, $idx, 1); last; } ++$idx; }

        Which helps clarify what you're trying to do: remove an element from an array by value. I'm still far from convinced that there is any benefit to a splice/last approach, even based solely on the grounds that it's slower and more complex than something like this:

        my $idx = firstidx { $_ == $target } @a; splice(@a, $idx, 1) if $idx >= 0;

        Timing for 1e6 entries:

        Rate for firstidx for 24.3/s -- -22% firstidx 31.1/s 28% --

        Certainly in this case, the for approach seems clearly suboptimal (List::MoreUtils' XS code does essentially the same thing as the for loop, much faster and more cleanly). Results were similar with any array sizes where efficiency is likely to matter.

      Acutally, it's more like:

      my @a = ( 1, 2, 3, 4, 2, 5, 7 ); my $last = $#a; my $i = 0; while( $i <= $last ){ if( $a[$i] == 2 ){ $last --; last if $i > $last; } }continue{ $i ++; } $#a = $last;
        Actually, it's more like:

        No, it isn't.

        The OP's code stops at the first occurrence. Yours does not. Hence, you (may) end up removing additional elements, per your own example:

        Test code as copied from original nodes:

        Output:

        shawnhcorey: 1 2 3 4 2 OP vsespb: 1 2 3 4 2 5

        Anyway, this is moot now that the OP has posted actual code which uses splice to remove an internal element rather than the last element via pop.

Re: foreach argument modification inside loop, followed by return from loop
by tobyink (Abbot) on Jul 10, 2013 at 10:32 UTC

    I can't imagine it ever causing problems. It's really the iteration of the loop after the modification where foreach can get confused.

    package Cow { use Moo; has name => (is => 'lazy', default => sub { 'Mooington' }) } say Cow->new->name
    can get confused.
Re: foreach argument modification inside loop, followed by return from loop
by roboticus (Canon) on Jul 10, 2013 at 10:45 UTC

    vsespb:

    I'm with tobyink on this one--since you're leaving your foreach immediately after changing the array, I don't see how it could/should matter.

    ...roboticus

    When your only tool is a hammer, all problems look like your thumb.

Re: foreach argument modification inside loop, followed by return from loop
by sundialsvc4 (Abbot) on Jul 10, 2013 at 11:49 UTC

    If I were to look at that piece of code, without comments, then I would finger it as almost-certainly “a bug,” because I can’t imagine why you would want to pop the last element off of an array if any element in it equals '2.'   I am going to presume, frankly, that you (whoever you were) didn’t write what you thought you wrote, and that you never exhaustively tested it.   Now, I’ve got to figure out what your code actually does, what you meant to write, and what else might be wrong ... nearby or far-away to this questionable subroutine.   (Where there’s one roach in the kitchen, there are many.)

    These admonitions about what to do and what not to do with arrays and lists are really (IMHO) just as much having to do with clarity (and future-proofing as maintenance happens), as with what will or won’t cause the interpreter to behave unexpectedly.   When you are tasked with maintaining hundreds of thousands of lines of years-old smelly code that you didn’t write, “cleverness” is not your friend.

    (Please note that my uses of the word “you” are entirely impersonal ... “you” could be anyone, anywhere, anytime.)

      because I canít imagine why you would want to pop the last element off of an array if any element in it equals '2.
      Don't try to analyze code logic. It's a proof-of-concept code. Perl does not care about it's logic when execute it, so you should not too.
      clarity

      So, if don't wish to see proof-of-concept code without any sane logic, perhaps you want to see real code, here it is.
      do_finish() removes element from @{$self->{jobs_a}}, after that we exit "for" loop in get_task().
      Do you think it will be more clear if it will return number of element to remove, and we remove it outside for loop?
        Do you think it will be more clear if it will return number of element to remove, and we remove it outside for loop?

        Yes, IMHO. My approach, using my own conventions, would be something like this (untested):

        sub do_finish { my $self = shift; my ($jobid_to_delete) = @_; delete $self->{jobs_h}->{$jobid_to_delete}; my $i_to_delete; my $ar_jobs_a = $self->{jobs_a}; JID: for my $i (0 .. $#$ar_jobs_a) { my $jid = $ar_jobs_a->[$i]{jobid}; next JID unless $jid == $jobid_to_delete; $i_to_delete = $i; last JID; } splice @$ar_jobs_a, $i_to_delete, 1 if defined $i_to_delete; return @$ar_jobs_a ? 'ok' : 'done'; }

        Update: Renamed  $jobid to  $jobid_to_delete in interests of self-documentation.

        vsespb, I sense that you have taken-offense at my comment, and I therefore wish to hasten to say that I meant nothing “personal” by it.   You see, through most of my career, I have dealt with “old code.”   None of the programmers who wrote the original code are still there ... they have moved on, and the legacy systems (of course) have not.   Therefore I am constantly (and quite impersonally ... every author is presumed to be a Consummate Pro) “looking for funny smells.”

        I don’t mean anything negative in my description of the sort of things that I have learned to look-for in source code.   Have I, or has anyone on this planet, written code that smells every bit as badly?   Alas, I have but one answer.

Re: foreach argument modification inside loop, followed by return from loop
by BillKSmith (Chaplain) on Jul 10, 2013 at 13:08 UTC
    I first saw this issue severl years ago on the newsgroup perl.misc. Its use greatly simplified the solution to a posted problem. The consensus then was the same as today. It may work, but don't do it!
    Bill
      Are you sure it was about returning from loop immediately after modification of array? Can you find the link? Very interesting!
        Sorry, It was much too long ago for my memory. I have not had access to a newsgroup server for some time. I do not even rember my old username. It was written by a very active user named PerlGurl (not sure of spelling). I reasonably certain that the application was randomizing an array. Notes that I had did not survive a hard drive failure.
        Bill

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others wandering the Monastery: (5)
As of 2014-12-21 08:54 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    Is guessing a good strategy for surviving in the IT business?





    Results (104 votes), past polls