I've labeled the lines that I'm commenting on with "#" in the left column.

   CONST BLANK = ' ';
#1 TYPE CHAR80 = PACKED ARRAY [1..80] OF CHAR;

   FUNCTION trim (C : CHAR80) : CHAR80;
#2
   VAR
#3   I        : INT;
     IN_QUOTE : BOOLEAN;
     J,K      : INT;
     T        : CHAR80;

   BEGIN
     I := 1;
     WHILE (I<80) AND (C[I] = BLANK) DO
       I := I + 1;
     IN_QUOTE := FALSE;
#4   FOR J := I TO 80 DO
       BEGIN
#5       IF (C[J] = #39) THEN
#6          IF (J=1) OR ((C[J+1]=BLANK) AND (C[J+2]=BLANK)) THEN
#7          IN_QUOTE := NOT IN_QUOTE;
         T[J-(I-1)] := C[J];
#8     END;
#9   W := 80;
#10  WHILE (W > 0) AND (T[W]=BLANK) DO
#11    W := W-1;
#12  trim  := T;
   END;

1. CHAR80? A near-standard in the Pascal community, and implied by some manuals, would be to call the type PAC80. PAC tells the Pascal programmer: PACKED ARRAY [1..something] OF CHAR.

2. No comment describing the function's goal. (Not a bug, but a potential problem.)

3. INT? What's that? And, why should a simple integer type be redefined? Is INT big enough to hold the value 80? We don't know. If INTEGER had been used, the reader would be confident that we could hold at least -32768..32767. If INT16 or If INT32 had been used, the reader would still be comfortable. A potential bug.

4. What happens if the entire C array is blank? In this case, probably due more to luck than to foresight, the entire FOR loop will be skipped (because I will be 81). It would be nicer to have either a comment explaining that, or an explicit check to skip the FOR loop for that case.

5. What's a #39 character? (Should be '''' or SINGLE_QUOTE or DOUBLE_QUOTE, as appropriate.) A potential bug (e.g., did the programmer mean single quote or double quote? If the latter, then 39 is a bug.)

6. If the first 78 characters were blank, then I is 79 here. This means that J is at least 79. This means that J+2 is 81, which is indexing off the end of C (which is only 80 bytes long). If range checking is enabled (as it should be), the program will abort.

7. It's only a quote if it's in column one or if two blanks follow it? That's probably a bug. Now, if there was a comment stating that that was the desired goal, it wouldn't be a bug. (Note, too, that the assignment should have been indented to clearly indicate it was controlled by the IF.)

8. What happened to IN_QUOTE? We calculated it and never used it!

9. It would be much nicer to see: W := SIZEOF (T), instead of the constant 80. This isn't currently a bug, but is a potential future one (i.e., if the type of the parameter is changed.)

10. Wait...where is W declared? Apparently, it's in the outer block (or, if TRIM is nested within another procedure, then within the parent procedure). This can cause bugs when the caller of TRIM doesn't realize that W may change value "behind their back".

11. So, we've calculated the "actual" (trailing-blank-trimmed) length of T. And now, what *do* we do with that W variable? Oops...forgot to use it again! The entire W loop is useless work.

    Actually, when we go to the source to see how TRIM is used, we find:

          writeln (trim (buf):W, ' is the answer');
    

    Of course, this wasn't even *hinted* at in the source for TRIM!

12. Hmmm...T is a CHAR80, so it's 80 bytes long. But, if we had any leading blanks in the original data, then we failed to initialize all the bytes of T! E.g., if the there were 2 leading blanks, then I is 3 after the initial WHILE statement. This means that characters 3..80 are copied from C [3..80] into T [1..78], so nothing ever stored into T [79] and T [80]. This means that the caller will receive junk data in the tail end of T when C had leading blanks.

Click here for a cleaned up version of this function.

(Click here to go back to the uncommented bad version.)

(Click here for the "How To Code Pascal" paper.)

---

(Updated 2000-05-04)