Is there a more concise way to write this in Perl?

Question

This subroutine is passed an array of file handles to close, which it closes one by one using the foreach loop:

sub closef
{
    foreach(@_) {
        my $fh = shift;
        close $fh;
    }   
}

Where all can this simple subroutine be modified to make it better?

Should I use close shift instead of doing it in two lines?

Answer 1

foreach(@_) {
    close $_;
}   

or

foreach my $fh (@_) {
    close $fh;
}   

or

close $_ foreach (@_);

Answer 2

sub closef { close $_ for @_ }

But the close function does have a return value and you would usually do well to examine it. Therefore,

sub closef { map { close $_ } @_ }

Answer 3

The most concise way is to use lexical filehandles which automatically close when they go out of scope:

sub firstline {
    open( my $in, shift ) && return scalar <$in>;
    # no close() required
}

See perldoc perlopentut.

Answer 4

Or you can also use map:

map { close $_ } @_;

With map you can even get an array of all results of close calls if you want to process it.

my @results = map { close $_ } @_;

Answer 5

Your code isn't doing what you think. You say you want to close each filehandle passed to the routine, but you only close some of them. Consider the following example:

sub t{
        foreach(@_){
                my $tmp = shift;
                print $tmp;
        }
}

t(('a'...'d'));

This will output

ab

Despite the argument list being a, b, c and d. This is because on each iteration shift is being called. To get the kind of behavior you want use

sub t{
        foreach(@_){
                my $tmp = $_;
                print $tmp;
        }
}

t(('a'...'d'));

Or, better, don't make a copy of $_, just alias it:

sub t{
        foreach my $tmp (@_){
                print $tmp;
        }
}

t(('a'...'d'));

Answer 6

There's an issue with the subroutine originally posted.

The shift function expects an array as an argument. The implicit variable inside the foreach is a scalar.

This means that $fh remains uninitialized. The reason why it probably goes unnoticed is because (as eugene y points out) lexical filehandles close themselves when they go out of scope.

Incidentally, use warnings; will notify about such things.

A more idiomatic approach would be to use a while loop:

sub closef {

    while ( my $fh = shift @_ ) {
        close $fh;
    }
}

For more compact notation, a for loop is the way to go:

sub closef { close for @_; }

The equivalent with a while loop is not so readable:

sub closef { close while $_ = shift }

Update

Michael Carman's comment below holds true here. The shift modifies the array being iterated over, which makes it an unsafe operation. This is, in fact, the problem.