All the Perl that's Practical to Extract and Report
use Perl Log In
bart (450)
comment on MJD's Clubbing someone to death with a loaded Uzi
Mark Jason Dominus is a Perl hacker and book author with a blog. In one of his latest entries,
Clubbing someone to death with a loaded Uzi, he rather harshly critiques other people's (beginners) code. But whenever you do that, you should make sure your replacement code isn't dodgy itself.
There is no way to let people comment on the blog, so I'm posting my remarks here.
He writes:
It could have been written like this:
printf FILE "$LOCATION{$location}\,";
printf FILE "%4s", "$min3\,";
printf FILE "%4s", "$max3\,";
printf FILE "%1s", "$wx3\n";
Eww. There's a few red flags in there:
- Don't use
printfwhere your data is used as a template. Granted, in this example, the data comes from a hash that is initialized with literal data stored in the script, but projects tend to evolve, and data just is not a template. If ever the data would contain a "%" sign, this code will blow up. - Why leave in first joining variables with other characters (such as commas), and then format the result with
printf? Put the comma in the template. - There's no need for this to be broken up into 4 statements.
In summary: this code could have become:
printf FILE "%s,%3s,%3s,%1s\n", $LOCATION{$location}, $min3, $max, $wx3;
which is quite a bit shorter, and cleaner too, IMO.
But there are still more things wrong with the original code that he didn't discuss. For example:
foreach $location_name (%LOCATION ) {
$location_code = $LOCATION{$location_name};
...
H*ll, if you want to loop over the keys of a hash, at least don't loop over both the keys and the values! Granted, in MJD's replacement code, the loop is gone, so this problem has disappeared too, but this is an major mistake that shouldn't just be skipped over.
foreach $location_name (keys %LOCATION ) {
...
Otherwise, if ever one of your hash values is also used as a key in this hash, you'll get noise output.
I am guessing that one reason why this person bothers to loop over a hash trying to find a particular hash item, may be to avoid outputting anything in case the item isn't in the hash. MJD just drops this, and outputs a line anyway. So I'm adding that conditional back in:
printf FILE "%s,%3s,%3s,%1s\n", $LOCATION{$location}, $min3, $max, $wx3
if exists $LOCATION{$location};
There. Comments? Anything that I overlooked?
has gotten too inbred in recent years. The Computer Scientists should
pay more attention to the Linguists, who have a much better handle on
how people prefer to communicate.
--Larry Wall
Simplicity (Score:2)
MJD is certainly adept at spotting red flags [plover.com].
--
xoa
The point. (Score:1)
Yes: the point of his post. :-)
It might have been useful if he had added a note to the effect that “this is still not very good, but it’s the first step to cleaning it up” – but a complete cleanup wasn’t what he set out to write about.
Re: (Score:2)
understanding beginners' code (Score:1)
Too harsh (Score:1)
I thought... (Score:1)
But there are still more things wrong with the original code that he didn't discuss. For example:
H*ll, if you want to loop over the keys of a hash, at least don't loop over both the keys and the values! Granted, in MJD's replacement code, the loop is gone, so this problem has disappeared too, but this is an major mistake that shouldn't just be skipped over.
Actually, I thought the entire point of that part of the article was to point out that the user was using a loop to find the element that they could have simply addressed directly... and (I suppose) that is why he didn't address other "issues" in the code. They weren't the point.
At least that was my interpretation...
Re: (Score:1)
the following bit would have been more accurate.