in reply to Failboat -- An Emotionally Disturbed Tool For Checking NetBackup Client Coverage

spinUp(); collectJobs(); parseJobs(); spinDown(); sub spinUp() {

Why the subroutines?    All the variables are global and nothing is passed to them so it's not to protect local variables.    They are only called once at the beginning of the script so it's not to reuse code.    What is the point?


foreach $item (@tempJobsTable) { $item=~s/\s+/ /g; @thisJob=split(" ",$item);

If the first argument to split is a string containing a single space character then split removes all whitespace characters so your use of a substitution to remove all whitespace characters first is redundant.    You can achieve exactly the same result like this:

foreach (@tempJobsTable) { @thisJob=split;

if ($failureType{$thisJob[6]}=="") { $failureType{$thisJob[6]}=$thisJob[3]; }

You are using a numerical comparison operator on a string so perl will conveniently convert that string to the number 0 to perform the comparison.    Perhaps you meant to use the eq string comparison operator instead?    Or the exists or defined funtions?


$lastValidBackupTime=`/usr/openv/netbackup/bin +/admincmd/bpcatlist -client $client 2>&1 | grep $client | head -n1 | +awk '{print $2}'`;

You are getting a single field from a single line from an externally executed command.    Because AWK splits its fields on whitespace the only whitespace in the returned string will be the newline that the backquotes add.

$lastValidBackupTime=~s/\s+/ /g;

You are converting the newline at the end of the string to a single space character.

@temp=split(" ",$lastValidBackupTime);

You are assigning to $temp[0] the contents of $lastValidBackupTime.

$lastValidBackupTime="$temp[1] $temp[2] $temp[ +3] $temp[4]";

You are assigning to $lastValidBackupTime the string "   ".

Why did you do all that just to assign three spaces to $lastValidBackupTime?

Replies are listed 'Best First'.
Re^2: Failboat -- An Emotionally Disturbed Tool For Checking NetBackup Client Coverage
by Anonymous Monk on Mar 19, 2010 at 04:27 UTC
    What is the point?

    Separation (organization).

Re^2: Failboat -- An Emotionally Disturbed Tool For Checking NetBackup Client Coverage
by Anonymous Monk on Mar 19, 2010 at 18:12 UTC
    Hey, super reply, jwkrahn. Thanks for the critique--seriously. I like having the opportunity to improve my style a bit, which is why I come here.

    In answer to your questions:

    1) I break out my code with subroutines simply for the sake of readability and organization. When I sit down to code, I write a simple block of steps in plain English as to what i'd like to do. From there, I build out the steps in code. If it turns out that over the course of hashing the code out, one of those routines needs to be called repeatedly, then i'm already there. :)

    2) My understanding is that if I were to supply a single whitespace in the split call WITHOUT first boiling out the instances of >1 consecutive whitespaces, I may end up with an array filled with things like " foo" and "bar ", not "foo" and "bar". My goal there was to have an array filled with nothing but values of importance, not interspersed with values of no importance (whitespaces).

    3) You are correct, I am doing a numerical compare on a string, and an exists call would suffice. This is just shorthand on my part. This way, I don't need to distinguish between numerical and non-numerical comparrisons in my code. Helps third-party readability too, I suppose.

    4) I had derived "$lastValidBackupTime" using a different method earlier. The regex to deflate the string is a holdover from that method, and superfluous.

    5) My goal there (if I remember correctly) was to break out a string containing a timestamp to something I could format neatly at will. See #3 above.

    Thanks again for the writeup!
      I break out my code with subroutines simply for the sake of readability and organization.

      That is great for Perl4 code but you should really start using the warnings and strict pragmas in your code.

      2) My understanding is

      split has an exeption to the rule that the first argument is always interpreted as a regular expression and that exeption is a string with a single space character.    It is almost like split( /\s+/ ) except that any leading whitespace is ignored.    The exception is there so that Perl can imitate AWK with the -a switch.