Code critique

Steve Lane sml at zfx.com
Fri Jan 18 18:16:59 CST 2002


Kevin Guidry wrote:
>         Hello all.  Would it be permissable and appropriate to post some
> code here on the list for review and critique?  

i think this is always appropriate and welcome for
lists such as this one.

> I'm not much of a coder,
> so I'd just like some feedback on a short program.  I know it works, but
> I'd like to know if it works well, is organized intelligibly and
> intelligently, etc.  I also know that there are many ways to do the same
> thing in perl and I'd like to know if the decisions I made were the right
> ones.

Kevin Guidry wrote:
>         The code is, in its present form, just a method of getting three
> pieces of information from a user at the console, inserting them
> into a command, and then exec'ing that command.  The specific pieces of
> info are IP address, subnet mask, and gateway.  I do a quick check to make
> sure that they all "look" like IP addresses before passing them to the
> command.  The specific command is a Windows 2000 command "netsh" which
> allows, among other things, you to change your network settings from the
> command line.  In this case, I am just changing the three afore mentioned
> items on a laptop that will be moved around quite a bit.
>         My sincere apologies if this is too long or not
> understandable; please bear with me as I learn.  Without further ado, here
> it is:

as far as working goes... if someone enters a non-IP,
your program sort of implies that the user gets another
chance to enter one, but they don't.  the program die's
whatever the user does.

so... here's my version of your program.  it does the
same thing (except with retry-input until good IP), but
i pulled the input stuff into a sub, and put in a
couple of common Perl idioms.

#!/usr/bin/perl -w
use strict;

my $ip       = get_ip( "Enter the IP address: " );
my $netmask  = get_ip( "Enter the subnet mask: " );
my $gateway  = get_ip( "Enter the default gateway: ");

#exec qq[netsh interface ip set address "Local Area Connection" static $ip $netmask $gateway 1\n];
print qq[netsh interface ip set address "Local Area Connection" static $ip $netmask $gateway 1\n];

exit;

### SUBS

# the three requests for input are the same
# except for the prompt, so make a sub out of them
sub get_ip {
# given a prompt, shows the prompt and
# asks user to enter IP address.
# re-prompts if IP is not valid.
  my $prompt = shift;

  my $ip;
  GET_IP: {   # this starts a block, for the "redo" and "last" later.
    print $prompt;

    # these next two are often combined, into:
    # "chomp( my $ip = <STDIN> );"
    $ip = <STDIN>;
    chomp $ip;
    last GET_IP if checkip( $ip );

    warn "'$ip' is not a valid IP address.  Please try again.\n";
    redo GET_IP;
  }

  return $ip;
}


sub checkip {
# confirm that input is a valid IP address.
# return 1 if so, 0 if not.
  my $ip = shift;
  my @quads = split /\./, $ip;

  # confirm that there are 4 quads,
  # and each quad is an integer,
  # and each integer is 0-255.
  my $QUADS_IN_IP = 4;
  return 0 unless $QUADS_IN_IP == grep {
    /^\d+$/  and
    $_ >= 0  and
    $_ <= 255
  }
    @quads;

  # looks like a good ip.
  return 1;
}

__END__

[sml at gull ~/bin]$ ./kevin-guidry-mine 
Enter the IP address: 45.100.26.1
Enter the subnet mask: 255.300.255.128
'255.300.255.128' is not a valid IP address.  Please try again.
Enter the subnet mask: 48
'48' is not a valid IP address.  Please try again.
Enter the subnet mask: 255.255.255.128
Enter the default gateway: 45.100.26.1
netsh interface ip set address "Local Area Connection" static 45.100.26.1 255.255.255.128 45.100.26.1 1

--
Steve Lane <sml at zfx.com>
http://knoxville.pm.org/




More information about the Knoxville-pm mailing list