Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

On being 'critical'

by herby1620 (Monk)
on Dec 14, 2006 at 18:56 UTC ( #589897=perlquestion: print w/ replies, xml ) Need Help??
herby1620 has asked for the wisdom of the Perl Monks concerning the following question:

A line in my code says:
open (TagPrint::THELOG, ">&STDOUT") or die "dup: $!";
and this works WONDERFUL. I have no problems with it. It has been running for many months. Now the boss wants to have all the code in the respoitory scanned with Perl::Critic, which I generally accept as a "good thing" (use proper Martha Stewart voice here).

The problem is that the line barfs for a couple of reasons:

1) A bareword file handle and

2) Two argument open.

My question: What is the 'fix'? From the stuff I read, if I want to duplicate a file handle, I should use the two argument form. Also, what do I quote to get around the 'bareword' complaint. The idea was to keep the file handle inside the module (TagPrint) since it is very localized.

Suggestions??

Comment on On being 'critical'
Download Code
Re: On being 'critical'
by TedYoung (Deacon) on Dec 14, 2006 at 19:04 UTC

    You can use lexical file handles under recent version of perl, and you can do the open statement with the 3-arg version too:

    open my $THELOG, '>&', \*STDOUT or die $!;

    Check out open.

    Ted Young

    ($$<<$$=>$$<=>$$<=$$>>$$) always returns 1. :-)
Re: On being 'critical'
by ikegami (Pope) on Dec 14, 2006 at 19:05 UTC
    open(our $TagPrint::THELOG, ">&", \*STDOUT) or die "dup: $!";

    my $THELOG would be much better than our $THELOG, but that might involve big code changes.

    Caveat: Now, you must use $THELOG where you previously used THELOG.

Re: On being 'critical'
by pemungkah (Priest) on Dec 14, 2006 at 19:41 UTC
    The solutions already posted are useful and accurate, but maybe it would be useful to go into the why a bit.

    (As an aside: picking up a copy of Perl Best Practices is a great idea, because it explains all this stuff in detail. However, let me summarize a bit here.)

    First: the lexical filehandle. These guys give you two advantages: one, they're lexicals instead of globals, so it's impossible for someone else to open or close your filehandle (unless you hand it to them, of course). When the lexical goes out of scope, the filehandle will get closed for you.

    In your case, this is a very important consideration: if your code opens the filehandle and then uses it in multiple calls to the subs in TagPrint, you'd need the our variable, or to store a reference to the filehandle scalar in a TagPrint object; that way it won't go out of scope until either program end (the our variable) or the object is destroyed.

    Second: three-argument open separates the mode in which you're opening the file from the file itself. The potential problem we're avoiding is this one:

    open my $fh, $file or die "..";
    This opens $file for input. However, if $file starts with a >, it's opened for output (and clobbered). Three-argument open fixes this, because the second argument is "how do I open this", and the third is "just the filename". Filenames with a leading > now can't screw you up. This isn't really a consideration for you, as you're duping a filehandle, and are using an explicit string.

    Final thoughts: Perl::Critic is not the end source of all Perl wisdom, and TheDamian is not Moses, bringing the Perl Commandments from on high: you've actually chosen a pretty good way of keeping random people from accidentally opening or closing your filehandle by pushing it out of the global namespace. Perl::Critic recommends things because they're ways of making your code more understandable and less likely to have bugs, not because they're the Only Right Way.

      ++ on getting Perl Best Practices. That book has violently changed my coding style :)

        sure was a good read, I particularly like the way that nearly every thing that it said was in contradiction to the development practices at $job

        @_=qw; ask f00li5h to appear and remain for a moment of pretend better than a lifetime;;s;;@_[map hex,split'',B204316D8C2A4516DE];;y/05/os/&print;
Re: On being 'critical'
by Sartak (Hermit) on Dec 14, 2006 at 20:14 UTC

    There's another reason for using the three-arg open. Consider the following statement:

    open(my $handle, $filename) or die "Unable to open $filename: $!";

    This may look OK but it really isn't, especially if $filename is part of the user input. Consider what happens if $filename is "|rm -rf *". This actually does execute rm -rf *, try it out (well, use echo or something a little safer instead). The three-arg open doesn't suffer from this problem:

    open(my $handle, '<', $filename) or die "Unable to open $filename: $!"

    If $filename eq "|rm -rf *" it'll try to open a file named "|rm -rf *" (which will probably fail) and won't clobber your directory.

    Your code doesn't suffer so much from this problem, because you explicitly tell open how you want the file opened. In any case the three-arg open is vastly preferable.

      You state why 3 argument open is preferable in the general case, where the file name is inpredictible, but IMO none of that applies here. I think 2 argument open is perfectly fine here. In fact, it makes more sense to use, than the 3 argument open.

      As for his choice in filehandle... *shrug* It works. Granted, as he wants to use a very localized filehandle, I think use of a my $fhandle (not our!) is warranted.

      Anyway, I do think Perl::Critic is out of line.

      p.s. Did you know the P5P refuse to fix the magical open for <>, it uses 2 argument open internally, so all your considerations apply there? Ask diotalevi about it some time, he can really get pissed off about it. Rightfully so, IMO.

        I completely agree with you that the 2-arg open is fine and even prefereable in this case.

        Did you know the P5P refuse to fix the magical open for <>, it uses 2 argument open internally,

        I haven't made up my mind on this yet, but what do you see wrong with that?

        My current thinking is that the magic open only comes into play when accepting arguments from the command line. And if the user decides to type '|rm -rf' as an argument to a perl script, they could just as easily type 'rm -rf' at that same command line.

        'Fixing' it, to use the 3-arg variant would entail throwing away a bunch of useful behaviours that the user can invoke from the command line.

        Are you aware of any situation where the user could do something by supplying bad args to a magical open that they could not more easily do by simply typing them straight into the command line?


        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.
Re: On being 'critical'
by sauoq (Abbot) on Dec 14, 2006 at 21:13 UTC

    I don't know why everyone else has suggested that you turn that bareword into a variable. Do it like this:

    open (*TagPrint::THELOG, '>&', *STDOUT);
    And that should eliminate both issues without requiring you to change any other code.

    -sauoq
    "My two cents aren't worth a dime.";
      When the boss asked to be compliant with Perl::Critic, s?he was probably (implicitly) asking to follow the lines in PBP, for all the reasons explained in PBP. Resorting to our or to your solution, while keeping Perl::Critic quiet, basically breaks this requirement. Now, a whole thread could start about changing perfectly working code...

      Flavio
      perl -ple'$_=reverse' <<<ti.xittelop@oivalf

      Don't fool yourself.
        When the boss asked to be compliant with Perl::Critic, s?he was probably (implicitly) asking to follow the lines in PBP, for all the reasons explained in PBP.

        Well, even though I once said I expected to never purchase PBP, it seems that I have indirectly done so as I have access to it via my Safari library subscription... So, I took a look at what TheDamian had to say on the subject.

        And it really doesn't seem that it applies very well here.

        The issue TheDamian raises is essentially that of global variables. From the PBP 10.1:

        If that symbol has already been used as a filehandle anywhere else in the same package, executing this open statement will close that previous filehandle and replace it with the newly opened one.
        But the OP had already fully qualified the package and the handle was named "THELOG" rather than something innocuous like "FILE" or "OUT".

        In a private message to bart the other day, I referred to Perl::Critic as a "cargo cult compliance module".

        Applying a practice in every situation simply because it is widely accepted as a best practice is not itself a best practice.

        -sauoq
        "My two cents aren't worth a dime.";
Re: On being 'critical'
by diotalevi (Canon) on Dec 15, 2006 at 19:32 UTC

    I patched Perl::Critic to not warn on two-arg open when using any of the >& features. The latest releases on CPAN should reflect that.

    ⠤⠤ ⠙⠊⠕⠞⠁⠇⠑⠧⠊

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://589897]
Approved by Corion
Front-paged by Corion
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (15)
As of 2014-09-18 16:31 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    How do you remember the number of days in each month?











    Results (118 votes), past polls