Review of first real code

SPIDEY has asked for the wisdom of the Perl Monks concerning the following question:

I am asking for your comments about this code I wrote. I am relatively new to perl, and this is one of my first major projects I worked on using perl, and would like feedback to things that I did wrong with relation to perl, ie error checking, better ways to do something, using better perl syntax, etc. Functionally, this program works, and is in use today, but I would like to make perl a standard here, and getting more knowledge is a good thing, IMHO. If you are interested, read on.

This program will call a sqr program inside it, change the output file to html, and send this via email to a recipient. The code should be pretty well commented in respects to what it is doing. If there are any questions. feel free to ask.
Thanks in advance

#!/usr/local/bin/perl
#
# Programmer: Chris Tantalo
# 
# This program will call eleadsht.sqt and email the eleadsht.lis file 
# based on the data in the dmg.eleadsht_parameters table
#

use Mail::Sender;
use DBI;

# check to see if test machine(cad2) or production(sam)
my $node = `hostname`;
chomp $node;

# establish a connection to the database
# change first paramater to the database that you want to connect to
read_config($node);
[download]

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");
chomp $mail_server_name;
$sender = new Mail::Sender {smtp => $mail_server_name};

my @row;

my $dbh = DBI->connect("dbi:Oracle:" . $database_name,"","") 
    or die "Cant connect to db: $DBI::errstr\n";

# statement handler to call sql statements
my $sth = $dbh->prepare("SELECT ep_call_date,ep_prospect_id,ep_nsr,ep_
+sales_rep,ep_product,ep_leadsht_id,ep_status,ep_branch from dmg.elead
+sht_parameters where ep_status in('I','R')")
    or die "Cant prepare SQL statement: $DBI::errstr\n";

# execute sth above
$sth->execute()
    or die "Cant Execute SQL statement: $DBI::errstr\n";


# this is the loop to do stuff with all the returned data from 
# the select statement above, if there is any
while(@row = $sth->fetchrow_array()){
 if($row[6] eq "I")
 {
    my $errnow = 0;

    # run the sqrt report here, which takes a little bit..
    # due to sqr runtime
    `sqrt $sqt_path / $row[0] $row[1] $row[2] $row[4] $row[3]`;

    error_msg("SR","NG",$row[5],$row[3],"Eleadsht.lis not generated du
+e to bad parameters for the lead.","No Email Address.") unless (-e $f
+ilename);
    update_record("N",$row[0],$row[1],$row[2],$row[3],$row[4],$row[5],
+$row[6]) unless (-e $filename);

    next unless (-e $filename);
    create_html($filename,$htmfilename);

    $errnow = 1 if($row[3] eq "UNKNOWN" || $row[3] eq "OPEN");
    # find out which sales rep to email based on $row[3]
    error_msg("SR","OE",$row[5],$row[3],$row[3],"No Email Address.") i
+f($errnow);


    # validate in lts table first
    my $invalid_sr = validate_sales_rep($row[3]);
    error_msg("SR","OE",$row[5],$row[3],"HR Person Id not set for this
+ Sales Rep.  Could not get email address.","No Email Address.") if($i
+nvalid_sr && !$errnow);
    $errnow = 1 if($invalid_sr && !$errnow);

    # call sales_rep_email_address() and return the email address
    my ($sr_email) = sales_rep_email_address($row[3]) unless($errnow);
    error_msg("SR","OE",$row[5],$row[3],"This person does not have an 
+email address in the HR system.","No Email Address.") if($sr_email eq
+ "" && !$errnow);
    $errnow = 1 if($sr_email eq "" && !$errnow);

     ##### email to sales rep person right here #####
     my $sr_recip_id = error_msg("SR","OS",$row[5],$row[3],"Email Sent
+.",$sr_email) unless($errnow);
    mail_it($htmfilename,$row[1],$sr_recip_id,$sr_email,$row[5],$node)
+ unless($errnow);
    
    # reset errnow here for dsa
    $errnow = 0;
    process_dsa_dsm();    

 } # end of if status is an I
 elsif ($row[6] eq "R")
 {
    my $resend_email_address = "";
    # run the sqrt report here, which takes a little bit..
    # due to sqr runtime
    `sqrt $sqt_path / $row[0] $row[1] $row[2] $row[4] $row[3]`;
    create_html($filename,$htmfilename);

    my $sth2 = $dbh->prepare("select erl_email_address from dmg.eleads
+ht_resend_leadsheet where erl_ep_leadsht_id_fk = ?")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth2->bind_param(1,$row[5]);

    # execute sth2 above
    $sth2->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    while( $resend_email_address = $sth2->fetchrow_array)
    {
     ##### email to resend person right here #####
     my $resent_recip_id = error_msg("RES","OS",$row[5],"Resent Leadsh
+eet.","Email Sent.",$resend_email_address);
    mail_it($htmfilename,$row[1],$resent_recip_id,$resend_email_addres
+s,$row[5],$node);

    }


 }
    # after emailing the lead sheet, update the record in the database
    update_record("G",$row[0],$row[1],$row[2],$row[3],$row[4],$row[5],
+$row[6]);
    unlink($htmfilename);
    unlink($filename);

}
warn "Data fetching terminated by early error: $DBI::errstr\n"
    if $DBU::err;

# disconnect from the database 
$dbh->disconnect()
    or warn "error disconnecting: $DBI::errstr\n";

# exits the program
exit;

sub check_inclusion_table
{
    my $dsm_person_id = shift @_;
    my $table_id = "";
    my $found = 0;

    my $sth = $dbh->prepare("SELECT eedi_person_id from dmg.eleadsht_e
+mail_dsm_inclusions ")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    while ($table_id = $sth->fetchrow_array)
    {
        $found = 1 if($table_id == $dsm_person_id);    
    }
    return $found;

}

sub validate_sales_rep
{
    my $sr = shift @_;
    # find sales_rep
    ##my $quoted_sr = $dbh->quote($sr);
    my $sth = $dbh->prepare("SELECT sri_person_id from lts.sales_rep_i
+nformation where sri_sales_rep = ?")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->bind_param(1,$sr);
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    my $sr_id = $sth->fetchrow_array();

    # return the code for being valid or not
    return 1 if($sr_id eq ""); 
    return 0; 
}

sub error_msg
{
    # get parameters being passed in
    my $recipient_type = shift @_;
    my $status_code = shift @_;
    my $leadsht_id = shift @_;
    my $recipient_name = shift @_;
    my $err_msg = shift @_;
    my $email_address = shift @_;


    my $quoted_recipient_name = $dbh->quote($recipient_name);
    my $quoted_recipient_type = $dbh->quote($recipient_type);
    my $quoted_status = $dbh->quote($status_code);
    my $quoted_err_msg = $dbh->quote($err_msg);
    my $quoted_email_address = $dbh->quote($email_address);

    my $sth = $dbh->prepare("SELECT dmg.eleadsht_recipient_id.nextval 
+from sys.dual")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    my $recipient_id = $sth->fetchrow_array();

    my $date = `date`;
        chomp $date;
        my ($day,$month,$numday,$fulltime,$zone,$year) = split(/ +/,$d
+ate);
        $numday = "0" . $numday if($numday <10);
        my $fulldate = $numday . "-".uc($month)."-".$year;
        my $quoted_date = "to_date('$fulldate','DD-MON-YYYY')";
        my ($hour,$min,$sec) = split(/:/,$fulltime);
        $hour = "0" . $hour if($hour <10);
        $fulltime = $hour . $min;

    $dbh->do("insert into dmg.eleadsht_email_log 
        (eel_leadsht_recipient_id,eel_ep_leadsht_id_fk,
         eel_recipient_name,eel_recipient_type,eel_status,
         eel_error_message, eel_email_address,eel_process_date,
         eel_process_time) 
         VALUES ($recipient_id,
             $leadsht_id,$quoted_recipient_name,
             $quoted_recipient_type,
             $quoted_status,$quoted_err_msg,
             $quoted_email_address,$quoted_date,$fulltime)"
         );
    # return the recipient id, in case it is a good email, so we can u
+se
    # this number for the email, to track who it goes to.
    return $recipient_id;

}

sub sales_rep_email_address
{
    # get parameter being passed in
    my $sales_rep = shift @_;

    # find sales_reps email address
    my $sth = $dbh->prepare("SELECT pv_email_address from lts.sales_re
+p_information,dmg.papf_view where sri_sales_rep = ? and pv_person_id 
+= sri_person_id")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->bind_param(1,$sales_rep);
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    my $srep_email_address = $sth->fetchrow_array();

    # return the sales rep email address
    return $srep_email_address;
}

sub mail_it
{
    # name of file to send, should be eleadsht.lis
    my $filename =  shift @_;
    
    # prospect_id
    my $prosp_id = shift @_;

    # recipient_id
    my $rec_id = shift @_;

    # reciever of mail message
    my $emailto = shift @_;

    # leadsheet id
    my $leadsht_id = shift @_;

    # node
    my $node = shift @_;

    my $company_name = get_company_name($prosp_id);

    # subject 
    my $subject = "Lead Sheet ID:$leadsht_id Company: $company_name Pr
+os_id:$prosp_id Rec_id:$rec_id"; 
    

    my @msg = "";
    # open the file to send
    open(MSG,$filename);
    push(@msg,$_) while(<MSG>);
    close(MSG);

    my $from = "";
    $from = "nssmail\@sam.XYZ.com" if($node eq "sam");
    $from = "nssmail\@cad2.XYZ.com" if($node eq "cad2");

    $sender->Open({ from => $from, to => $emailto, subject => $subject
+, headers => "MIME-Version: 1.0\r\nContent-type: text/html\r\nContent
+-Transfer-Encoding: 7bit" }) or die $Mail::Sender::Error,"\n";
    $sender->Send(@msg);
    $sender->Close();

}

sub find_branch
{
    # get arguments being passed in
    my $prosid = shift @_;
    my $prod_code = shift @_;

    my $sth = $dbh->prepare("SELECT ppi_branch from dmg.prospect_produ
+ct_info where ppi_prospect_id = ? and ppi_product = ? ")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->bind_param(1,$prosid);
    $sth->bind_param(2,$prod_code);
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    my $branchnum = $sth->fetchrow_array();
    
    # if branchnum is empty, check dmg.prospect table
    if($branchnum eq "")
    {
        # search for branch again in dmg.prospect table.
        my $sth2 = $dbh->prepare("SELECT pro_pr_branch from dmg.prospe
+ct where pro_prospect_id = ?")
            or die "Cant prepare SQL statement: $DBI::errstr\n";
        $sth2->bind_param(1,$prosid);
        $sth2->execute()
            or die "Cant Execute SQL statement: $DBI::errstr\n";
        $branchnum = $sth2->fetchrow_array();
    }
    # convert to a number field to lose the '00' in front of the numbe
+r
    # otherwise sql lookups based on this number will not work
    $branchnum = $branchnum + 0;
    return $branchnum;
}

sub update_record
{
    my $new_status = shift @_;
    my $scr_date = shift @_;
    my $pros_id = shift @_;
    my $tsr_name = shift @_;
    my $sales_rep_name = shift @_;
    my $prod_code = shift @_;
    my $leadsht_id = shift @_;
    my $curr_status = shift @_;

    my $quoted_new_status = $dbh->quote($new_status);
    my $quoted_scr_date = $dbh->quote($scr_date);
    my $quoted_pros_id = $dbh->quote($pros_id);
    my $quoted_tsr_name = $dbh->quote($tsr_name);
    my $quoted_sales_rep_name = $dbh->quote($sales_rep_name);
    my $quoted_prod_code = $dbh->quote($prod_code);
    my $quoted_curr_status = $dbh->quote($curr_status);


    $dbh->do(" update dmg.eleadsht_parameters
           set ep_status = $quoted_new_status
           where ep_call_date = to_date($quoted_scr_date,'DD-MON-RR')
             and ep_prospect_id = $quoted_pros_id
             and ep_nsr = $quoted_tsr_name
             and ep_sales_rep = $quoted_sales_rep_name
             and ep_product = $quoted_prod_code 
             and ep_leadsht_id = $leadsht_id
             and ep_status = $quoted_curr_status");

}

sub get_company_name
{
    my $prospect_id = shift @_;
    my $comp_name;
    my $sth = $dbh->prepare("SELECT pro_company from dmg.prospect wher
+e pro_prospect_id = ? ")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->bind_param(1,$prospect_id);
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";
    $comp_name = $sth->fetchrow_array;
    return $comp_name;
}

sub read_config()
{
    my $node = shift @_;
    my $cfg_file = "";

    $cfg_file = "/opt/appl/hrstmk/bin/elead.cfg" if($node eq "sam");
    $cfg_file = "/usr/appl/cad/prod/hrstmk/bin/elead.cfg" if($node eq 
+"cad2");
    open(CFG_FILE,"$cfg_file") ||die "cannot open $cfg_file for readin
+g:$!";

    while(<CFG_FILE>)
    {
        next if($_ =~ /^#/);
        ($key,$value) = split(/=/,$_);
        $hash{$key}=$value;
    }

    close(CFG_FILE);
}

sub create_html
{
    my $lisfilename = shift @_;
    my $htmfile = shift @_;
    my $line = "";
    open(IN,"$lisfilename");
    open(OUT,">$htmfile");
    use HTML::FromText;
    $line = <IN>;
    while(<IN>){
        $line .= $_;

    }
    close(IN);
    print OUT text2html($line, urls => 1, pre => 1);
    close(OUT);
}

sub process_dsa_dsm
{
    # find branch number now...
    my $errnow = 0;
    my $no_branch = 0; 
    my $branch_number = $row[7];
    my @row2 = "";
    my $found = 0;
    my $no_branch = 0;
    $no_branch = 1 unless($branch_number);
    error_msg("DSA","OE",$row[5],"DSA","No branch given for this lead,
+ could not send DSA email.","No Email Address.") if($no_branch);
    error_msg("DSM","OE",$row[5],"DSM","No branch given for this lead,
+ could not send DSM email.","No Email Address.") if($no_branch);

    # DSA here
    if(!$no_branch)
    {
        my $branch = $branch_number +0;
        my $quoted_dsa_dsm = $dbh->quote("DSA");
        my $quoted_prod_code = $dbh->quote($row[4]);
        my $sth = $dbh->prepare("SELECT decode(pv_email_address,NULL,e
+tea_email_address,pv_email_address) ,substr(fse_first_name||' '||fse_
+last_name,1,30), fse_person_id from dmg.field_sales_employees,dmg.fie
+ld_sales_assignments, dmg.papf_view, dmg.product, dmg.eleadsht_temp_e
+mail_addresses where fsa_off_nbr = $branch and prod_product = $quoted
+_prod_code and fsa_fse_id_fk = fse_fse_id and fsa_sales_force = prod_
+product_group and fse_person_id = pv_person_id(+) and fsa_job_type = 
+$quoted_dsa_dsm and etea_id(+) = fse_fse_id")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
    $sth->execute()
        or die "Cant Execute SQL statement: $DBI::errstr\n";

        while(@row2 = $sth->fetchrow_array())
        {
            my $dsa_email = $row2[0];
            my $dsa_name = $row2[1];
            my $dsa_pid = $row2[2];
            
            $found++;
            $errnow = 0;
            error_msg("DSA","OW",$row[5],$row2[1],"This DSA has not be
+en assigned a person id in Field Sales Employee.","No Email Address."
+) if($dsa_pid eq "");
            error_msg("DSA","OE",$row[5],$dsa_name,"This DSA does not 
+have an email address in the HR system and there is no temporary addr
+ess.","No Email Address.") if($dsa_email eq "");
            $errnow = 1 if($dsa_email eq ""); 
            $dsa_name = "No Name" if($dsa_name eq "");
             ##### email to dsa person right here #####
             my $dsa_recip_id = error_msg("DSA","OS",$row[5],$dsa_name
+,"Email Sent.",$dsa_email) unless($errnow);
            mail_it($htmfilename,$row[1],$dsa_recip_id,$dsa_email,$row
+[5],$node) unless($errnow);

        }
        # validate dsa exists for this branch
        error_msg("DSA","OE",$row[5],"Branch $branch_number DSA","No D
+SA assigned to this branch in Field Sales Assignments.","No Email Add
+ress.") unless($found);
    }

    if(!$no_branch)
    {
        $found = 0;
        my $branch2 = $branch_number +0;
        my $quoted_dsa_dsm2 = $dbh->quote("DSM");
        my $quoted_prod_code2 = $dbh->quote($row[4]);
        @row2 = "";
        my $sth2 = $dbh->prepare("SELECT decode(pv_email_address,NULL,
+etea_email_address,pv_email_address) ,substr(fse_first_name||' '||fse
+_last_name,1,30), fse_person_id from dmg.field_sales_employees,dmg.fi
+eld_sales_assignments, dmg.papf_view, dmg.product, dmg.eleadsht_temp_
+email_addresses where fsa_off_nbr = $branch2 and prod_product = $quot
+ed_prod_code2 and fsa_fse_id_fk = fse_fse_id and fsa_sales_force = pr
+od_product_group and fse_person_id = pv_person_id(+) and fsa_job_type
+ = $quoted_dsa_dsm2 and etea_id(+) = fse_fse_id")
        or die "Cant prepare SQL statement: $DBI::errstr\n";
        $sth2->execute()
            or die "Cant Execute SQL statement: $DBI::errstr\n";

        while(@row2 = $sth2->fetchrow_array())
        {
            my $dsm_email = $row2[0];
            my $dsm_name = $row2[1];
            my $dsm_pid = $row2[2];
            $found++;
            $errnow = 0;
            error_msg("DSM","OW",$row[5],$row2[1],"This DSM has not be
+en assigned a person id in Field Sales Employee.","No Email Address."
+) if($dsm_pid eq "");
            error_msg("DSM","OE",$row[5],$dsm_name,"This DSM does not 
+have an email address in the HR system and there is no temporary addr
+ess.","No Email Address.") if($dsm_email eq "");
            $errnow = 1 if($dsm_email eq ""); 
            $dsm_name = "No Name" if($dsm_name eq "");

            my $inc_check = 0;
            $inc_check = check_inclusion_table($dsm_pid) unless ($dsm_
+pid eq "" || $errnow);
            error_msg("DSM","NS",$row[5],$dsm_name,"This DSM was exclu
+ded by NSS.",$dsm_email) if(!$inc_check && !$errnow);
            $errnow = 1 unless($inc_check);

             ##### email to dsm person right here #####
             my $dsm_recip_id = error_msg("DSM","OS",$row[5],$dsm_name
+,"Email Sent.",$dsm_email) unless($errnow);
            mail_it($htmfilename,$row[1],$dsm_recip_id,$dsm_email,$row
+[5],$node) unless($errnow);

        }
        # validate dsm exists for this branch
        error_msg("DSM","OE",$row[5],"Branch $branch_number DSM","No D
+SM assigned to this branch in Field Sales Assignments.","No Email Add
+ress.") unless($found);
    }


}

#### end perl code here
[download]

-------------------------------
Just Your Friendly Neighborhood
_SPIDEY_

Comment on Review of first real code Select or Download Code

Replies are listed 'Best First'.
Re: Review of first real code by sauoq (Abbot) on Oct 22, 2002 at 19:46 UTC
This code is difficult to read. You could improve it a lot with better formatting. Very long lines should be avoided. Break them up into shorter lines. Use here documents or string concatenation to get long strings under control. Be consistent. Some notes: You should `use strict;`. Consider turning warnings on too. Read more... (4 kB) Although you are a beginner, the program is reasonably large and making it all work together must have been quite a feat. I applaud the effort. You did, however, make it harder than it needed to be in many ways and the code won't be very easy to maintain. Now that you have it working, I suggest that you write it again. You might think I'm crazy for suggesting it but I bet you would get a lot out of it. Come back with the next version and ask again. :-) -sauoq "My two cents aren't worth a dime.";	[reply] [d/l] [select]
Re: Review of first real code by demerphq (Chancellor) on Oct 22, 2002 at 20:45 UTC
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...} Read more... (5 kB) 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....	[reply] [d/l] [select]
Re: Review of first real code by graff (Chancellor) on Oct 23, 2002 at 02:52 UTC
I would endorse all the advice given so far, especially demerphq's excellent points about getting rid of unnecessary repetitions of "if(...)" statements. I would extend that sort of advice further, in general terms: whenever you find yourself re-typing (or copy/pasting) portions of code and/or large strings of quoted text, you almost certainly have a good opportunity for setting up a subroutine to parameterize a common operation, or a single scalar variable to hold a complicated string that will be used multiple times. Even if a big string may be slightly different from one use to the next, it will be easier to have a single "template" string, with distinct placeholders where the variable parts can be substituted in when needed -- e.g. (not related to your app, but suggestive): `$big_query = "select FIELDMATCH from foo where bar='baz' and ..."; ... ($name_query = $big_query) =~ s/FIELDMATCH/name/; ... ($node_query = $big_query) =~ s/FIELDMATCH/node/; ...` [download] (or, if each use of $big_query involves altering several portions of the text, make it a subroutine, with a hash of arrays to store the appropriate strings for each usage) Basically, anything that's big or complicated -- and is needed more than once -- should only show up once in your code. This is one of the really crucial issues affecting how easy or hard it is to maintain/adapt/extend an application.	[reply] [d/l]
Re: Review of first real code by rdfield (Priest) on Oct 23, 2002 at 09:18 UTC
Some of your SQL statements look very similar to each other in `sub process_dsa_dsm`. You might want to replace the interploted strings with placeholders and pass in the parameters in the execute. This will increase the efficiency of the SQL parsing (as will using table aliases and qualifying all fully column names). Also, I would (personal opinion, of course) refrain from building the SQL directly in the prepare statement, but instead use a scalar variable instead - it can greatly aid debugging, especially when using complex queries, (and, personally I wouldn't be using something as important as `@row` so inconsistently - sometimes global, sometimes a parameter). The parameter passing in `update_record` can be more easily accomplished using a list assignment, e.g. `my ($new_status,$scr_date,$pros_id, $tsr_name,$sales_rep_name,$prod_code, $leadsht_id,$curr_status) = @_;` [download] The code obviously works, so ++ for that, but you might want to reconsider the use of so many globals: maintaining the code as it stands, I believe, would tend to increase the complexity (i.e. increase the 'spagetti'ness). rdfield	[reply] [d/l] [select]
Re: Review of first real code by Sifmole (Chaplain) on Oct 23, 2002 at 12:59 UTC
Update: I didn't see demerphq's Read more tag. This doesn't add much if anything to what he said. Mea culpa. I don't have time to comment on the whole thing, but thought a comment on the following piece of code might be useful to you: 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"); chomp $mail_server_name; [download] First, Where did `%hash` come from? I am assuming that it is a by product of the `read_config($node);` call. It would be more obvious if you commented this, or even better ( IMO ) made `read_config($node);` return a hash-ref. Second, it seems that you have take a liking to post_conditionals. Each of these pairs could be rewritten as `if {} else {}` constructs. With the dual post-conditionals you are forcing the evaluation of both conditions every time, potentially doubling your work. Third, there is a pattern to what you are doing in this block -- refactor it. One suggestion is: `my $postfix = ''; if ( $node eq 'cad2' ) { $postfix = '_TEST'; } my $attrlist = { filename => $hash{"LIS_FILE${postfix}"}, htmfilename => $hash{"HTM_FILE${postfix}"}, sqt_path => $hash{"SQT_PATH${postfix}"}, database_name => $hash{"DB_NAME${postfix}"}, mail_server_name => $hash{"MAIL_SERVER${postfix}"} };` [download] Now obviously your information is stored in a hash and will change the way you refer to the data -- but you could set it up so that you have scalars: `my ($filename, $htmfilename, $sqt_path, $database_name, $mail_server_name) = ('', '', '', '', ''); my @refs = (\$filename, \$htmfilename, \$sqt_path, \$database_name, \$mail_server_name); foreach my $attr ( qw( LIS_FILE HTM_FILE SQT_PATH DB_NAME MAIL_SERVER ) ) { my $store = shift @refs; $$store = $hash{"${attr}${postfix}"}; }` [download] Of course this method will lead to a coupling between the `@refs` and list of fields to be gathered. As well, this code does not test whether the desired `%hash` entry exists. Including that you might get: `my ($filename, $htmfilename, $sqt_path, $database_name, $mail_server_name) = ('', '', '', '', ''); my @refs = (\$filename, \$htmfilename, \$sqt_path, \$database_name, \$mail_server_name); foreach my $attr ( qw( LIS_FILE HTM_FILE SQT_PATH DB_NAME MAIL_SERVER ) ) { my $store = shift @refs; if ( defined $hash{"${attr}${postfix}"} ) { $$store = $hash{"${attr}${postfix}"}; } else { die "Missing required config data: ${attr}${postfix}"; } }` [download]	[reply] [d/l] [select]
Re: Review of first real code by SPIDEY (Initiate) on Oct 23, 2002 at 19:24 UTC
Thank you all for your great comments. I am going to try and re-work this so it is more efficent and easier to maintain. One other question that popped into my head was that during testing, certain variables ended up being null and caused the program to die. Is there a way to trap for that error before hand in the DBI statement without checking each var for a null? something like a try catch block in C? Thanks again for all your great criticism! Chris ------------------------------- Just Your Friendly Neighborhood _SPIDEY_	[reply]