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;
#3 I : INT;
IN_QUOTE : BOOLEAN;
J,K : INT;
T : CHAR80;
I := 1;
WHILE (I<80) AND (C[I] = BLANK) DO
I := I + 1;
IN_QUOTE := FALSE;
#4 FOR J := I TO 80 DO
#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];
#9 W := 80;
#10 WHILE (W > 0) AND (T[W]=BLANK) DO
#11 W := W-1;
#12 trim := T;
- 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.
- No comment describing the function's goal.
(Not a bug, but a potential problem.)
- 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.
- 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.
- 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.)
- 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
- 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.)
- What happened to IN_QUOTE? We calculated it
and never used it!
- 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.)
- 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".
- 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
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
- 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
 and T . 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
(Click here for the "How To Code Pascal" paper.)