Critiques for my subnet calculator

[schwarznavy]

Howdy all.
I had an assignment to build a subnet calculator that would take (and validate) an IP and CIDR input and return the net_id, broadcast_id, first and last assignable IP addresses, and the number of assignable IP address.
Would anyone care to look at my code and offer criticism?
Thank you.

#! c:\perl -w	
system("pause");	## to allow viewing of warnings 

do	## loop allows user to perform another calculation
{	system("cls");
	$doagain = ""; @bin_octets =(); @bin_subnet_mask = (); 	## initialize values

	do	## loops until IP is valid
	{	$valid = 0;
		print "\n--------------------------------------------------------------------------\n";
		print "\nThis program will calculate the network ID and broadcast ID, first and \nlast assignable IP address, and number of assignable IP addresses based \non the IP address and CIDR you input.";
		print "\n\nEnter your IP address and CIDR (e.g. 192.168.2.1/28): ";
		chomp($input = <STDIN>);

		if ($input =~ /^(22[0-3]|2[0-1][0-9]|1[0-9][0-9]|0?[1-9][0-9]|0{0,2}[1-9])\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/(0?30|0?[12][0-9]|0?0?[89])$/)
		{	for ($x = 1; $x < 5; $x++)
			{	${"oct$x"} = ${"$x"};	}
			$cidr = $5;
			$valid = 1;	## increment $valid to get out of while loop
		} 
		else { print "\n\nInvalid IP or CIDR. Please try again.\n"; }
	} while ($valid ne 1);
	
	for ($x = 1; $x < 5; $x++) ## convert octets to binary
	{	@bin_octets = (@bin_octets, dec_2_bin(${"oct$x"}));	}

	for ($x = 1; $x <= $cidr; $x++) ## Build binary subnet mask from CIDR
	{	@bin_subnet_mask = (@bin_subnet_mask, 1);	}
	for ($x = $cidr + 1; $x <= 32; $x++)
	{	@bin_subnet_mask = (@bin_subnet_mask, 0);	}

	for ($x = 0; $x < 32; $x++) 
	{	$bin_net_id[$x] = $bin_octets[$x] & $bin_subnet_mask[$x]; ## AND mask with octets to find Net_id
		$bin_broadcast[$x] = $bin_octets[$x] | !$bin_subnet_mask[$x]; ## OR octets with mask inverse to find broadcast_id
	}

	for ($i=1; $i<=4; $i++) ## split the 32-bit binary arrays into 4 8-bit arrays and then convert to decimal
	{	@{"bin_net_id$i"} = split32($i, @bin_net_id);
		${"net_id$i"} = bin_2_dec(@{"bin_net_id$i"});
		@{"bin_broadcast$i"} = split32($i, @bin_broadcast);
		${"broadcast$i"} = bin_2_dec(@{"bin_broadcast$i"});
	}

	print "\nReady for the results? "; system("pause"); system("cls");	## print results
	print "\n--------------------------------------------------------------------------\n";
	print "\n\tOriginal IP address and CIDR: \t\t$oct1.$oct2.$oct3.$oct4/$cidr";
	printf("\n\tNumber of assignable IP addresses: \t%d", 2**(32-$cidr)-2);
	print "\n\tNetwork ID: \t\t\t\t$net_id1.$net_id2.$net_id3.$net_id4/$cidr"; 
	printf("\n\tFirst assignable IP: \t\t\t%d.%d.%d.%d/%d", $net_id1,$net_id2,$net_id3,$net_id4+1,$cidr);
	printf("\n\tLast assignable IP: \t\t\t%d.%d.%d.%d/%d", $broadcast1,$broadcast2,$broadcast3,$broadcast4-1,$cidr);
	print "\n\tBroadcast address: \t\t\t$broadcast1.$broadcast2.$broadcast3.$broadcast4/$cidr\n"; 
	print "\n--------------------------------------------------------------------------\n";

	print "\nHit 'Y' if you would you like to calculate another subnet: ";
	chomp($doagain = <STDIN>);
} while ($doagain eq "y" or $doagain eq "Y");

print "\nQuitting ...\n";

########################## SUB FUNCTIONS ##########################
sub dec_2_bin ## to convert a decimal number into an 8-bit binary array
{	split (//, sprintf("%08b", @_), 8);	}

sub bin_2_dec ## to convert a binany number into a decimal number
{	oct("0b" . join("",@_));	}

sub split32  ## used (along with a 'for' loop), to split a 32-bit binary string into 4 8-bit strings
{	my ($a, @b) = @_;
	my $joined = join("",@b);
	if ($joined =~ /([01]{8})([01]{8})([01]{8})([01]{8})/)
	{	${"$a"};	}
}

[mitchems]

Since you're doing this for a class, I suppose the use of Net::IP is not allowed as "cheating". But in the real world, I'd do something like this (from the Net::IP man page):

use Net::IP;
  
  my $ip = new Net::IP ('193.0.1/24') or die (Net::IP::Error());
  print ("IP  : ".$ip->ip()."\n");
  print ("Sho : ".$ip->short()."\n");
  print ("Bin : ".$ip->binip()."\n");
  print ("Int : ".$ip->intip()."\n");
  print ("Mask: ".$ip->mask()."\n");
  print ("Last: ".$ip->last_ip()."\n");
  print ("Len : ".$ip->prefixlen()."\n");
  print ("Size: ".$ip->size()."\n");
  print ("Type: ".$ip->iptype()."\n");
  print ("Rev:  ".$ip->reverse_ip()."\n");

As for your code, using a simple two stage split and pop the values out of an array, would be much easier that using your assignments IMO.

my ($ip,$crd)=split(/\//,$input);
my (@octs)=split(/\./,$ip);
#you can then go through the octs array
for (@octs){
 #do something here with each oct (stored in $_)
}

The way you're doing it in this program seems to me a case of "when I have a hammer (in your case the for($x...) loops) everything looks like a nail".

Deepening your knowledge of hashes and arrays can help you write more efficient and terser code. If there's anything a perl programmer likes it's terseness.

Anyway, nice job going all the way through that.

[m4v]

I was working on a similar project and found it a bit easier to write the regexp as -

if($input =~ m/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/)
{@octet = split(/\./,$input);
    $j = 0;
    foreach $_ (@octet){
    $j++;
    if($_ > 255){
    print "Octet $j must be between 0 and 255.\n";
    undef $input;}
    }
} else
{ print "Bad IP Format.\n";}

[mitchems]

What are you using $j for? The reason I ask is two-fold: 1) you never reference it in this code, except to increment it AND 2) You can do this:

my $count=(@octet)=split(/\./,$input);

and count will be set to the number of elements in the array, eliminating the need for a counting variable.

[m4v]

What are you using $j for? The reason I ask is two-fold: 1) you never reference it in this code, except to increment it AND 2) You can do this:

my $count=(@octet)=split(/\./,$input);

and count will be set to the number of elements in the array, eliminating the need for a counting variable.

I'm still a bit of a newb myself and learning. Just looked easier to read than manually specifying the ranges. That way obviously looks much cleaner.

EDIT: Real quick, let me clarify, because taking a second look at that, it'd print out the total amount of elements in the array. There's going to be 4 octets, I know that, but by counting the way I had posted above, it prints out the specific octet that went over 255.

[mitchems]

I'm still a bit of a newb myself and learning. Just looked easier to read than manually specifying the ranges. That way obviously looks much cleaner.

EDIT: Real quick, let me clarify, because taking a second look at that, it'd print out the total amount of elements in the array. There's going to be 4 octets, I know that, but by counting the way I had posted above, it prints out the specific octet that went over 255.

That is true, you can print out the octet that exceeds 255 (or is less than zero BTW). If you do it the way I suggest you can verify if there are more than (or less than) 4 octets before you do the loop though.

Still, all of that being said, I'd use Net::IP because the heavy lifting has been done already (and this can be said of many, many usual tasks in perl or any other language with reusable modules or classes).

[m4v]

That is true, you can print out the octet that exceeds 255 (or is less than zero BTW). If you do it the way I suggest you can verify if there are more than (or less than) 4 octets before you do the loop though.

That part makes a lotta' sense. Didn't really occur to me to check for more/less than 4 octets. Primarily because only myself and one other would be using the script.

I'll give Net::IP a shot though for some of the other stuff. Thanks!

[mitchems]

BTW, you don't need the $_ in that statement...

foreach $_ (@octet){

That is an automatic feature in perl. As a person that started many years ago with perl, I appreciate those types of things, but many do not. The short-hand of $_, $|, $/, etc. for me is elegant and terse and nice, but many think it is sloppy and can't get used to it.

Someone just asked about perl 6. I think many people are still programming in perl 4 style... myself included at times.