Hi, I'm the poor schmuck you've been referring to. I've been trying to teach myself Perl in between classes for a couple months now (my college only really offers Java). I know my code is inelegant, but I'm also still really new to Perl. The program being laughed at was my attempt to help a friend from another class--he needed a character counter and he needed it fast, so I tried to hack one together in Perl for him (in case anyone is going to mention cheating, he wasn't turning my code in for an assignment). I'm sorry, I honestly didn't know that my code was so bad. I agree with the above post about responsibility, though I thought I was doing right by making all of my source open and available. The hope was that someone might find a post or two useful if they were trying to learn Perl too. When I found this thread on PerlMonks I laughed a little. You see, about two days ago I noticed that my blog was actually getting traffic and that a lot of it was from PM. I joked with my wife that it was really cool that I was getting referrals from a site like PM...unless I was the "How Not to Code" showcase of the week. Anyway, I didn't realize that my blog was doing harm--I'm going to take it down after I finish this post. Maybe after I've been studying Perl for a few years I'll hazard writing some tutorials for CIS students, but I'll try to go through proper channels to do so. To the couple of you that tried to contact me, email is usually the easiest way: zhespelt at gmail dot com. | [reply] |
I know it's not as elegant as the code above, but I think it's a bit better than my first one.
#!/usr/bin/perl -w
use strict;
use warnings;
local $/ = undef;
my $string = <>;
my @array = split(//, $string);
my %hash;
foreach (@array) {
if ($_ =~ /\w/) {
$hash{$_}++;
}
}
my @keys = sort keys %hash;
my $total;
foreach (values %hash) {
$total += $_;
}
foreach (@keys) {
printf "%s\t%d\t%.3f\n", $_, $hash{$_}, $hash{$_}/$total;
}
print "$total characters\n";
| [reply]
[d/l] |
This is lovely code. It has a nice balance between terseness and clarity.
Despite your baptism of fire, I hope you stay around. You'll learn a lot of stuff, not all necessarily about Perl.
• another intruder with the mooring in the heart of the Perl
| [reply] |
This code is a huge improvement, except for the variable names!
You started with names like $total_chars and %ascii_counts, and ended up with $total and %hash. %hash and @array are about the worst variable names you can use.
You've come a long way very quickly, and kudos for taking the criticism well.
| [reply] |
I was hoping you'd come by. Welcome to the Monastery.
This is 100x better. Well-done. Variable names can be improved, but I can actually scan this code and understand the basic intent. The biggest improvement now would be to skip intermediate variables that only exist once. So, something like:
use strict;
use warnings FATAL => 'all';
use List::Utils qw( sum );
my $input = do {
local $/;
<>;
};
my %letters;
$letters{$_}++ for grep /\w/, split //, $input;
my $total = sum values %letters;
for ( sort keys %letters ) {
printf "%s\t%d\t%.3f\n", $_, $letters{$_}, $letters{$_}/$total;
}
print "$total characters\n";
The big differences there are:
- Removing intermediate variables
- Scoping the localization of $/
- Using a module to do the summation
- Chaining functions in the grep split.
This would be considered more "perlish". Remember - readablity also includes conciseness. This is why well-written Java is less readable than well-written Perl.
My criteria for good software:
- Does it work?
- Can someone else come in, make a change, and be reasonably certain no bugs were introduced?
| [reply]
[d/l] |
| [reply] |
