mhearse has asked for the wisdom of the Perl Monks concerning the following question:

I'm seeking constructive critisism on some code. The whole purpose of this script is to make sure a user enters macro commands correctly. Here is an example of the contents of a file which would be checked:
GRD,G,B,L00,HGT,S,10
Here is the script which does the work. Unfortunately, the system this will run on has only the perl 5.05 executable. No modules are available. Any input would be helpful. 

   while (<>) {
      chomp;
      s/ //g;
      tr/a-z/A-Z/;
      } if (/^GRD/) {
         @GRD = split /,"?|""?/;
         while (@GRD) {
            attribs (\@GRD, 6);
            field (\@GRD, 1, \@GRDF1);
            field (\@GRD, 2, \@GRDF2);
            field (\@GRD, 3, \@GRDF3);
            field (\@GRD, 4, \@GRDF4);
            field (\@GRD, 5, \@GRDF5);
            field (\@GRD, 6, \@GRDF6);
            last;
         }
      } elsif (/^\s*$/) {
         next;
      } elsif (/^\*/) {
         next;
      } else {
         s/\W.*//;
         chomp $_;
         warn "$ARGV ling $.: \"$_\" is not a valid APG command!\n";
      }
   close FH if eof;
   close ARGV if eof;
   }


sub field {
   my ($cmd, $num, $fld) = @_;
   $max = $#$fld;
   for ($i = 0; $i <= $max; $i++) {
      if (@$cmd[$num] eq @$fld[$i]) {
         last;
      } elsif (@$cmd[$num] eq "") {
         last;
      } elsif ($i == $max) {
         print "$ARGV line $.: \"@$cmd[$num]\" is not valid for @$cmd[
+0] field $
num.\n";
         last;
      }
   }
}

sub attribs {
   my ($cmd, $book) = @_;
   my $given = $#$cmd;
   if ($given != $book) {
      warn "$ARGV line $.: \"@$cmd[0]\" incorrect number of attributes
+.  There s
hould be $book.\n";
   }
}

BEGIN {

@GRDF1  = qw(G Y);
@GRDF2  = qw(B);
@GRDF3  = @TBLB2;
@GRDF4  = @TBLB3;
@GRDF5  = qw(S U);
@GRDF6  = @MONEY;
}

[download]

Replies are listed 'Best First'.
Re: Code review
by dws (Chancellor) on Sep 18, 2003 at 03:58 UTC
    I'm seeking constructive critisism on some code.

    First, I don't think 

      while (<>) {
      chomp;
      s/ //g;
      tr/a-z/A-Z/;
      } if (/^GRD/) {

    [download]
    is doing what you think it is. Ponder the difference between 
      while ( ... ) { ... }
  if ( ... ) { ... }

    [download]
    and 
      while ( ... ) {
      ...
      if ( ... ) {
          ....
      }
  }

    [download]
    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";
    }

    [download]
    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.

[reply]
[d/l]
[select]
      And what is 
        close ARGV if eof;

      [download]
      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.

      [download]
      Now, it's possible that eof in the previous line is used in a wrong way: 
      close FH if eof;

      [download]
      but since the code fragment omits any other use of FH, it's hard to say.

      Abigail

[reply]
[d/l]
[select]
      Thanks for the input. I'm going to try the routine you suggested. The reason for: 
      @GRD = split /,"?|""?/;

      [download]
      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.
[reply]
[d/l]
Re: Code review
by Plankton (Vicar) on Sep 18, 2003 at 04:47 UTC
    Some # comments couldn't hurt. For example ... 
    # GRD stands for Green Royal Donkeys
@GRD = split /,"?|""?/;
...

    [download]
    Unless GRD stands for something widely used by developers that will be maintaining your code, you should probably come up with a better name. I can only guess at what GRD stands for? Ground maybe. Now if you are doing electronics Ground means something completely different to say a flight control system's meaning of Ground as in you are about to hit the GROUND! :)

    Plankton: 1% Evil, 99% Hot Gas.
[reply]
[d/l]
Re: Code review
by InfiniteSilence (Curate) on Sep 18, 2003 at 21:50 UTC 
                     field (\@GRD, 1, \@GRDF1);
                 field (\@GRD, 2, \@GRDF2);
                 field (\@GRD, 3, \@GRDF3);
                 field (\@GRD, 4, \@GRDF4);
                 field (\@GRD, 5, \@GRDF5);
                 field (\@GRD, 6, \@GRDF6);

    [download]
    When you see lots of calls that look very, very similar it is a sign that the code needs to be rewritten for brevity. If you had changed @GRDF to a list of list references (as in @GRDF = ( [G Y], [B], \@TBLB2 ) \@TBLB3 [S U] \@MONEY )): 
         for (1..6) {
       field (\@GRD,$_,$GRDF[$_]);
     }

    [download]
    You are using slices when you mean to be referring to list elements. You wrote: 
         sub field {
        my ($cmd, $num, $fld) = @_;
        $max = $#$fld;
        for ($i = 0; $i <= $max; $i++) {
           if (@$cmd[$num] eq @$fld[$i]) {

    [download]
    But what you meant was: 
        my ($cmd, $num, $fld_ref) = @_;
    my $found = 0; #false
    if ($cmd->[$num] eq "") {$found = 0} else {
    for (0..$#$fld_ref){
       if ($cmd->[$num] eq $fld->[$_]) {$found = 1};
      } 
     }
    if not ($found) {print "ITEM what-cha-ma-calit is not valid ..."};

    [download]
    Because your function is basically looking to see if the element is in the array. There are better, cleaner ways to write this than the above but you were using array references in the function calls so this should be consistent with your original purpose (I think)...

    Celebrate Intellectual Diversity

[reply]
[d/l]
[select]
Re: Code review
by mhearse (Chaplain) on Sep 18, 2003 at 03:41 UTC
    I should mention that other than GRD, the loop is actually checking for 40 other commands in the same fashion. Thanks.
[reply]

Back to Seekers of Perl Wisdom