Re: Review of first real code

sauoq did a good review, but I have a few thoughts to add.

It cannot be said too many times by too many people how important and useful strict and warnings are. Your code does not have them. This is a serious negative point. The _first_ thing I do when I have to work on code I didnt write is to make sure that these are turned on. Usually when they are something screams or breaks.

_{And now that that little lecture is over...}

This...

my $filename = "";
$filename = $hash{"LIS_FILE"}      if ( $node eq "sam" );
$filename = $hash{"LIS_FILE_TEST"} if ( $node eq "cad2" );
chomp $filename;
my $htmfilename = "";
$htmfilename = $hash{"HTM_FILE"}      if ( $node eq "sam" );
$htmfilename = $hash{"HTM_FILE_TEST"} if ( $node eq "cad2" );
chomp $htmfilename;
my $sqt_path = "";
$sqt_path = $hash{"SQT_PATH"}      if ( $node eq "sam" );
$sqt_path = $hash{"SQT_PATH_TEST"} if ( $node eq "cad2" );
chomp $sqt_path;
my $database_name = "";
$database_name = $hash{"DB_NAME"}      if ( $node eq "sam" );
$database_name = $hash{"DB_NAME_TEST"} if ( $node eq "cad2" );
chop $database_name;

my $mail_server_name = "";
$mail_server_name = $hash{"MAIL_SERVER"}      if ( $node eq "sam" );
$mail_server_name = $hash{"MAIL_SERVER_TEST"} if ( $node eq "cad2" );
[download]

Is meant to provide a machine based configuration right? And note that each "cad2" macine has "_TEST" on the keys... so the above could become:

my $key_ext=$node eq "sam" ? "" : "_TEST";
my $filename         = $hash{"LIS_FILE"     .$key_ext};
my $htmfilename      = $hash{"HTM_FILE"     .$key_ext};
my $htmfilename      = $hash{"HTM_FILE_TEST".$key_ext};
my $sqt_path         = $hash{"SQT_PATH"     .$key_ext};
my $database_name    = $hash{"DB_NAME"      .$key_ext};
my $mail_server_name = $hash{"MAIL_SERVER"  .$key_ext};
[download]

Which while still probably not being the best approach it is at least a lot less code (to change later when you decide a better strategy, like for instance using hashes entirely and doing away with all the random scalars everywhere). But id like to return to this pattern because it occurs throughout your code. You seem to do this

error_msg( "SR", "OE", $row[5], $row[3],"A STRING","No Email Address."
+ )
    if ( $invalid_sr && !$errnow );
$errnow = 1 
    if ( $invalid_sr && !$errnow );
[download]

and this a lot.

$mail_server_name = $hash{"MAIL_SERVER"}      if ( $node eq "sam" );
$mail_server_name = $hash{"MAIL_SERVER_TEST"} if ( $node eq "cad2" );
[download]

In the first case you execute two identical conditional statements. This should be written

if ( $invalid_sr && !$errnow ) {
   error_msg( "SR", "OE", $row[5], $row[3],"A STRING","No Email Addres
+s." )
   $errnow = 1;
}
[download]

And in the second case you execute a conditional, even when you know that is impossible for it to succeed. They should be rewritten

if ( CONDITION ) {
     ACTION
} elsif ( OTHER-CONDITION ) {
     ACTION
} else {
     Maybe this shouldnt have been a possibility? Throw an Error?
}
[download]

Consider that $node may not equal either of those values for some reason...

BTW, to clarify something that sauoq said this

sub foo {
   my $whatzit=shift; #key information for calculating the $snoozle
   my $baz    =shift; #Modifier, allows for negative $snoozle if this 
+is set
[download]

isnt (well, IMO) that bad a style. It lets you see the key variable in a sub, along with useful information about them. His point was more that

sub foo {
   my $whatzit=shift # the whatzit
[download]

is a particularly unuseful comment. In fact, its the kind of comment most people would do without. Use a well chosen variable name and minor comments indicating usage, but never restate the code in comment literally. Comments are about the code, not a mirror of it in english.

One other minor peeve, the indirect object notation is discouraged. Ie:

$sender = new Mail::Sender { smtp => $mail_server_name };
[download]

This does not look right to me. It may work, but it doesnt scan properly. (And Im aware of the possibility that it was that way in its documentation.) This

$sender = Mail::Sender->new( { smtp => $mail_server_name } );
[download]

reads properly. Also in some situations the indirect notation can be parsed by perl very differently than you think. As a beginner its not worth the hassle learning when you can and cannot use it. Just use the direct notation and save yourself the grief.

Not a bad first try. Keep going!

--- demerphq
my friends call me, usually because I'm late....

Comment on Re: Review of first real code Select or Download Code