While working on a project for a client recently, I wrote an interesting piece of code, that after I wrote raised more questions than it solved. The summarised code, edited for clarity and focus, is as follows:

#!/usr/bin/perl -Tw

use strict;
use warnings;

my $file = "/usr/local/app/etc/app.conf";
my @match = eval {
    use Fcntl qw/:DEFAULT/;
    local *FH;
    sysopen FH, $file, O_RDONLY;
    flock FH, 8;
    map { chomp; qr/$_/ } <FH>;
} if (-e $file && -r _);

[download]

The intent of this code is to load a series of regular expressions from a file, precompile them and store them in a match array which is referenced later in the program. Once written however, it raised a number of questions in my mind as to the 'correctness' of this code.

Consider the following:

map { chomp; qr/$_/ } <FH>;

[download]

Traditionally, the use of map in this context, without immediate assignment of the results, would be a bad thing. The "preferred" programming style something more like this:

my @match = eval {
    use Fcntl qw/:DEFAULT/;
    local *FH;
    sysopen FH, $file, O_RDONLY;
    flock FH, 8;
    my @map = map { chomp; qr/$_/ } <FH>;
    return @map;
} if (-e $file && -r _);

[download]

Yet in the context of an eval statement where the results are being caught and assigned outside the scope of the block, such lapses in programming style can (I hope) be excepted. Is this 'correct' style though? I am not disregarding the returned results of the map statement from a block level, but from a statement level I am committing a heinous crime

Furthermore, the code within the eval itself ...

{
    use Fcntl qw/:DEFAULT/;
    local *FH;
    sysopen FH, $file, O_RDONLY;
    flock FH, 8;
    map { chomp; qr/$_/ } <FH>;
}

[download]

... One will note that there is no close statement for the opened file handle. Rather than implicitly closing my filehandle, I have relied upon the local scope of the FH file handle. While certainly not "incorrect", this aspect of the code still made me stop and pause upon review.

There is also the use of the underscore filehandle in the -r file test, but this syntax has been discussed extensively previously.

Am I approaching Perl with a style too pragmatic, taking the rope that Perl gives me and slowly tying a noose? Or is this approach welcomed as a more practical approach and exploitation of the feature-set provided?

 

Ooohhh, Rob no beer function well without!

Replies are listed 'Best First'.
Re: Scope and Context
by dws (Chancellor) on Nov 14, 2001 at 05:56 UTC
    Traditionally, the use of map in this context, without immediate assignment of the results, would be a bad thing.

    From the description of eval in perlfunc,

    ... the value returned is the value of the last expression evaluated inside the mini-program; a return statement may be also used, just as with subroutines. The expression providing the return value is evaluated in void, scalar, or list context, depending on the context of the eval itself.
    So your map is executed in a list context, and becomes the return value of the eval (assuming no exception was thrown). Works for me, though explicitly assigning to a temporary array might make the core more readable. Then again, if you're concerned about space before readability, drop in a comment to remind the reader how eval works.

    Your code fragment doesn't show you're checking to see if eval has caught an exception, which it might if there was a bogus regexp in the file.

[reply]
[d/l]
[select]
Re: Scope and Context
by jynx (Priest) on Nov 14, 2001 at 06:29 UTC

    With salt firmly on tongue,

    For something that's not so obvious such as the fact that the map is being returned, it would seem better style to explicitly return it for clarity. The long route is to save the contents in an array and return the array. The short route is to tack a return statment on the fron of the map. Demonstrate:

    my @map = map {chomp; qr/$_/} <FH>;
return @map;

# or

return map {chomp; qr/$_/} <FH>;

    [download]
    Which is clearer? You be the judge, you'll have to maintain it, but i generally go for no unnecessary temporary variables.

    Secondly, the bare _ in the filetest seems just fine since it's coupled with the other file test immediately. If you were doing filetests on the same filename but seperated (even if only be whitespace) it might be clearer to explicitly put in the filename (in this case $file). On the other hand, as said before, you're the one maintaining this, so if it's not clear to you, then it definitely won't be clear to the next guy (as a rule of thumb).

    jynx

[reply]
[d/l]
[select]
Re: Scope and Context
by perrin (Chancellor) on Nov 14, 2001 at 06:55 UTC
    I hope you won't take this the wrong way, but that code is very hard to read. It's cool to do things like this with eval, but not a good idea from a maintenance perspective most of the time. I would have arranged it something like this (untested): 
    my @matches = get_matches($file);

sub get_matches {
    my $file = shift;
    return unless (-e $file && -r _);

    local *FH;
    sysopen FH, $file, O_RDONLY;
    flock FH, 8;
    map { chomp; qr/$_/ } <FH>;
}

    [download]
    I don't think the map is a big deal here, since you want the return value. I'm not sure it's worth messing around with sysopen though (I've never needed to use it, so I don't even know if it still works the way I rearranged it), and hard-coding the constant "8" is probably not a good idea either. I expect you know all this and were just posting to point out other things, but maybe someone else reading this didn't.
[reply]
[d/l]
      I hope you won't take this the wrong way, but ...

      Nah, never taken the wrong way ... *memo to self - add perrin to the list* ... :)

      I understand your point with regard to readability ... I have been guilty of writing some horrendously difficult code to decipher before, purely without intent - The reason for the embedding of the subroutine within the code at this point is so that the regular expressions in the file are loaded entirely within the BEGIN {} block. Yes I can still call the subroutine from this point, but for my own mindset, I like keeping such initialisation code in the one place. Nevertheless, your point is well noted ...

      The hard-coding of the constant 8 is more of an oversight, but one which I should most definitely correct - Guess that's what happens when you get too pensive regarding programming methodology and ignore the practicalities *grin*

       

      Ooohhh, Rob no beer function well without!

[reply]
[d/l]
[select]
        The reason for the embedding of the subroutine within the code at this point is so that the regular expressions in the file are loaded entirely within the BEGIN {} block. Yes I can still call the subroutine from this point, but for my own mindset, I like keeping such initialisation code in the one place.

        You could put the sub definition in the BEGIN block...

[reply]
(tye)Re: Scope and Context
by tye (Sage) on Nov 14, 2001 at 23:16 UTC

    1) Never use statment modifies on my declarations:     my @match= whatever   if whyever; does weird things when whyever is false. Such usage may become an error (or warning) in future releases of Perl.

    2) If you use eval, then you usually need to check and report $@ (you can report it in a non-fatal way).

    3) I don't like any of the uses of map (even those in replies). They all (that I saw) modified $_ inside the map. Several alternatives are possible: 

    chomp( my @lines= <FH> );
return map qr/$_/, @lines;

    [download]
    or 
    return map {
    chomp( my $line= $_ );
    qr/$line/;
} <FH>;

    [download]
    to show a couple. You can do the chomp on a separate line from the my in either of those cases if you prefer.

            - tye (but my friends call me "Tye")
[reply]
[d/l]
[select]
Re: Scope and Context
by mugwumpjism (Hermit) on Nov 19, 2001 at 15:02 UTC

    I think doing this sort of thing is fantastic. You end up doing far less pushing and popping of stuff from the stack.

    Additionally, I've found that: 

    sub foo {

    ...

    $_[0] = $obj
    goto scalar $_[0]->can("method");

}

    [download]

    Doesn't involve the same amount of stack work as:

    sub foo {

    ...

    $obj->(@_[1..$#_]);
}

    [download]

    Bad style? Perhaps. But necessary for any real speed until the optimising compiler in Perl 6 does things like the above for you.

[reply]
[d/l]
[select]
Re: Scope and Context
by petral (Curate) on Nov 14, 2001 at 20:41 UTC
    Why do you use eval instead of do?

      p
[reply]
[d/l]
[select]

Back to Meditations