Re: How to name variables in a program?

Arthur J. O'Dwyer wrote:
> On Mon, 30 May 2005 websnarf@xxxxxxxxx wrote:
> >>> http://msdn.microsoft.com/library/default.asp?
> >>> url=/library/en-us/dnvs600/html/hunganotat.asp
> >
> > /* A name like "sy" is extremely unfortunate. But I have no idea what
> > it
> > was supposed to be (and still don't) so I guess I will just leave
> > it. */
>
> Rule #1 of recoding for readability, on Usenet: Make sure your lines
> are under 75 columns. ;) One of these days I should sit down with
> http://www.contrib.andrew.cmu.edu/~ajo/free-software/usenetify2.c
> and explain to it how to reformat icky-wrapped block comments...

Yeah, yeah, yeah. I in fact was using a tool that helps me do that, I
just was too lazy to do it. I already expended too much energy trying
to untangle that ridiculous mess of code.

> > extern int syEndOfs;
>
> FYI, I still have no idea what this identifier is supposed to mean.
> "EndOfs" presumably has something to do with an offset of the end of
> something (the hash table, IIRC) --- but the abbr is obf. Once you've
> gone over six characters, there's not even a historical reason to
> use such extreme name-compression.

Yeah, it was a first pass translation, and I didn't bother to go back
and figure out a good name for it. Its not used in context of the
code, so I didn't know how to name it. In general I always try to
minimize global variable. When I see a global variable, my first
thought it always "should this be read only (const)? And if not, why
isn't this hidden behind a thread safe accessor function?"

So when I see that global variable, with no context, I mostly just
throw my hands up and, if anything, want to just remove it.

> > /* [...] The original declaration of () for the
> > parameters should really be outlawed. */
> >
> > struct SY * PsyCreate (int);
>
> Wow --- good catch!

Except I should probably drop the "P" (redundant with the "*" that
immediately preceeds it.) I'm also still not really satisfied with it,
because there is neither a "static" nor an "extern". Prototypes should
really always be one or the other.

> > /* Random unnamed constants should not appear in your code. Define
> > and name them. The only reason not to do this is if the constant
> > necessarily only appears once, and is accompanied by a comprehensive
> > inline comment.
> > */
> >
> > #define BOTTOM_15_BITS (077777)
>
> Here I disagree. My rule would be, "Random unnamed constants should not
> appear in your code; define and name them. The only reason not to do this
> is if the constant(s) only appears in one section of the code, and its
> meaning is clear and unchanging. For example, the constants 0 and 1, and
> bitmasks of the lowest K bits."

Ok, constants like 0, 1, 2, -1, are fine not to comment. But low K
bits, should be commented as something like "low K bits", since C
doesn't support binary source formats, and the mental translation
between HEX (or Octal) and binary is still effort that is truly
unnecessary. Usually, the name is also something more relevant like
INDEX_MASK or something.

I should also point out that doing this masking is just a plain useless
redundancy for a hash function anyways. It really should just be
dropped directly if there is an outer modulo anyways. (Of course it
shouldn't be using modulo either, but that's another discussion.)

> [...] An auxiliary rule is "Don't use octal."

Yeah. The thing is, in transforming this code, I was trying to
understand what the hell it was doing. (That's the only way I could
figure out that this was some kind of open hash insert function.)
During the process I didn't want to throw away any hints as to what it
was doing, so I left the octal as octal. Of course, the octal is
irrelevant, and a throwback to 1950s.

> I find it impossible to read by sight, and thus obfuscatory. This may be
> a function of my generation. ;) Still, the idea of taking bits in groups
> of /three/ seems like a bad idea to me.

Yeah, tell that to the Unix people who wrote chmod.

> > /* [...] Just const char * will be sufficient for that, and the name
> > "key" to help us understand that this is a hash table, or other ADT,
> > entry key. */
>
> I picked 'text', since I'm not sure whether it's most correct to call
> that string a "key." We're really creating the hash key from the string
> by hashing it with that '<< 11 + *s++' function. (BTW, good catch there,
> too! Changing '<>' to '<<' didn't occur to me at all.)

That was one of the last things I caught. I had to fix the entire rest
of the code before I realized that this was some sort of hash table
thing. Then I just took a guess at what they were trying to do that.
What I cannot believe is that that was cut and paste from anywhere.
And who ever makes a typo of making a << into <>? It could only come
from the foreign brainspace that put them into using hungarian notation
in the first place.

> [...] Also, I notice
> you made the struct member's name 'entry'; whenever there's a situation
> in which we copy some parameter into a struct member, I like to keep
> the names the same: 'p->text = text', for example.

> > /* Search for a match, or find the tail of the current chain */
> > for (;*currEntry;currEntry = &currSy->next) {
> > char *szSy = (currSy = (struct SY *) &dict[*currEntry])->entry;
>
> Icky! You really like the 'p = (q = r)' idiom?

Yes because its non-ambiguous. I certainly like it a lot better than
*p++ or *(++p) since I know there are strange circumstances where the
operation order isn't what you think it is.

> [...] My very first step
> in prettifying, even more important than fixing indentation, is
> pulling out all the nested assignments so they appear in chronological
> order in the source file.

Code maintainability is not about aesthetics. Clarity and being
explicit are of the highest concern. After that, optimizing screen
space usage by packing
things on one line using common idioms like this actually ends up
having a net payoff. Its really just a generalization of the common:

j = k = 0;

Other common cases include:

if (NULL == (p = (char *) malloc (...))) err ("Out of memory");

The point being that dealing with NULL returns from malloc is a
pervasive thing that you have to do all the time. Expanding it out to
the most wordy vertical screen space chewing version provides no value.
You already know that malloc is going to wrapped with this sort of
test anyway.

Saving the screenspace and learning the idiom is, IMHO, simply a net
gain in real maintainability.

> > if (0 == strcmp (key, szSy)) return currSy;
>
> Another good catch. I wasn't sure the code was doing the same thing,
> but I see now that it is. For that matter, 'bltbyte' could be replaced
> with 'strcpy', or with 'memcpy' if we're concerned about microefficiency.

I had originally changed it to memcpy() (and I changed Zero() to
memset(*,0,*)), but in the end decided I couldn't conclusively
determine that that is what bltbyte() was doing. But obviously those
transformations should be there, if correct, for the exact same reason
I used strcmp().

You could argue that the inline version is faster, but this code cannot
make a credible claim to trying to optimize performance. Besides many
modern compilers will optionally automatically inline many common
functions such as these anyway.

> > The casting back and forth between struct SY * and int * is indicative
> > that there is something really wrong with the core design.
>
> (I agree.)

In some cases its ok to do such thing, but the code itself has to make
all the sematics extremely clear as to what tricks its pulling, so that
the code remains maintainable. That simply isn't the case with this
code (even after cleaning it up).

> > Some of the other names like or "cwSy" etc, I am just too lazy to
> > decode what the hell they are trying to say with such names.
>
> The comments at the bottom of the page explain that 'cwSy' is the
> same as 'sizeof (struct SY)', I think; either that or the same number
> in Windows dwords instead of bytes. One big problem with this function's
> use of Hungarian is its vast overuse of the 'w' prefix, which Simonyi
> uses to mean "word with arbitrary contents" (i.e., anything at all).
> 'wXyz' seems to be the Hungarian equivalent of 'foo, bar, baz'. Only
> it's more dangerous, because it /looks/ organized! :)

Reading through the rules for hungarian notation really lets you see
into the minds of these fools and what they consider to be important in
program source code maintenance. This obesssion with offsets, arrays,
pointers, etc. Like obsessing with epsilon beyond the C programming
languages basic semantics is where the entire focus should be. Naming
is meant for abstraction, and this should match the level of
abstraction that highest possible level of design is trying to
describe.

Only C++ could save them, and I think its unfortunate that it did.

--
Paul Hsieh
http://www.pobox.com/~qed/
http://bstring.sf.net/

.

Relevant Pages

  • Re: indentation
    ... yeah, it is a matter of taste I guess. ... braces are indended the same as the opening braces. ... this is a common practice though, and also I don't personally know of any OS ... the parens are a matter of style and tradition... ...
    (comp.lang.c)
  • Re: indentation
    ... yeah, it is a matter of taste I guess. ... braces are indended the same as the opening braces. ... this is a common practice though, and also I don't personally know of any OS ... the parens are a matter of style and tradition... ...
    (comp.lang.c)
  • Re: Kernel SCM saga..
    ... > DL> just wanted to point out that recent news shows that sha1 isn't as ... It's possible to generate another object with the same hash, ... > yeah, yeah, I know about the current theoretical case, and I don't ... send the line "unsubscribe linux-kernel" in ...
    (Linux-Kernel)
  • Re: New Paper: More than 600 million users surf at high risk
    ... Hash: SHA1 ... Jeff Martin wrote: ... Yeah, that was what I tried to get at earlier. ...
    (Bugtraq)
  • Re: Settling an Argument - Assembly *IS* a Language, Right?
    ... From a grammatological standpoint this usage ... but it is fairly common in computer circles]. ... Yeah. ...
    (sci.lang)