Thanks for your time Chris. I will do the same when I get a bit better for others.
Let me digest this and get back to you. Gavin. Chris Devers said: > 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>
