3
votes
\$\begingroup\$

In my nested for loops below, it is sometimes necessary to exit the outer loop and then continue program execution. Since loop labels and other goto-like constructions are heavily discouraged, I'm a bit afraid of raptors. Is this usage OK? Is there a better way to structure the program?

my @objects = load_objects();
OUTER: for my $obj ( @objects ) {
    for my $element ( @{$obj->elements} ) {
        if ( is_case_1( $element ) ) {
            ...
        } elsif ( is_case_2( $element ) ) {
            ...
        } else {
            warn "invalid element: $element";
            last OUTER;
        }
        stuff_that_assumes_valid_element( $element );
    }
    stuff_that_assumes_valid_object( $obj );
}
store_objects( @objects );
\$\endgroup\$

2 Answers 2

Reset to default
3
votes
\$\begingroup\$

I have two comments to make. The first addresses the use of labels. The second is on why you you might want an exception rather than a logical test.

Loop Labels

Loop labels and loop control are the one common form of goto that isn't generally frowned upon. (The other form is the rare use of a particular form in AUTOLOADER to seamlessly transfer control to generated code--unless you start experimenting with deeper magics you will never need it.)

@rassie's point that your tests should be encapsulated in an is_valid() method for your objects is excellent, but I want to comment on another important aspect of using labeled loops: naming.

Using OUTER as a label is like using %hash as a variable. The name tells a maintenance programmer nothing about what is happening or why.

Perl syntax calls out for a noun-verb naming structure for labels with keywords like next and last. Good names for your outer loop could be OBJECT_TEST, OBJECT_VALIDATION, or OBJECT_CHECK.

Exceptions

When I look at your code I see a structure that is focussed on the normal case. This is fine as long as you have a way to ensure that errors are caught.

If your stuff_that_assumes_valid_object() method checks for validity, then throws an exception (via die), then your code becomes very simple.

This code is identical to your nested loop:

 eval { $_->stuff_that_dies_on_invalid_object() for @objects }; 
 store_objects(@objects);

But if you want the behavior in @rassie's code of filtering out invalid objects and only storing the valid ones, you can do:

my @to_store = grep {
    eval { $_->stuff_that_dies_on_invalid_object; 1 }
} @objects;
store_objects(@to_store);

It's also nice to know that your calls to code that makes assumptions will be safe. Either you expect a failure and catch the exception, or your program terminates on an unexpected, unsafe operation.

P.S. @rassie is absolutely right about the Law of Demeter. Keep your object internals and knowledge about them as limited as possible. Consider making store_objects(), assumes_valid_elements() and assumes_valid_objects() code into methods.

\$\endgroup\$
0
3
votes
\$\begingroup\$

In general, the code is OK, i.e. I don't think it must be changed. However, to increase clarity, I would do a couple of things differently:

It's probably better to write an is_valid() method for your objects, so that whole inner loop is a single function call. Then, you can use grep to shorten the outer loop like this:

    my @objects = load_objects();
    my @valid_objects = grep { $_->is_valid() } @objects;
    stuff_that_assumes_valid_object($_) for @valid_objects;
    store_objects(@valid_objects);

Also, depending on what stuff_that_assumes_valid_element does, you can either put that call into is_valid() or into stuff_that_assumes_valid_object. Either way, your top-level code works with objects and thus shouldn't know too much about single elements inside those objects, that's something the object itself should know and do (see Law of Demeter for reasoning).

\$\endgroup\$