perl efficient code

Question

for the sake of becoming a better scripter, is there a way to make the following code in less 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 be writing a lot of these subroutines. Basically I am running this subroutine through a loop of routers and if something doesn't match what I am expecting I want to return back what router is incorrect and what is missing. The code works, but it feels very verbose as I haven't been coding for very long. Thanks in advance for any feedback.

Answer 1

There are a lot of areas for improvement and I think you're better off just trying to improve it step by step, testing it to make sure it works each time.

The thing that jumps out at me the most is the repeated use of the hardcoded IP addresses 67.176.10.2 through 67.176.10.9. You associate some data with each of them (like @r1 and the like). Thus you would be well advised to consider a hash.

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

Here's an example of how you can simplify that first big routine:

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

Notice two things:

1. We construct a hash associating IP addresses to references to arrays (using the \ reference operator). This reference will point to the same array even as it is changed with the push operations.
2. On the last line of the for loop, we retrieve the reference as a scalar (because scalars are references). Then we "dereference" the reference using the @ operator, which returns the underlying array. Then the push works as you would expect.

Look for ways to use associative arrays and references to your advantage. Then you can do lookups in far fewer lines of code and you don't need so many if statements in your loops. Also look for common conditions that hold in all cases, and factor those out of the inner conditionals and putting them at the top of the loop.