Professional Web Applications Themes

Could this be made shorter and cleaner? - PERL Beginners

> It looks like 99% of the code in both blocks is exactly the same. You  Thanks John and Chris. Chris, I have gone for John method of using the $seperator, as I will use that more in this script when I add more features, thanks for the pointer on here doents though. I have taken on board all your suggestions, but I still have two questions: 1. How does the above code get the return code from rdiff to tell if it failed or passed? Is this the $backup ? part, i.e. failed first, then completed second? 2. use ...

  1. #1

    Default Re: Could this be made shorter and cleaner?

    > It looks like 99% of the code in both blocks is exactly the same. You 

    Thanks John and Chris. Chris, I have gone for John method of using the
    $seperator, as I will use that more in this script when I add more
    features, thanks for the pointer on here doents though.

    I have taken on board all your suggestions, but I still have two questions:

    1. How does the above code get the return code from rdiff to tell if it
    failed or passed? Is this the $backup ? part, i.e. failed first, then
    completed second?

    2. use POSIX qw(strftime); I got this from Perl Cookbook. I take it this
    is calling the POSIX strftime and putting it in a list, so when I select
    %T etc. it is picking it from the list? Why does it fail if I put use
    POSIX; and put qw(strftime); somewhere else.


    Oh, and I have the bug now :-) It's a great felling when your script works!!!

    You will be seeing much more of my code in here, so hopefully I will be
    learning and not pasting the same mistakes.

    P.S. I am now a programmer or a scripter, I am not sure is perl is
    programming or scripting? I think programming.

    Gavin.

    Would anyone actually like a me.uk e-mail address?



    --
    Just getting into the best language ever...
    Fancy a me.uk? Just ask!!!
    Gavin Guest

  2. #2

    Default Re: Could this be made shorter and cleaner?

    From: Chris Devers <com> 
    > >
    > > A simple check of the message source is sufficient to preclude virus
    > > etc.[/ref]
    >
    > But why should people have to do that? A simple check of the source
    > in, say, Outlook might be enough to fire off a worm these days -- why
    > make people risk it when inline plain text isn't a question at all?[/ref]

    Previewing the message could fire of a worm in Outlook, whether there
    is an attachment or not is irrelevant.

    - Don't use Outlook
    - Install an antivirus
     

    Not everyone has his own pages to put that to and registering on a
    free web hosting just to be able to ask for help on a script looks a
    bit too much to me.

    Besides ... how do you know the URL doesn't point to a malevolent
    page that'll abuse a hole in the browser and install a worm or
    something?

    Anyway if the code is too long to fit in the body of the email it's
    too long for people to read. We are all busy people here and if
    someone posts a ten pages long script few people will care to read
    it.

    Jenda
    ===== cz === http://Jenda.Krynicky.cz =====
    When it comes to wine, women and song, wizards are allowed
    to get drunk and croon as much as they like.
    -- Terry Pratchett in Sourcery

    Jenda Guest

  3. #3

    Default Re: Could this be made shorter and cleaner?

    Gavin Henry wrote: 
    >
    > Thanks John and Chris. Chris, I have gone for John method of using the
    > $seperator, as I will use that more in this script when I add more
    > features, thanks for the pointer on here doents though.
    >
    > I have taken on board all your suggestions, but I still have two questions:
    >
    > 1. How does the above code get the return code from rdiff to tell if it
    > failed or passed?[/ref]

    The same way it did before.
     

    It seems like you may be confused by the conditional operator. The statement:

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

    Is short for:

    my $msg;
    if ( $backup == 0 ) {
    $msg = 'completed';
    }
    else {
    $msg = 'failed';
    }
     

    No, this is telling perl that the only function we want to import from the
    POSIX module is 'strftime'.
     

    Because the list of function names must follow the module name.

    perldoc -f use



    John
    --
    use Perl;
    program
    fulfillment
    John Guest

  4. #4

    Default RE: Could this be made shorter and cleaner?

    Bob Showalter said: 
    >>
    >> Yup, out of the cookbook.[/ref]
    >
    > Odd. Which cookbook?[/ref]

    2 and 3:

    Recipe 16.2 in version ":

    To avoid the shell, call system with a list of arguments:

    $status = system($program, $arg1, $arg);
    die "$program exited funny: $?" unless $status == 0;

    The returned status value is not just the exit value: it includes the
    signal number (if any) that the process died from. This is the same value
    that wait sets $? to. See Recipe 16.19 to learn how to decode this value.
     

    I get you now. rdiff-backup thinks -v5 --print-statistics in one arg, not
    two.
     

    Gavin Guest

  5. #5

    Default RE: Could this be made shorter and cleaner?

    > Since there's more than one argument to system(), the shell is bypassed, 

    I have split the options up tp $option1 and $option2:

    http://www.perl.me.uk/downloads/rdiff-script

    Is this messy?

    Gavin.


    --
    Just getting into the best language ever...
    Fancy a me.uk? Just ask!!!
    Gavin Guest

  6. #6

    Default RE: Could this be made shorter and cleaner?

    On Tue, 31 Aug 2004, Gavin Henry wrote:
     

    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 withdetails.\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 com
    http://devers.homeip.net:8080/blog/

    np: 'Mah-na Mah-na'
    by Barrio Sésamo
    from 'Sesame Street'
    Chris Guest

  7. #7

    Default RE: Could this be made shorter and cleaner?

    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: 
    >
    > 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 com
    > http://devers.homeip.net:8080/blog/
    >
    > np: 'Mah-na Mah-na'
    > by Barrio Sésamo
    > from 'Sesame Street'[/ref]

    Gavin Guest

  8. #8

    Default RE: Could this be made shorter and cleaner?

    stat is a new one for me, am only on Chapter 7 in Learning Perl.

    I will have to come back to this script when I get a bit further.

    All good though :-)




    --
    Just getting into the best language ever...
    Fancy a me.uk? Just ask!!!
    Gavin Guest

  9. #9

    Default Re: Could this be made shorter and cleaner?

    --As of Tuesday, August 31, 2004 10:07 AM +0100, Gavin Henry is alleged to
    have said:
     

    --As for the rest, it is mine.

    Short answer: Yes. (It is at least one of the two.) ;)

    Long answer... You honestly don't want to get into it. You start
    devolving into arguments over what is 'scripting' versus what is
    'programming', and from there what is best...

    To me, scripting is a sub-class of programming anyway, so why should it
    matter? ;)

    Daniel T. Staal

    ---------------------------------------------------------------
    This email copyright the author. Unless otherwise noted, you
    are expressly allowed to retransmit, quote, or otherwise use
    the contents for non-commercial purposes. This copyright will
    expire 5 years after the author's death, or in 30 years,
    whichever is longer, unless such a period is in excess of
    local copyright law.
    ---------------------------------------------------------------
    Daniel Guest

  10. #10

    Default Re: Could this be made shorter and cleaner?

    On Tue, 2004-08-31 at 10:57 -0400, Daniel Staal wrote: 
    >
    > --As for the rest, it is mine.
    >
    > Short answer: Yes. (It is at least one of the two.) ;)
    >
    > Long answer... You honestly don't want to get into it. You start
    > devolving into arguments over what is 'scripting' versus what is
    > 'programming', and from there what is best...[/ref]

    I would say Perl is at least both. Allow me to devolve..

    The act of writing a perl program is script in in that you're simply (or
    not so simply) instructing /usr/bin/perl to do stuff based on you
    "script". You're running the source code with an interpreter, so to
    speak.

    Perl programing is programing in that the word "programing" is loosely
    defined enough such that even COBOL is a programming language. So long
    as the "language" has constructs that allow you to tell the machine it's
    running on to do something and not just a parsing specification, I'd say
    you'd be hard pressed to argue it's not a language. Whoever said that
    just because your handiwork isn't converted to a format that the kernel
    itself ps into machine code which it then feeds to a CPU that you're
    code isn't a program? What's java? It has an interpreter, and it isn't
    machine code, right? Oh yeah, it's a "run time system"... Oh, I forgot.

    There. Hopefully that made enough sense, and will end all devolving.


    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.2.6 (GNU/Linux)

    iD8DBQBBNLHoubgCGkrWpN4RAn4HAKCnrX9AtvjUgdoPLShpqk 6igqdZFwCfUnV+
    D+ufKg4baloRR4EJilvnKYo=
    =7SwZ
    -----END PGP SIGNATURE-----

    Shawn Guest

Similar Threads

  1. How to make it shorter and better?
    By AlGorr in forum MySQL
    Replies: 2
    Last Post: April 29th, 02:01 PM
  2. shorter way to write a comparison??
    By Pawe in forum PHP Development
    Replies: 5
    Last Post: May 29th, 08:57 AM
  3. sure shorter way to do this?
    By Jonathan Moss in forum PHP Development
    Replies: 2
    Last Post: January 28th, 06:19 PM
  4. Shorter URL Script
    By Chris in forum PHP Development
    Replies: 0
    Last Post: July 19th, 01:45 PM
  5. shorter way maybe?
    By Jon Kraft in forum PHP Development
    Replies: 2
    Last Post: June 27th, 01:01 PM

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139