Making a hash recognize unique data in Perl

Question

I'm working on a introduction to computer programming final, and I'm coding in Perl.

I'm trying to use a hash to filter a list of IP addresses and push all the unique ones onto an array.

For some reason it's only holding one of the two IPs.

my @broken_data;
my @source_ip;
my @source_ip_mod;
my @destin_ip;
my @destin_ip_mod;
my $file_input;
my $file_output;
my $countline = 0;    # set counter to 0
my $countuser = 0;
my $countpass = 0;

# Command to open the source file for use. Gives user the option of what file to look at.
print "Please enter a file name for diagnosis. \n";
$file_input = <STDIN>;    # file name input
chomp $file_input;
open SF, $file_input or die "Couldn't open Source File: $!\n";    # open the users file

# allows the user to name the File Output
print "Please enter a file name for the summary output. \n";
$file_output = <STDIN>;                          # collects name
chomp $file_output;                              # chomps the input
open(SFO, ">$file_output") or die "Couldn't create $file_output. \n"; # creates a file in current directory for output

while (<SF>) {    # while SF is open
  $countline++;    # counts each line

  if ($_ =~ /USER/i) {
    $countuser++;
  }

  if ($_ =~ /PASS/i) {
    $countpass++;
  }

  chomp($_);

  if ($_ =~ /^22:28/) { # look for any instence of  22:28, ^ to match with the beginning of string

    @broken_data = split(' ', $_);   # takes the data and splits it at the space
    print "$broken_data[0], $broken_data[2], $broken_data[4], $broken_data[-1]\n";    # takes out each element that i need to work with

    print "\tTime: $broken_data[0]\n";    # Prints the time portion of the array

    @source_ip = split('\.', $broken_data[2]);     # splits the source ip at the period

    print "\tSource IP: $source_ip[0].$source_ip[1].$source_ip[2].$source_ip[3] Port: $source_ip[-1]\n"; # Prints the Source  IP

    @destin_ip = split('\.', $broken_data[4]);   # splits the destination ip at the period
    @destin_ip_mod = split(':', $destin_ip[4]);  # cuts off the trailing semi-colon
    $destin_ip[4] = $destin_ip_mod[0];

    print "\tDestination IP: $destin_ip[0].$destin_ip[1].$destin_ip[2].$destin_ip[3] Port: $destin_ip[4]\n";

    print "\tPacket size: $broken_data[-1].\n";
  }
}

my @unique_source_ip;    # creates an array for the unique source ips
my %seen_source_ip;      # hash to sort the data
foreach $_ (@broken_data[2]) {    # foreach loop, setting the Source IP to the default scalar
  if (!$seen_source_ip{$_}) {     # if the source IP has not been seen, put it into the default scalar
    push(@unique_source_ip, $_);  # push the default varriable into the unique source ip array
    $seen_source_ip{$_} = 1;
  }
}

my $unique_source_cnt = @unique_source_ip;

Answer 1

The best I can do is to show you how I would write the same code. I am not saying this would be my solution, as I can't understand exactly what it is you're doing, but I hope you can see that there is a better way to write software.

The biggest differences are

• use strict and use warnings at the start of the program. You should do this with every program you write, no matter how small. Without use strict there is no point in declaring any of your variables with my, as Perl will just use package variables everywhere.

• Indent your code properly, and write comments only when you are writing something very complex. When you do add a comment, never say what the code is doing - the code itself does that. Instead, say why it is doing it. Most of the time your code should be self-explanatory.

• Don't declare all your variables in a block at the top. You may as well not bother at all as I have said. This is usually a legacy of a programmer's experience with C, and it is important to remember that there is almost nothing similar between Perl and C.

• Use the three-parameter form of open and lexical file handles. Incorprate the value of $! in the die string so that you know why the open failed.

The problem is probably in your original line

foreach $_ (@broken_data[2]) { 

which executes the loop just once, setting $_ to the value of $broken_data[2]. The array @broken_data is overwritten on each execution of the while read loop, so when you hit the for you're looking at the data from the last line read. I can't tell what you intended but I'm sure that isn't right.

use strict;
use warnings;

print "Please enter a file name for diagnosis: ";
my $input_fname = <STDIN>;
chomp $input_fname;
open my $in, '<', $input_fname or die "Couldn't open '$input_fname': $!";

print "Please enter a file name for the summary output: ";
my $output_fname = <STDIN>;
chomp $output_fname;
open my $out, '>', $output_fname or die "Couldn't create '$output_fname': $!";

my $n = 0;
my @broken_data;
my ($countuser, $countpass) = (0, 0);

while (<$in>) {
  chomp;
  ++$n;

  ++$countuser if /USER/i;
  ++$countpass if /PASS/i;

  next unless /^22:28/;

  @broken_data = split;
  print join(', ', @broken_data[0, 2, 4, -1]), "\n";

  print "\tTime: $broken_data[0]\n";

  my @source_ip = split /\./, $broken_data[2];

  print "\tSource IP: ", join('.', @source_ip[0,1,2,3,-1]), "\n";

  my @destin_ip     = split /\./, $broken_data[4];
  my @destin_ip_mod = split /:/, $destin_ip[4];
  $destin_ip[4]     = $destin_ip_mod[0];

  print "\tDestination IP: ", join('.', @destin_ip[0..4]), "\n";

  print "\tPacket size: $broken_data[-1].\n";
}

my @unique_source_ip;
my %seen_source_ip;
for ($broken_data[2]) {
  unless ($seen_source_ip{$_}) {
    push(@unique_source_ip, $_);
    $seen_source_ip{$_} = 1;
  }
}

my $unique_source_cnt = @unique_source_ip;