Today's theme is: unnecessary variable clutter.
my $fh = new Filehandle("/usr/bin/somecmd someargs |");
while (my $l=<$fh>) {
if ($l=~m/foo bar zot/) {
# lots of code that does not depend on
# a variable file handle, nor creative
# usage of $l where $_ could not be used
# implicitly just as easily
}
}Sure, there are situations where it makes sense to assign filehandles to variables, or using $l instead of $_, but the above example is not one of them. I found no particular reason why the original programmer had used new Filehandle, either.Perl 5 has, on purpose, made this easier for us than in the above example:
open my $FH, '-|', '/usr/bin/somecmd someargs'
or croak "OMG\n";
while (<$FH>) {
if (/foo bar zot/) {
# lots of code
}
}
19 comments:
The use of indirect constructor calls and non-lexical filehandles is also twitchworthy! Here's to twitch-free programming.
I find it easier to use the variable assignment in the while loop all the time instead of having it sometimes and not having it other times. It's a question of having a standard convention usable all the time vs. only some of the time. I dislike using $_ for big loops for a long time and then having to change it sometime down the road when the situation changes.
As for the new and improved regexp, I'm partial to "next unless" :)
It all depends on the code in question.
In this case, using $l instead of $_ resulted in less readable code, not just in the regexp in question.
I'll concede that "next unless" might make sense in some cases, but in this example, it's just an excessively verbose way of writing "if".
Please note that the non-lexical filehandles that chromatic is referring to are in the second example - in your preferred way of doing it. Maybe http://www.modernperlbooks.com/mt/2010/02/a-decade-of-lexical-filehandles.html will convince you to change your ways of doing it.
zby: TIMTOWDI for more than one reason.
I like non-lexical filehandles in some circumstances, and I avoid them in other cases.
I use non-lexical filehandles in quick little scripts with little consequence, which incidentally the above example was taken from.
In more complex scripts where I need to keep track of a bunch of filehandles, I use hashes of filehandles and other "tricks" to keep the code reasonably tidy.
Just as I think "EXPR1 if (EXPR2)" is bad when EXPR1 and EXPR2 are long, I don't necessarily think it's bad when they are short.
Coding Perl is quite a bit about the time spent programming; you often want to save keystrokes for those simple things.
Regarding "next unless" here, I see it as conserving horizontal space for (many?) lines vs. a few extra characters in one line with "if." Choice depends on what you're optimizing for.
Your modified code has introduced a possible bug though. Since you are using $_ but not localizing it, anyone calling your code will have their $_ variable changed. And if they were using in a for loop, your code will have changed the array they were iterating over. Example:
sub bad_loop {
open FH, '-|', 'ls';
while () { }
}
$, = ', ';
$\ = "\n";
my @array = (1..5);
print 'before', @array;
for (@array) {
bad_loop();
}
print 'after', @array;
Haarg:
Please don't take the example out of its context.
If you take things out of context, it's very easy to make things break.
I'm sorry if it isn't clear enough from the blog entry text, but there is no implicit or explicit usage of $_ in the loop-internal code.
The risk for problems as you describe is zero.
I just noticed that my post didn't come through properly. The sub should read:
sub bad_loop {
open FH, '-|', 'ls';
while (<FH>) { }
}
The initial post doesn't give any context of how the code is used externally, so I was pointing out a scenario that could easily occur that would present a problem.
I feel like I am misunderstanding what you are saying, but the example you posted:
open (FH, '-|', '/usr/bin/somecmd someargs') or die "OMG\n";
while (<FH>) {
if (/foo bar zot/) {
# lots of code
}
}
does have an implicit usage of $_. Even if the 'lots of code' section doesn't use it, the fact that you are modifying $_ (via <FH>) without localizing it can present a problem for anyone calling that code if it exists in a sub.
Aha.
Well, I certainly could have been better at providing a full background, but I failed to do so in the interest of brevity.
This time, it apparently reduced the usefulness of the example, I'll try to find a way of including the necessary caveats while maintaining brevity in my next "twitchy" post.
Thanks for the feedback!
I count only 4 keystrokes saved in:
open (FH, '-|', '/usr/bin/somecmd someargs')
versus
open (my $fh, '-|', '/usr/bin/somecmd someargs')
and then perhaps 1 more for each use of $fh. To be honest, that much of saving does not sound worth even the time to consider if it is worth for a particular case :)
zby: it's not all about keystroke. It's about readability, too.
The original code was hard to get a handle on (pun intended).
I am not talking about the original code - I am talking about using FH instead of my $fh. In current Perl (in fact in Perl version not older then 10 years) you don't need to use the Filehandle module to have it. You just write
open my $fh, $mode, $filename
and you are done. If you are so bold to as to propose your way of coding as kind of standard - then please at least make it safe for the beginners to use it. Using FH is not safe and it is also not more readable nor many less keystrokes then my $fh.
zby: I'm not in any way proposing a "standard" for coding.
The title is "Coding styles that make me twitch".
Please note the second to last word.
Your claim that "Using FH is not safe" is context dependant. It is safe in the context of the code in question.
No code, however, is safe is someone changes it.
Your posts sound very authoritative - I would not be surprised if newcomers would treat them as a guide of how to program in Perl. But using non-lexical filehandles is not a good advice. It will blow up only in some circumstances - but in virtually no circumstances it is superior to using lexical ones.
Very well. Please read the edited post.
Fair enough - thanks.
Even though I would just change the code to:
open (my $fh, '-|', '/usr/bin/somecmd someargs') or die "OMG\n";
while (<$fh>) {
if (/foo bar zot/) {
# lots of code
}
}
Although I'm a cranky, semi-old bastard, I can listen to good sense.
The second example is now in line with next-to-last generation best practices.
Nice :) I was afraid I am forcing it too much.
Post a Comment