On Tue, 31 Aug 2004, Gavin Henry wrote:
I have split the options up tp $option1 and $option2:
http://www.perl.me.uk/downloads/rdiff-script
Is this messy?
It's not bad, but it still has a lot of repeated code, which should be a warning flag: if you have a lot of code repetition, it should probably be moved out into a subroutine.
Hence, instead of this (with adjusted whitespace):
if ($backup == 0) {
my %mails = (
To => "$to",
From => "$from",
Subject => "Remote backup complete from $ENV{HOSTNAME} on $time",
Message => "The remote backup has been completed on $ENV{HOSTNAME}"
. " on $time with the command:\n\n $rdiff @args\n"
);
sendmail(%mails);
# Success finish message
print "\n", $sep, "Remote backup complete on $time. E-mail sent with
details.\n", $sep;
# Create a success logfile open LOG, ">>$datestamp-rdiff-backup-success.log" or die "Cannot create logfile: $!"; print LOG "Remote backup completed on $time, with the command:\n\n$rdiff @args\n\nAn e-mail has been sent.\n"; close LOG; print "Logfile created on $time.\n\n";
} else { # Failure
my %mailf = (
To => "$to",
From => "$from",
Subject => "Remote backup failed from $ENV{HOSTNAME} on $time",
Message => "The remote backup has failed on $ENV{HOSTNAME}"
. " on $time with the command:\n\n$rdiff @args\n"
);
sendmail(%mailf);
# Failure finish message
print "\n", $sep, "Remote backup failed on $time. E-mail sent with
details.\n", $sep; # Create a failure logfile
open LOG, ">>$datestamp-rdiff-backup-failed.log"
or die "Cannot create logfile: $!";
print LOG "Remote backup failed on $time, with the command:\n\n$rdiff @args\n\nAn
e-mail has been sent.\n";
close LOG;
print "Logfile created on $time.\n\n";
die "Backup exited funny: $?" unless $backup == 0;
}Try this:
my $to = $to;
my $from = $from;
my ( $subject, $message, $log, $logmessage, %stat );
if ( $backup == 0 ) {
# The backup worked
$subject = "Remote backup complete from $ENV{HOSTNAME} on $time";
$message = "The remote backup has been completed on $ENV{HOSTNAME}"
. " on $time with the command:\n\n $rdiff @args\n"
$log = "$datestamp-rdiff-backup-success.log";
$logmessage = "Remote backup completed on $time, with the command:\n\n"
. "$rdiff @args\n\nAn e-mail has been sent.\n";
$stat{now} = "complete";
$stat{then} = "completed";
} else {
# The backup failed
$subject = "Remote backup failed from $ENV{HOSTNAME} on $time";
$message = ""The remote backup has failed on $ENV{HOSTNAME}"
. " on $time with the command:\n\n$rdiff @args\n";
$log = "$datestamp-rdiff-backup-failed.log";
$logmessage = "Remote backup failed on $time, with the command:\n\n"
. "$rdiff @args\n\nAn e-mail has been sent.\n";
$stat{now} = "fail";
$stat{then} = "failed";
} # Report status to console, send a status report, write to log
print "\n", $sep, $message, $sep;
sendmail(
To => "$to",
From => "$from",
Subject => "$subject",
Message => "$message"
); open LOG, ">>$log" or die "Cannot open logfile $log for writing: $!";
print LOG $logmessage;
close LOG;
print "Logfile $log created on $time.\n\n";
die "Backup exited funny: $?" unless $backup == 0;By keeping the if { ... } else { ... } construct as short as I could get it, the code should become easier to read. Because the section that actually does work at the end isn't duplicated, I've reduced the chances of introducing typos.
You could even (debatably) make things a bit more terse & clear by using the %stat variable I introduced, with, e.g.
# Report status to console, send a status report, write to log
print "\n", $sep, $message, $sep;
sendmail(
To => "$to",
From => "$from",
Subject => "$subject",
Message => "The remote backup has $stat{now} on $ENV{HOSTNAME}"
. " on $time with the command:\n\n $rdiff @args\n"
); open LOG, ">>$log" or die "Cannot open logfile $log for writing: $!";
print LOG "Remote backup $stat{then} on $time, with the command:\n\n"
. "$rdiff @args\n\nAn e-mail has been sent.\n";
close LOG;
print "Logfile $log created on $time.\n\n";
die "Backup exited funny: $?" unless $backup == 0;The main benefit of this is that you can then eliminate the $message and $logmessage variables, so the "if { ... } else { ... }" block gets even shorter, and you move even closer to having the common code -- or common text in this case -- not being duplicated unnecessarily.
-- Chris Devers [EMAIL PROTECTED] http://devers.homeip.net:8080/blog/
np: 'Mah-na Mah-na'
by Barrio Sésamo
from 'Sesame Street'
-- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] <http://learn.perl.org/> <http://learn.perl.org/first-response>
