Effective perl code

Question

in order to become a better screenwriter, is there a way to make the following code in smaller lines?

sub task1_2_2 { #Make sure syslog configuration is correct my @r3r = my @r4r = my @r5r = my @r6r = my @r7r = my @r8r = ("name = 67.176.10.200","facility-override = local3"); my @r1r = ("name = 67.176.10.200","facility-override = local3","source-address = 67.176.255.1"); my @r2r = ("name = 67.176.10.200","facility-override = local3","source-address = 67.176.255.2"); my (@r1,@r2,@r3,@r4,@r5,@r6,@r7,@r8,%seen,@result1,@result2,@result3,@result4,@result5,@result6,@result7,@result8,$results,$item,$ii); my $pass = "pass"; my $xinfo = shift; my $ip = shift; my $data = XML::LibXML->load_xml(string => $xinfo); my $datax = XML::LibXML::XPathContext->new($data); my $syspath = '/configuration/system/syslog/host/*'; foreach ($datax->findnodes($syspath)) { if($ip eq "67.176.10.2" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r1,$v}; if($ip eq "67.176.10.3" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r2,$v}; if($ip eq "67.176.10.4" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r3,$v}; if($ip eq "67.176.10.5" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r4,$v}; if($ip eq "67.176.10.6" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r5,$v}; if($ip eq "67.176.10.7" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r6,$v}; if($ip eq "67.176.10.8" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r7,$v}; if($ip eq "67.176.10.9" && $_->getName() ne "contents") {my $v = join " = ",$_->getName(),$_->to_literal;push @r8,$v}; } @seen{@r1} = ( ); if($ip eq "67.176.10.2") {foreach $item (@r1r) { push(@result1, $item) unless exists $seen{$item}; }} if(@result1) { foreach $ii (@result1) {$results .= "On R1 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r2} = ( ); if($ip eq "67.176.10.3") {foreach $item (@r2r) { push(@result2, $item) unless exists $seen{$item}; }} if(@result2) { foreach $ii (@result2) {$results .= "On R2 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r3} = ( ); if($ip eq "67.176.10.4") {foreach $item (@r3r) { push(@result3, $item) unless exists $seen{$item}; }} if(@result3) { foreach $ii (@result3) {$results .= "On R3 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r4} = ( ); if($ip eq "67.176.10.5") {foreach $item (@r4r) { push(@result4, $item) unless exists $seen{$item}; }} if(@result4) { foreach $ii (@result4) {$results .= "On R4 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r5} = ( ); if($ip eq "67.176.10.6") {foreach $item (@r5r) { push(@result5, $item) unless exists $seen{$item}; }} if(@result5) { foreach $ii (@result5) {$results .= "On R5 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r6} = ( ); if($ip eq "67.176.10.7") {foreach $item (@r6r) { push(@result6, $item) unless exists $seen{$item}; }} if(@result6) { foreach $ii (@result6) {$results .= "On R6 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r7} = ( ); if($ip eq "67.176.10.8") {foreach $item (@r7r) { push(@result7, $item) unless exists $seen{$item}; }} if(@result7) { foreach $ii (@result7) {$results .= "On R7 Syslog $ii ,is either missing or incorrect configuration was done\n";} } %seen=(); $item=$ii = undef; @seen{@r8} = ( ); if($ip eq "67.176.10.9") {foreach $item (@r8r) { push(@result8, $item) unless exists $seen{$item}; }} if(@result8) { foreach $ii (@result8) {$results .= "On R8 Syslog $ii ,is either missing or incorrect configuration was done\n";} } if($results) { return $results; } elsif(!$results) { return $pass } } 

I am going to write a lot of these routines. Basically, I run this routine through a loop of routers, and if something does not match what I expect, I want to get back which router is wrong and what is not. The code works, but it looks very verbose, since I did not code very long. Thanks in advance for any feedback.

Answer 1

There are many areas for improvement, and I think that you better just try to improve it step by step, checking it to make sure that it works every time.

What pops up on me the most is the reuse of hard-coded IP addresses 67.176.10.2 through 67.176.10.9 . You associate some data with each of them (e.g. @r1 and the like). Thus, it would be useful for you to consider the hash.

"But wait, I can only put the scalars in a hash value!" True, therefore, you need to use links (the tutorial is here ).

Here is an example of how you can simplify this first big procedure:

 sub task1_2_2 { # ... my %ip_address_to_r = ( "67.176.10.2" => \@r1, "67.176.10.3" => \@r2, "67.176.10.4" => \@r3, "67.176.10.5" => \@r4, "67.176.10.6" => \@r5, "67.176.10.7" => \@r6, "67.176.10.8" => \@r7, "67.176.10.9" => \@r8, ); # ... foreach ($datax->findnodes($syspath)) { next unless $_->getName() ne "contents"; # Go to next iteration of loop if we have "contents" my $v = join(" = ", $_->getName(), $_->to_literal); push @{$ip_address_to_r{$ip}}, $v; } 

Pay attention to two things:

• We are constructing a hash associating IP addresses with array references (using the link operator \ ). This link will point to the same array, even if it is modified using push operations.
• In the last line of the for loop, we get the link as a scalar (because scalars are links). Then we “play” the link using the @ operator, which returns the base array. Then push works as you expected.

Look for ways to use associative arrays and references to your benefits. Then you can search in far fewer lines of code, and you don't need as many if in your loops. Also look for general conditions that apply in all cases, and set them out of the internal legend and place them at the top of the loop.