6.9. Non-Lexical Loop Iterators

Always declare a for loop iterator variable with my.

When using an explicit iterator variable in a for loop, make sure it's explicitly declared as a lexical variable, using the my keyword. That is, never write a for loop like this:

    my $client;

    SEARCH:
    for $client (@clients) {
        last SEARCH if $client->holding(  );
    }

    if ($client) {
        $client->resume_conversation(  );
    }

If you leave off the my, Perl doesn't reuse the lexical variable declared above the loop. Instead, it silently declares a new lexical variable (which is also named $client) as the iterator variable. That new lexical is always scoped to the loop block, and it hides any variable of the same name from any outer scope.

This behaviour is contrary to all reasonable expectation. Everywhere else in Perl, when you declare a lexical variable, it's visible throughout the remainder of its scope, unless another explicit my declaration hides it. So it's natural to expect that the $client variable used in the for loop is the same lexical $client variable that was declared before the loop.

But it isn't. The previous example is actually equivalent to:

    my $client;

    SEARCH:
    for my $some_other_variable_also_named_client (@clients) {
        last SEARCH if $some_other_variable_also_named_client->holding(  );
    }

    if ($client) {
        $client->resume_conversation(  );
    }

Writing it that way makes the logical error in the code much more obvious. The loop isn't setting the outermost lexical $client to the first client who's on hold. It's setting an inner lexical variable (which is also named $client in the original version). Then it's throwing that variable away at the end of the loop. The outer lexical $client retains its original undefined value, and the if block is never executed.

Unfortunately, the first version shown doesn't make that error obvious at all. It looks like it ought to work. It would work if the loop construct was anything other than a for. And that's the problem. Finding this particular bug is made very much more difficult by the counter-intuitive semantics of loop iterators.

Fortunately, if you always use an explicitly declared lexical iterator instead, the problem never arises, because it's obvious that there are two distinct $client variables:

    my $client;

    SEARCH:
    for my $client (@clients) {
        last SEARCH if $client->holding(  );
    }

    if ($client) {
        $client->resume_conversation(  );
    }

Of course, the code is still broken. But now the declaration of a second lexical $client makes the problem obvious. Best practice isn't only about coding in a way that doesn't introduce errors. Sometimes it's also about coding in a way that doesn't conceal errors.

It's simply impossible to use the final value of a loop iterator variable after its for loop has terminated; iterator variables always cease to exist after the loop exits. So the correct solution here is to stop trying to reuse the iterator outside its loop and use a separate variable entirely:

    my $holding_client;

    SEARCH:
    for my $client (@clients) {
        if ($client->holding(  )) {
             $holding_client = $client;
             last SEARCH;
        }
    }

    if ($holding_client) {
        $holding_client->resume_conversation(  );
    }

Or better still, factor the search out into a subroutine:

    sub get_next_holding_client {
        
# Search for and return any client on hold...
        for my $client (@_) {
            return $client if $client->holding(  );
        }

        
# Fail if no clients on hold...
        return;
    }

    

    # and later...

    my $client_on_hold = get_next_holding_client(@clients);

    if ($client_on_hold) {
        $client_on_hold->resume_conversation(  );
    }

Or, best of all, just use a core utility^[*]:

^[*] Core in 5.8 and later. Available on the CPAN for earlier versions of Perl.

    use List::Util qw( first );

    my $client_on_hold = first {$_->holding} @clients;

    if ($client_on_hold) {
        $client_on_hold->resume_conversation(  );
    }

The first function is exactly like a short-circuiting version of grep. You give grep a block and a list of values, and it returns all those values for which the block evaluates true. You also give first a block and a list of values, but then it returns only the first value for which the block is true (and doesn't bother to check the rest of the values).