in reply to Review of first real code
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:
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.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;
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:
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 $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}"} };
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; $$store = $hash{"${attr}${postfix}"}; }
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}"; } }
|
|---|