in reply to Critique of some perl code.

To be honest, I don't consider this code that bad. Yes, there are some small issues - they mainly suggest the author is not a perl expert - but it seems unlikely that any of them would affect correct behaviour or cause significant performance issues in the context this code is likely to run.

My first concerns would rather be with two things you don't mention: that on failing to open the file for reading (most likely due to permissions), it fails to mention the name of the file it tried to open; and that it stuffs @ARGV with an arbitrarily long list of the words it found, which feels like a possible design flaw.

That last, however, suggests this configuration file is expected rarely to be more than a couple of lines long, so performance of the processing loop is really not likely to be very relevant.

Data is assigned to $_ and then copied to $line but $_ is never used inside the loop.

This is pretty reasonable if as a non-expert you are vaguely aware that while ($line = <$fh>) has some differences around termination conditions compared to while (<$fh>).

This is the line that gobsmacked me. I ran some tests with Benchmark and re 'debug' compared to the FAQ answer "s/\s+$//;" and it turns out that this is a good example of what not to do.

And how much CPU time do you think replacing it would save over the lifetime of the universe? I suspect the total would sit comfortably under a second, maybe even under a millisecond.

Replies are listed 'Best First'.
Re^2: Critique of some perl code.
by Fletch (Bishop) on Apr 18, 2022 at 14:30 UTC

    Doing a while where the conditional is a function of a filehandle iterator (or a couple other iterater-y things) has implicitly done the defined test whether or not there's an assignment to a variable for a while now.

    If the condition expression of a "while" statement is based on any of a group of iterative expression types then it gets some magic treatment. The affected iterative expression types are "readline", the "<FILEHANDLE>" input operator, "readdir", "glob", the "<PATTERN>" globbing operator, and "each". If the condition expression is one of these expression types, then the value yielded by the iterative operator will be implicitly assigned to $_. If the condition expression is one of these expression types or an explicit assignment of one of them to a scalar, then the condition actually tests for definedness of the expression's value, not for its regular truth value.

    The cake is a lie.
    The cake is a lie.
    The cake is a lie.

[reply]
      [...] has implicitly done the defined test [...] for a while now

      Do you know which version introduced that?

      Alexander

      --
      Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
[reply]

        It’s been a while but not specifically no. I may go diving through perldeltas because I’m curious myself.

        Edit: So . . .

        • perl5280delta notes the changes to the while documentation to its current state (commit 5e979393c70, issue).
        • perl518delta notes that while(each %h) was changed to imply while(defined($_ = each %h)) using the phrasing "like readline and readdir" so it's present before then.
        • The oldest possibly related change I've found was commit 54310121b44 which rolls up patches 5.003_86..5.003_95 where it changes perlsyn where the explicit version of the while loop given in the "Loop Control" section has a defined($line = <ARGV>) conditional, so I'm guessing it's possibly a 5.004-ism . . .
        • stopping poking for now . . .

        The cake is a lie.
        The cake is a lie.
        The cake is a lie.

[reply]
[d/l]
[select]
Re^2: Critique of some perl code.
by jwkrahn (Abbot) on Apr 18, 2022 at 03:00 UTC
    This is pretty reasonable if as a non-expert you are vaguely aware that while ($line = <$fh>) has some differences around termination conditions compared to while (<$fh>).

    What "termination conditions" are different between them?

[reply]

      I was also puzzled by the differing "termination conditions." I don't see them.

      Read more... (2 kB)

      Give a man a fish:  <%-{-{-{-<

[reply]
[d/l]
[select]

      I think it's no longer true - and I'm struggling to remember how to test it - but I'm pretty sure there used to be the distinction that while (<>) was special-cased to act as while (defined($_ = <>)), while anything more complicated (such as while ($line = <>)) did not get the "defined" check added to it. So if <> were to return something like an empty string, or "0", the latter case could terminate early.

      The oldest perl I have to hand is a maint-5.8, and I couldn't reproduce it there with perl -e 'print "0\n0"' | perl -we 'while ($r = <>) { printf "%s", $r }' -, so either I'm testing it wrong, or it affected only even earlier perls, or I'm going senile. (These options are not necessarily exclusive.)

      Update but the point was that it could be written by someone with a vague memory that something like that could happen - much like I have - and if so, that didn't necessarily make it awful code.

[reply]
[d/l]
[select]
Re^2: Critique of some perl code.
by jwkrahn (Abbot) on Apr 17, 2022 at 19:30 UTC

    I agree completely.

    I just felt like venting a bit. :)

[reply]

In Section Meditations