Re: Could this be made shorter and cleaner?

Mon, 30 Aug 2004 23:58:17 -0700

Gavin Henry wrote:
Hi all, 

Hello,

This is my first real go at a perl script. Thoughts?

Subject: Could this be made shorter and cleaner? 

Yes.

# Globals my $rdiff = '/usr/bin/rdiff-backup'; my $localdir = '/home/ghenry/perl'; my $userhost = "[EMAIL PROTECTED]"; my $remotedir = '/home/slim_jim/perl'; my $args = "$localdir $userhost\:\:$remotedir"; my $to = "[EMAIL PROTECTED]"; my $from = "[EMAIL PROTECTED]"; my $time = localtime; my $datestamp = strftime "%d.%m.%y.%T", localtime;

You are using single quotes on strings that do not interpolate but you are using double quotes on strings that have characters that interpolate? Use single quotes unless you *need* double quotes. Make $args an array because system() may invoke the shell if you pass it a scalar.

# Globals
my $rdiff     = '/usr/bin/rdiff-backup';
my $localdir  = '/home/ghenry/perl';
my $userhost  = '[EMAIL PROTECTED]';
my $remotedir = '/home/slim_jim/perl';
my @args      = ( $localdir,  "$userhost\::$remotedir" );
my $to        = '[EMAIL PROTECTED]';
my $from      = '[EMAIL PROTECTED]';
my $time      = localtime;
my $datestamp = strftime '%d.%m.%y.%T', localtime;


# Suretec Message print "\n----------------------------------------------------------------------------\n"; print "Brought to you by Suretec Systems Ltd.\n"; print "----------------------------------------------------------------------------\n";

Your hyphen-separator is used multiple times so you may want to store it in a variable.

my $separator = '-' x 76 . "\n";

# Suretec Message
print "\n", $separator, "Brought to you by Suretec Systems Ltd.\n", $separator;


# Using system command call to give us a return code, with die after if{}else{} block.
my $backup = system( $rdiff, $args );


Using a list instead of scalar:

my $backup = system $rdiff, @args;


# 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" );
    sendmail( %mails );
    # Success finish message
    print 
"\n----------------------------------------------------------------------------\n";
    print "Remote backup complete on $time. E-mail sent with details.\n";
    print 
"----------------------------------------------------------------------------\n";

    # 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";
    close LOG;
    print "Logfile created on $time.\n\n";

    # Failure
    }
else {
    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----------------------------------------------------------------------------\n";
    print "Remote backup failed on $time. E-mail sent with details.\n";
    print 
"----------------------------------------------------------------------------\n";

    # Create a failure logfile
    open LOG, ">>$datestamp-suretec-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";
    }

It looks like 99% of the code in both blocks is exactly the same. You should factor out duplicated code.

my $msg = $backup ? 'failed' : 'completed';

my %mails = (
To => $to,
From => $from,
Subject => "Remote backup $msg from $ENV{HOSTNAME} on $time",
Message => "The remote backup has $msg on $ENV{HOSTNAME} on $time with the command:\n\n $rdiff @args\n"
);
sendmail( %mails );
# finish message
print "\n", $separator, "Remote backup $msg on $time. E-mail sent with details.\n", $separator;

# Create a logfile
open LOG, ">>$datestamp-suretec-backup-success.log" or die "Cannot create logfile: $!";
print LOG "Remote backup $msg 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";




John
--
use Perl;
program
fulfillment

--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
<http://learn.perl.org/> <http://learn.perl.org/first-response>

Reply via email to