Re: foreach argument modification inside loop, followed by return from loop

[sundialsvc4]

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

[vsespb]

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?

[AnomalousMonk]

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]

Yes, perhaps.

[sundialsvc4]

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.