patternMinor
Can I jump like this?
Viewed 0 times
thiscanlikejump
Problem
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 );Solution
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
Also, depending on what
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).Code Snippets
my @objects = load_objects();
my @valid_objects = grep { $_->is_valid() } @objects;
stuff_that_assumes_valid_object($_) for @valid_objects;
store_objects(@valid_objects);Context
StackExchange Code Review Q#2060, answer score: 3
Revisions (0)
No revisions yet.