in reply to Code Critique
step zero ... declare your variables. That limits the scope, so you can tell where something exists, where it gets modified. Pass variables in and out of routines, don't just change things globally.
The first thing I would say is the program needs indentation, so you can tell what happens when. Probably the web site mucked up the nice indentation you had going. if you didn't, check out perltidy, you can download it from CPAN.
A long linear program like this hard to understand. Break it into subroutines, and it becomes simpler, more compact. Instead of having to understand a dozen lines of code, the subroutine name explains what is happening ... and at a separate time, you can look at the subroutine body to determine how it's implemented.
Bareword file handles are out-of-date, as is the two-argument open(). Real variables are easier to pass to subroutines or embed in an object, and three-arg open is safer in terms of hacks. Modern style is ...
open my $infile, '<', @ARGV[0] or die ....
Your while() loop uses the auto assignment to $_, then you assign to a variable. Why not just assign into the variable, and save a line?
while ( my $line = <INFILE>) { chomp $line; ...
Your many if() blocks are truly independent, only one will be processed for any line. Use if/elsif to clarify that only one will be true. Put the block bodies into subroutines, to hide away the details. Use a subroutine to determine the 'type' of the current line, and pass back a symbolic representation.
# at the top use Readonly; Readonly my $PATIENT_ID => 0; Readonly my $DOB => 1; # and in the main code ... my %data; my $linetype = determine_type( $line ); if ( $linetype == $PATIENT_ID ) { process_patient( $line, \%data ); elsif ( $linetype == $DOB ) { process_dob( $line, \%data ); elsif ...
If you're using Perl 5.10 or 5.12, you can use a given{} block, or you could use a hash of subroutine references.
given ( determine_type( $line ) ) { when { $PATIENT_ID } { process_patient( $line, \%data ); } when { $DOB } { process_dob( $line, \%data ); } ...
Or in older Perls, if all the options use the input line and update a %data data structure, use a hash:
# define the process_hash near the top # Readonly my %PROCESS => ( $PATIENT_ID => \&process_patient, $DOB => \&process_dob, ... ); # and use it in the loop # my $type = determine_type( $line ); $PROCESS{$type}->( $line, \%data );
As Occam said: Entia non sunt multiplicanda praeter necessitatem.
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: Code Critique
by rhiridflaidd (Novice) on Oct 04, 2010 at 22:09 UTC | |
|
Re^2: Code Critique
by rhiridflaidd (Novice) on Oct 20, 2010 at 21:43 UTC |