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...
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 $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" );
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 thismy $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};
and this a lot.error_msg( "SR", "OE", $row[5], $row[3],"A STRING","No Email Address." + ) if ( $invalid_sr && !$errnow ); $errnow = 1 if ( $invalid_sr && !$errnow );
In the first case you execute two identical conditional statements. This should be written$mail_server_name = $hash{"MAIL_SERVER"} if ( $node eq "sam" ); $mail_server_name = $hash{"MAIL_SERVER_TEST"} if ( $node eq "cad2" );
And in the second case you execute a conditional, even when you know that is impossible for it to succeed. They should be rewrittenif ( $invalid_sr && !$errnow ) { error_msg( "SR", "OE", $row[5], $row[3],"A STRING","No Email Addres +s." ) $errnow = 1; }
Consider that $node may not equal either of those values for some reason...if ( CONDITION ) { ACTION } elsif ( OTHER-CONDITION ) { ACTION } else { Maybe this shouldnt have been a possibility? Throw an Error? }
BTW, to clarify something that sauoq said this
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 thatsub foo { my $whatzit=shift; #key information for calculating the $snoozle my $baz =shift; #Modifier, allows for negative $snoozle if this +is set
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.sub foo { my $whatzit=shift # the whatzit
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 = new Mail::Sender { smtp => $mail_server_name };
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.$sender = Mail::Sender->new( { smtp => $mail_server_name } );
Not a bad first try. Keep going!
--- demerphq
my friends call me, usually because I'm late....
In reply to Re: Review of first real code
by demerphq
in thread Review of first real code
by SPIDEY
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |