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
    In Re your quibble on:

    sub options { use Getopt::Long; . . .

    I've noticed that "use"age in several actual O'Reilly books written by various "Big Name" perl gurus. It seems to me that they do that when the only reason for the "use ...;" is completely within the subroutine. Personally I find that to be kinda self documenting. Plus it really doesn't matter where you place a "BEGIN{}" or "use ...;" as they all get done at compile time in the order they occur in the code.

    However I generally place all "use"ages at the top of the script myself.


    (madams55075.spamtrap.@comcast.net)=~s/\.spamtrap\.//;