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.


In reply to Re: Code Critique by TomDLux
in thread Code Critique by rhiridflaidd

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.