in reply to Code review

I'm seeking constructive critisism on some code.

First, I don't think

while (<>) { chomp; s/ //g; tr/a-z/A-Z/; } if (/^GRD/) {
is doing what you think it is. Ponder the difference between
while ( ... ) { ... } if ( ... ) { ... }
and
while ( ... ) { ... if ( ... ) { .... } }
Next, can you explain what   @GRD = split /,"?|""?/; is supposed to be doing? It leads me to believe that there's more to your data than the sample shows. Is your data a CSV file with quoted values?

And what is   close ARGV if eof; supposed to be doing? Is there a file handle named ARGV that you opened previously? If so, that's the not the right way to test for eof.

Your field routine looks like it was written by someone who speaks C, but who hasn't yet come to terms with Perl, either in terms of control structures or in terms of how to deal with variable names. If I'm reading your intent correctly, it can be collapsed into something like

my @GRD = split /,/; warn "line $.: wrong number of attributes\n" unless @GRD == 6; check($GRD[0], 1, @GRDF1); check($GRD[1], 2, @GRDF2); ... check($GRD[5], 6, @GRDF6); sub check ( my($GRD, $col, $@GRDF) = @_; return if grep { $GRD eq $_ } @GRDF; print "line $.: \"$GRD\" is invalid in column $col\n"; }
There are are better ways of doing this, but this gives you an example to chew on that illustrates some of the aspects of Perl that C programmers can gain great advantage in learning.

Replies are listed 'Best First'.
Re: Code review
by Abigail-II (Bishop) on Sep 18, 2003 at 09:27 UTC
    And what is
    close ARGV if eof;
    supposed to be doing? Is there a file handle named ARGV that you opened previously? If so, that's the not the right way to test for eof.

    The answer is yes, there is a file handle named ARGV. And IMO, the use of eof is the right way to test for eof. Here's a quote from perldoc -f eof:

    In a "while (<>)" loop, "eof" or "eof(ARGV)" can be used to detect the end of each file, "eof()" will only detect the end of the last file.
    Now, it's possible that eof in the previous line is used in a wrong way:
    close FH if eof;
    but since the code fragment omits any other use of FH, it's hard to say.

    Abigail

Re: Re: Code review
by mhearse (Chaplain) on Sep 18, 2003 at 04:41 UTC
    Thanks for the input. I'm going to try the routine you suggested. The reason for:
    @GRD = split /,"?|""?/;
    is that the files may contain many different macros, such as:

    ISO,Q,R,L25,F000,HGT, ,120,S,E,N,L00,L00,,3,3,, "C:WHITE"

    I'm splitting up the input at , and ," I've pretty much copied the same structure for all 40 of the other commands (GRD, ISO, ISF, .....). Wondering if there is a way to compact it. Current script is 700+ lines.