How to rewrite this eval block

Question

Hi,

This code smells... how do I rewrite it better?

my $record;

eval {
    while (
        # undef $record here, so if getRecord() failed, nothing will be written
        # in the reject file
        do { undef $record; defined( $record = $dataFile->getRecord ) }
    ) {
        $LT_DataFile->encode($record);
    } 
    1;
};

if ( my $error = $@ ) {
    $rejectFile->writeRecord( $error, $record );
}

Thanks.

Answer 1

You do not need a variable in the catch part because when execution arrives there its content will always be undef. Replacing this with a literal value lets us restrict $record to a smaller scope.

use Try::Tiny;
try {
    while (defined(my $record = $dataFile->getRecord)) {
        $LT_DataFile->encode($record);
    }
} catch {
    $rejectFile->writeRecord($_, undef);    # T::T puts error in $_
}

Answer 2

Ok, reworked my answer.

I think the REAL problem here is how you handle the errors. It's confusing at first glance to see a single error handler when you have multiple places where things could go wrong. I see two alternatives.

First, keep it mostly the same as you have now, but specifically check for each type of error:

my $record;
eval {
    while (defined( $record = $dataFile->getRecord )) {
        $LT_DataFile->encode($record);
    } 
};
if (my $error = $@) {
    given ($error) {
        when (/get record error/) {  $rejectFile->writeRecord($_, undef);   }
        when (/encode error/)     {  $rejectFile->writeRecord($_, $record); }
    }
}

This way, you're explicit in how you handle your errors. Of course, with Try::Tiny, this simplifies into the following

my $record;
try {
    while (defined( $record = $dataFile->getRecord )) {
        $LT_DataFile->encode($record);
    } 
} catch {
    when (/get record error/) {  $rejectFile->writeRecord($_, undef);   }
    when (/encode error/)     {  $rejectFile->writeRecord($_, $record); }
}

Alternatively, you could add the lexical record per Daxim's answer. This requires a second eval or try, closer to the problem and adding a last call:

eval {
    while (defined( my $record = $dataFile->getRecord )) {
        eval { $LT_DataFile->encode($record) };
        if (my $error = $@) { $rejectFile->writeRecord($error, $record); last }
    } 
};
if (my $error = $@) {
    $rejectFile->writeRecord($error, undef);
}

Unfortunately this method won't work with Try::Tiny, because the blocks passed to try are actually subrefs.

Answer 3

I would refactor the while loop to

while (defined( $record = $dataFile->getRecord )) {
    $LT_DataFile->encode($record);
} continue {
    undef $record;
}

The continue block is executed after each iteration before conditional is tested. It will still be called if your code uses next to start the next iteration early. If your not likely to call next then simplifying the code to

while (defined( $record = $dataFile->getRecord )) {
    $LT_DataFile->encode($record);
    undef $record;
}

would work as well.