current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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 );
share|improve this question

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

This question came from our site for professional and enthusiast programmers.
1  
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. –  Telemachus Jul 30 '13 at 13:50
    
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. –  Leon K Jul 30 '13 at 13:56
3  
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. –  choroba Jul 30 '13 at 13:58
    
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; –  Hynek -Pichi- Vychodil Jul 30 '13 at 15:21

2 Answers 2

up vote 2 down vote

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}};
share|improve this answer
up vote 1 down vote

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],
    };
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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