if ( my $line = readline( $self->{file_handle} ) ) {
    return $line;
} else {
    return;
}
[download]

I appreciate old-school, but I think the quoted code will fail to return the last line of a file if it is '0' with no terminating newline. I haven't tested it, but wouldn't

if (defined(my $line = readline( $self->{file_handle} ))) {
    return $line;
} else {
    return;
}
[download]

Thanks for your contribution, sorry for the late response. In my case I happen to know that will never happen but perfectly valid point. My code is working but I will improve it with your suggestion.

You can't reload or open a new file using the above. Additionally, you can end up having a discrepancy whereas $self->{file} points to one file and $self->{file_handle} to another. If you want that functionality, then I would set file_handle inside get_filehandle() with appropriate logic.

I always run this code on one unique file name at a time, but thanks for the contribution, I appreciate it.

Not to the topic, but there should NEVER an else after a return/exit/croak/die.

A return/die/exit/croak will end the current scope immediately, making the else obfuscating the code that follows, as the code after the else block will never be executed if the if branch is taken.

If I were a code reviewer, that code would be vetoed.

That is ridiculous: the else block is only executed if the condition on the if is false. Since the return is in the if block, it is only executed if the condition tested evaluates to true, which means that the else would never be executed anyway.

This is a perfectly legitimate construct when different return values must be produced in different conditions, or when an early exit is needed under some conditions.

It is not what is executed in the else block, but what is execute after.

if (expression) {
    statement (1);
    }
else {
    statement (2);
    }
statement (3);
[download]

If statement (1) exits the enclosing scope with return/exit/die/croak, statement (3) is never executed. An if/else is the to do something different based on a condition and then continue doing what is after the construct for all cases. This implies that an else immediately after return/exit/die/croak is always useless.

The original code does it right in the first block:

sub get_filehandle {
    my $self = shift;
    $self->{file} = shift;
    open my $fh, "<", $self->{file}
        or die "can't open $self->{file}";
    return $fh;
    }
[download]

Which - when following the wrong method you approve of - would be

sub get_filehandle {
    my $self = shift;
    $self->{file} = shift;
    if (open my $fh, "<", $self->{file}) {
        return $fh;
        }
    else {
        die "can't open $self->{file}";
        }
    }
[download]

In my way, the second block should be reduced to

sub get_lines {
    my $self = shift;
    $self->{file} = shift;
    ### get the filehandle if we don't already have one
    $self->{file_handle} ||= $self->get_filehandle ($self->{file});
    return readline ($self->{file_handle}) || undef;
    }
[download]

That last return is arguable anyway, as the readline method can be a method that legally can return undef, "", and 0 (or any object evaluating as false) indicating that the original code was wrong to start with.


Enjoy, Have FUN! H.Merijn