2
\$\begingroup\$

The script currently loops through an output line-by-line, picking out elements of interest and adds them to a hash data structure. If a key already exists (in this case the interface name), it appends data to it.

I can work with a data structure, but I am never happy with the approach I take of looping through the data line by line.

Is there a cleaner way of achieving this?

#!/usr/bin/env perl

use strict;
use warnings;
use feature 'say';

my $output = <<END;
1C:C1:DE:AC:15:C2   192.168.50.16     236263      dhcp-snooping   50    FastEthernet3/0/8
00:24:B5:F7:1A:E3   192.168.58.77     225905      dhcp-snooping   58    FastEthernet3/0/8
00:22:64:A6:E5:C6   192.168.50.61     139103      dhcp-snooping   50    FastEthernet3/0/38
00:1B:25:2E:91:2F   192.168.51.42     222285      dhcp-snooping   51    FastEthernet4/0/15
00:23:7D:4F:18:A5   192.168.51.36     382530      dhcp-snooping   51    FastEthernet4/0/21
C8:F4:06:E0:32:7C   192.168.57.47     258924      dhcp-snooping   57    FastEthernet4/0/21
18:A9:05:1E:5C:B3   192.168.49.21     256946      dhcp-snooping   49    FastEthernet2/0/34
00:22:64:1B:F7:8C   192.168.49.18     192605      dhcp-snooping   49    FastEthernet2/0/21
00:23:7D:50:68:A4   192.168.51.110    381036      dhcp-snooping   51    FastEthernet4/0/26
00:21:E1:FE:E0:94   192.168.57.87     177812      dhcp-snooping   57    FastEthernet2/0/6
00:23:7D:4E:BA:02   192.168.49.80     167839      dhcp-snooping   49    FastEthernet2/0/31
00:23:7D:4F:1A:23   192.168.51.95     293268      dhcp-snooping   51    FastEthernet2/0/36
5C:E2:86:F5:B3:04   192.168.57.48     177809      dhcp-snooping   57    FastEthernet2/0/31
END

my @data = split /\n/, $output;

use Data::Dumper;

my %data_str;
foreach my $line (@data) {
    my @output_data = split /\s+/, $line;
    # Interface already found, append IP and MAC
    if ( exists $data_str{ $output_data[5] }
        && $data_str{ $output_data[5] } eq $data_str{ $output_data[5] } )
    {
        push $data_str{ $output_data[5] },
            { $output_data[1] => $output_data[0] };
    }
    # Add IP address and MAC address to hash
    else {
        $data_str{ $output_data[5] }
            = [ { $output_data[1] => $output_data[0] } ];
    }
}

say Dumper( \%data_str );
\$\endgroup\$

migrated from stackoverflow.com Aug 2 '13 at 19:20

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ The data seems to be one record per line. So why are you unhappy "looping through the data line by line"? To put it another way, this seems tailor made for a line-by-line approach. \$\endgroup\$ – Telemachus Jul 30 '13 at 13:50
  • \$\begingroup\$ Thanks, didn't realise I was actually asking for a review since I thought the code was just plain bad. Colleagues have commented that the code isn't scalable if the input list grows large. Will play around with it myself and checkin with codereview.stackexchange if need be. \$\endgroup\$ – Leon K Jul 30 '13 at 13:56
  • 3
    \$\begingroup\$ What do you mean by $data_str{ $output_data[5] } eq $data_str{ $output_data[5] }? It compares a value to itself, i.e. is always true. In Perl, use autovivification, do not check for existence. \$\endgroup\$ – choroba Jul 30 '13 at 13:58
  • \$\begingroup\$ Your whole inside loop can be my ($mac, $ip, $if) = (split /\s+/, $line)[0,1,5]; push $data_str{$if}, {$ip => $mac}; But I don't understand why you not use hash of hashes so last statement will be $data_str{$if}{$ip} = $mac; \$\endgroup\$ – Hynek -Pichi- Vychodil Jul 30 '13 at 15:21
2
\$\begingroup\$

You don't have to write code for testing of key existence because autovivification will do the work for you. So your cycle can be written as simple:

foreach my $line (@data) {
    my ($mac, $ip, $if) = (split /\s+/, $line)[0,1,5];
    push @{$data_str{$if}}, {$ip => $mac};
}

It is good practice to name your data instead of pollute indexes all over your code. It is much better if they are all in one place. Alternative way is:

my ($mac, $ip, undef, undef, undef, $if) = split /\s+/, $line;

which shows line structure visually.

Another thing is your data structure. Why it has hash value which is array of hashes with IP as one key and mac address. This structure is not good to work with. If I would have to find which MAC belong to given IP I would have to write something like

sub get_mac {
    my ($hash, $if, $ip) = @_;
    for my $x (@{$hash->{$if}}) {
       return $x->{$ip} if exists $x->{$ip};
    }
    return ();
}

which is slow and tedious. Much better is construct two level hash:

foreach my $line (@data) {
    my ($mac, $ip, $if) = (split /\s+/, $line)[0,1,5];
    $data_str{$if}{$ip} = $mac;
}

Now if I would like to know MAC of given IP

my $mac = $data_str{$if}{$ip};

Easy and efficient. I still can get all IPs and MACs

my @ips = keys %{$data_str{$if}};
my @macs = values %{$data_str{$if}};

I can also make original pairs out of this structure

my @pairs = map {+{$_ => $data_str{$if}{$_}}} keys %{$data_str{$if}};
\$\endgroup\$
1
\$\begingroup\$

Use autovivification,

my %data_str;
foreach my $line (@data) {
    my @output_data = split /\s+/, $line;    

    push @{ $data_str{ $output_data[5] } }, {
      $output_data[1] => $output_data[0],
    };
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.