Xref: utzoo comp.databases:1725 comp.lang.c:14805 comp.unix.xenix:4189
Path: utzoo!utgpu!attcan!uunet!lll-winken!lll-tis!ames!xanth!mcnc!rti!xyzzy!throopw
From: throopw@xyzzy.UUCP (Wayne A. Throop)
Newsgroups: comp.databases,comp.lang.c,comp.unix.xenix
Subject: Style (was: C-DATABASE B-PLUS a quick look)
Summary: picky, picky, picky
Message-ID: <2404@xyzzy.UUCP>
Date: 15 Dec 88 21:28:47 GMT
References: <179@lakesys.UUCP>
Organization: Data General, RTP NC.
Lines: 79

In the code fragment posted in this line of conversation, there was
one "style fault" that could actually qualify as a bug under draft
X3J11, and which therefore I felt ought to be pointed out.

Now when I got done with the posting, I found I had picked on four
more points of style in the code fragment, and my posting had ended up
looking like a hatchet job rather than a warning of a nonportable
practice.  Well, rather than pare down my nits to the most important
one, I'm posting the whole critique.  I hope folks won't take this as
the hatchet job it superficially resembles, but will take in the way
it was intended: an important warning followed by four stylistic
afterthoughts that I really and truely think result in code of
"superior style".  Happy coding, and if you feel I've been too harsh
despite the disclaimer, feel free to email me and vent your spleen.

The fragment in question is:

    void strupr( s )
    char s[];
    {
            int p = 0;
            while ( s[p] != NULL ) {
               s[p] = toupper( s[p] );
               p++;
            }
    } /* strupr */

My comments, in decreasing order of urgency (or increasing order of
pickiness) are these.

First, comparing a character to NULL is stylistically wrong in K&R C,
since NULL is to be used in pointer contexts (just as FALSE or NO or
whatnot is to be used in "boolean" contexts).  In draft X3J11 C, this
is even a potential error, since NULL can legitimately be defined as
((void *)0), in which case an implementation (I think) is allowed to
diagnose an error in the above comparison.

Second, the loop control is scattered over the body of a while, which is
exactly what the for loop was invented for.

Third, the function might as well return s instead of void, so that
it can be slipped unobtrusively into expressions instead of forcing
separate statements or awkward expressions.

Fourth, it isn't indicated that ctype.h ought to be included.
(This is, of course, fairly obvious, and may have been omitted on
 purpose...)

Fifth, the object of this excersize is somewhat more naturally cast in
terms of pointers rather than subscripting in C.  Two marginal
justifications for this are that first it is more natural to step
along an array once by pointers, and second compilers with few or no
optimizing smarts will generate better code for the pointer case on
most (but not all) architectures. (This, of course, is a really picky,
obnoxiously finicky nit.)

Applying all of these, we get the "improved" fragment (using prototypes):

    #include <ctype.h>
    char *strupr( char *s ) {
        register char *p;
        for( p=s; *p != '\0'; p+=1 )
            *p = toupper( *p );
        return( s );
    }

Note that some might prefer the condition in the for loop to be

        *p != 0
or just
        *p

(In fact, I prefer just *p myself, but most folk seem to prefer the
 explicit comparison to a character value.)

--
"Leave him alone.  He is already neutralized."
"I don't want neutral.  I want dead."
                                        --- exchange in "Die Hard"
-- 
Wayne Throop      <the-known-world>!mcnc!rti!xyzzy!throopw