Beefy Boxes and Bandwidth Generously Provided by pair Networks
Keep It Simple, Stupid
 
PerlMonks  

Re: Review of first real code

by Sifmole (Chaplain)
on Oct 23, 2002 at 12:59 UTC ( #207373=note: print w/ replies, xml ) Need Help??


in reply to Review of first real code

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;
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}"} };
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}"}; }
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}"; } }


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

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://207373]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others musing on the Monastery: (9)
As of 2015-07-02 04:32 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The top three priorities of my open tasks are (in descending order of likelihood to be worked on) ...









    Results (27 votes), past polls