in reply to code review
Basically what Chromatic and Gaal have already told you, but also:
use vars qw/ %opt /;
Why not just use sub-quotes instead? You're quoting a word, not words.
sub revlookup {
revlookup? What does that mean? I'm gonna take a guess and say "reverse lookup," but the only way I could determine that was to look at the routine code itself. I shouldn't have to do that. The name should tell me exactly what the routine does. Why not reverse_lookup() or--if you like CamelCase--reverseLookup() instead?
my $res=Net::DNS::Resolver->new; $res->nameservers($server); my $search = $res->search($input);
Inconsistency does terrible, terrible things to software. How come some of your assignment statements have spaces on either side of the equals sign but others don't?
if ($search) { foreach $rr ( $search->answer)
Again with the inconsistancy. Why the extra space around $search->answer but not $search? They're both conditionals, right?
sub fwlookup {
The name thing again: try forward_lookup() or forwardLookup() instead.
sub options { use Getopt::Long;
That use statement belongs towards the top of the script; it's a compile-time declaration and in this case sticking it inside of a routine doesn't change its behavior, it merely obfuscates it.
I would recommend giving perlstyle a good once-over. You don't have to take it as gospal, but at least consider each and every one of its recommendations and follow them unless you have good reason not to.
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: code review
by Madams (Pilgrim) on Jul 12, 2004 at 00:06 UTC |