# Adding elements to a hash from an output

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] };
}
else {
$data_str{$output_data[5] }
= [ { $output_data[1] =>$output_data[0] } ];
}
}

say Dumper( \%data_str );


## migrated from stackoverflow.comAug 2 '13 at 19:20

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

• 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
• 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

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}};


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],
};
}