On Tue, 31 Aug 2004, Gavin Henry wrote:
# Globals
my $rdiff = '/usr/bin/rdiff-backup';
my $localdir = '/home/ghenry/perl';
my $userhost = "slim_jim\@ssh.magicfx.co.uk";
my $remotedir = '/home/slim_jim/perl';
my $args = "$localdir $userhost\:\:$remotedir";
my $to = "ghenry\@perl.me.uk";
my $from = "ghenry\@perl.me.uk";
my $time = localtime;
my $datestamp = strftime "%d.%m.%y.%T", localtime;
It's nitpicking, but I like lining repetitive lines like these up so
that the differences stand out better, like so:
my $rdiff = '/usr/bin/rdiff-backup';
my $localdir = '/home/ghenry/perl';
my $userhost = "slim_jim\@ssh.magicfx.co.uk";
my $remotedir = '/home/slim_jim/perl';
my $args = "$localdir $userhost\:\:$remotedir";
my $to = "ghenry\@perl.me.uk";
my $from = "ghenry\@perl.me.uk";
my $time = localtime;
my $datestamp = strftime "%d.%m.%y.%T", localtime;
Also, it seems to me like a good habit to minimize putting in literals
when you can help it -- typos are all too easy to make. Therefore,
my $to = "ghenry\@perl.me.uk";
my $from = $to;
If $from needs to be something else later, you can change it. (This is
kind of a thin example, but I'm trying to point out a broader idea.)
# Suretec Message
print "\n----------------------------------------------------------------------------\n";
print "Brought to you by Suretec Systems Ltd.\n";
print "----------------------------------------------------------------------------\n";
Heredocs are popular for statements like this, but I prefer this idiom:
print qq[
------------------------------------------------------
Brought to you by Suretec Systems Ltd.
------------------------------------------------------
------------------------------------------------------
Initialising remote backup synchronsation on $time.B
------------------------------------------------------
];
Less typing; more readable. IMO.
# Send e-mail with a few details for success and failures
# Success
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" );
Again, whitespace makes stuff like this a lot easier to read:
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);
A lot of people like avoiding temp variables like this, and would rather
just combine the `my %mails` declaration into the `sendmail(...)` call.
But whatever, that's a judgement call -- it may make the code a hair
faster, but probably not enough to care about; the bigger issue is
whether the temp variable is more readable, but that's a judgement call.
# Create a success logfile
open LOG, ">>$datestamp-suretec-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";
Again, I think the qq{...} syntax can make this kind of thing cleaner:
print LOG qq{Remote backup completed on $time, with the command:
$rdiff $args
An e-mail has been sent.
};
# Did it work?
die "Backup exited funny: $?" unless $backup == 0;
A lot of people like to `die` immediately when an error comes up, rather
than waiting until the end of the script like this. The common idiom is:
some_risky_thing(...) or die "You sunk my battleship! $!";
or
die "You sunk my battleship! $!" unless some_risky_thing(...);
which, again, comes down to which seems more readable. This can depend a
lot on the particular example -- in this case, I think the first version
looks cleaner, because it puts the common case first, while the second
puts the exceptional case first and so looks backwards. In other cases,
that might be fine though.
