Beefy Boxes and Bandwidth Generously Provided by pair Networks
We don't bite newbies here... much
 
PerlMonks  

Re: Review of first real code

by Sifmole (Chaplain)
on Oct 23, 2002 at 12:59 UTC ( [id://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}"; } }

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others romping around the Monastery: (6)
As of 2024-03-29 01:17 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found