in reply to Call for (gentle) critique.

You repeat the exact-same assignment in both the if and thenparts of your program: 


sub copy_run_start {
  if ($regex) {
    my $mib = "$wrmem";
   ...
    }

  } else {
    my $mib = "$wrmem";
    ...
    }
  }
}
sub copy_run_tftp {
  if ($regex) {
    my $mib = "$wrnet$tftpserver";
    ...

# and the same for the else

[download]
It increases the lines of code un-necessarily.

Replies are listed 'Best First'.
Re: Re: Call for (gentle) critique.
by c (Hermit) on Aug 28, 2001 at 20:23 UTC
    Thanks for pointing that out. I think I knew it was there, but was turning my head out of laziness, i replaced all of the subroutines with a single new one that seems to get the job done:

    sub execute {
my $i = shift;
my $mib = "$oid{$i}$tftpserver";
  for my $host(@routers) {
    chomp $host;
    next if (($regex) and ($host !~ /$regex/));
    $filename = "$host.cfg" if ($i eq "wrnet");
    my $s = Net::SNMP->session( -hostname  => $host,
                                -community => $community );
    $s->set_request($mib, OCTET_STRING, $filename);
    my $error = $s->error;
    $s->set_request($oid{wrmem}, INTEGER, "1") if ($commit);
    $s->close;
    if ($error) {
      print "\n$error\n\n";
    } elsif ($i eq "wrnet") {
      print "\nSuccessfully wrote config to tftpserver for $host.\n\n"
+;
    } elsif ($i eq "confnet") {
      print "\nSuccessfully sent config to $host.\n\n";
    }
  }
}

    [download]

    humbly -c

[reply]
[d/l]

In Section Meditations