code review

svankalken has asked for the wisdom of the Perl Monks concerning the following question:

Hey everyone, just wanted to post some code to have it reviewed.

#!/usr/bin/perl 
use Net::DNS::Resolver;
use NetAddr::IP;

use vars qw/ %opt /;

######################################################################
+###################
#                                            #
#    Sub to perform DNS lookup                            #
#    Too lazy to write one sub with var for fw/rev so did two instead 
+       #
#                                            #
######################################################################
+###################
sub revlookup
{
    my $res=Net::DNS::Resolver->new;
    $res->nameservers($server);
    my $search = $res->search($input);
    
    if ($search)
    {
        foreach $rr ( $search->answer)
        {
            my $type=$rr->type;
            if ($type eq "A")
            {
                $host=$rr->address;
            }
            if ($type eq "PTR")
            {
                $host=$rr->ptrdname;
            }
            else
            {
                print "$input\t$rr->type\n";
            }
        #print "$input\t$host\n";
    push(@reverseip,$input);
    push (@reversename, $host);
        }
    }
}

sub fwlookup
{
    my $res=Net::DNS::Resolver->new;
    $res->nameservers($server);
    my $search = $res->search($input);
    
    if ($search)
    {
        foreach $rr ( $search->answer)
        {
            my $type=$rr->type;
            if ($type eq "A")
            {
                $host=$rr->address;
            }
            if ($type eq "PTR")
            {
                $host=$rr->ptrdname;
            }
            if ($type eq "CNAME")
            {
                $host=$rr->cname;
            }
            else
            {
                #print "$input\t$rr->type\n";
            }
        #print "$input\t$host\n";
    push(@forwardip,$host);
    push (@forwardname, $input);
        }
    }
    else
    {
        push (@forwardip, $res->errorstring);
        push (@forwardname, $input);
    }
}

######################################################################
+###################
#                                            #
#    sub to check command line options passed to program for validity 
+       #
#                                            #
######################################################################
+###################
sub options
{
    use Getopt::Long;
    if ($#ARGV lt 0)
    {
        &usage;
    }
    GetOptions ("r:s"     => \$cidr,
            "h"     => \$help,
            "s:s"    => \$server);
    &usage if $help;    
    &usage if not $cidr;
    &usage if not $server;
}


######################################################################
+###################
#                                            #
#    sub to display a usage message                            #
#                                            #
######################################################################
+###################
sub usage
{
    print "-h help message\n";
    print "-r [range] to search in CIDR format: 128.0/8\n";
    print "-s [server] to direct queries to\n";
    exit 1;
}


######################################################################
+###################
#                                            #
#    Main program                                    #
#    Too lazy to write sub to do check so just shoved it in here      
+      #
#                                            #
######################################################################
+###################
&options;

my $ip = new NetAddr::IP($cidr);
$range = $ip->range();
$bcast = $ip->broadcast();
print "Searching range: $range: Broadcast $bcast\n";
while ($ip < $ip->broadcast)
{
    ($iponly,$mask) = split /\//, $ip;
    $input=$iponly;
    &revlookup;
    $ip++;
}

foreach (@reversename)
{
   $input=$_;
   &fwlookup;
}


for ($count=0; $count ne $#reversename; $count++)
{
    $revip=$reverseip[$count];
    $revname=$reversename[$count];
    $fwip=$forwardip[$count];
    $fwname=$forwardname[$count];

    if ($revip ne $fwip)
    { 
        print "\n\n";
        print "REVERSE: $revip\t$revname\n";
        print "FORWARD: $fwname\t$fwip\n";
    }
    if ($fwname ne $revname)
    {
        print "\n\n";
        print "WARNING: $revname\t$fwname\n";
    }
}
[download]

I know it's not the best in the world, but it sort of works.

My question is: would it be better to have a variable passing to a lookup sub to say "this is forward" or "this is reverse"
I know that there is unnecessary use of too many arrays here

Anyway, any comments appreciated.

Thanks for the feedback everyone.I'll probably post another updated version in a week or so when I get time to update it.....
comments much appreciated.

Comment on code review Download Code

Replies are listed 'Best First'.
Re: code review by chromatic (Archbishop) on Jul 08, 2004 at 00:03 UTC
My question is: would it be better to have a variable passing to a lookup sub to say "this is forward" or "this is reverse" The two subs look almost exactly the same to me, so I'd return the data from them instead of sticking it in global variables and merge the two subs into one. Aside from unnecessary use of global variables instead of passing parameters and not using strict, I have a few small quibbles: `if ($#ARGV lt 0) { &usage; }` [download] You probably don't want to use string comparisons where you mean numeric comparisons. Also, I personally dislike the Perl 4-style subroutine calls. I'd instead write: `usage() unless @ARGV;` You can also replace C-style loops: `for ($count=0; $count ne $#reversename; $count++) {}` with Perl style loops: `for my $count ( 0 .. $#reversename ) {}` I'd probably do the comparison while looping over IP addresses, though; there's no real need to create the arrays.	[reply] [d/l] [select]
Re: code review by gaal (Parson) on Jul 08, 2004 at 04:55 UTC
`use strict; use warnings;` [download]	[reply] [d/l]
Re: code review by William G. Davis (Friar) on Jul 08, 2004 at 09:45 UTC
Basically what Chromatic and Gaal have already told you, but also: `use vars qw/ %opt /;` [download] Why not just use sub-quotes instead? You're quoting a word, not words. `sub revlookup {` [download] 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);` [download] 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)` [download] Again with the inconsistancy. Why the extra space around $search->answer but not $search? They're both conditionals, right? `sub fwlookup {` [download] The name thing again: try forward_lookup() or forwardLookup() instead. `sub options { use Getopt::Long;` [download] 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.	[reply] [d/l] [select]
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; . . .` [download] 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\.//;`	[reply] [d/l] [select]