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" );
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};
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 );
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" );
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; }
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? }
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
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
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 };
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 } );
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....


In reply to Re: Review of first real code by demerphq
in thread Review of first real code by SPIDEY

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.