Bad Hungarian

One more word about Hungarian Notation and then I’ll let it drop, honest. (Maybe.)

If you’re going to uglify your code with Hungarian Notation, please, at least do it right. The whole point is to make the code easier to read and reason about, and that means ensuring that the invariants expressed by the Hungarian prefixes and suffixes are actually invariant.

Here’s some code I found once in the “diagram save” code of a Visio-like tool:

long lCountOfConnectors = srpConnectors->Count();
while( --lCountOfConnectors)
{
// [Omitted: get the next connector]
// [Omitted: save the connector to a stream]
}

OK, first of all, that should be cConnectors, c for “count of”. But that’s just a trivial question of what lexical convention we use. There’s a far more serious problem here. The number of connectors is not decreasing as we iterate the loop, so cConnectors should not be decreasing.

Hungarian makes it easier to reason about code, but only when you make sure that the algorithm semantics and the Hungarian semantics match. Seriously, when I first read this code I naturally assumed that it was removing connectors from the collection for some reason, and therefore decreasing the count so that the count variable would continue to match reality. But in fact it was just using the count as an index, which is semantically wrong. The code should read something like:

long cConnectors = srpConnectors->Count();
for(long iConnector = 0 ; iConnector < cConnectors ; ++iConnector)
{
// [...]
}

In other words, the name of a variable should reflect its meaning throughout its lifetime, not merely its initialization.

12 thoughts on “Bad Hungarian

  1. Why not just name the original variable `iConnector`?

    long iConnector = srpConnectors->Count();
    while( --iConnector)
    {
    // [Omitted: get the next connector]
    // [Omitted: save the connector to a stream]
    }
    
    • Because ideally we want the semantics of an index to be that its value is always a valid index; if you initialize it with the count, that invariant is violated. Remember, the whole point of this naming convention is that the semantics of the value are accurately captured by the name. It should always be legal to use iSomething as an index into a rgSomething.

      • I wish there was some good way to express this with Hungarian, especially since this both simplifies (although this is somewhat subjective) and optimizes the code…

    • Since the variable has been initialized with the count, if it’s used as an index, then you need to decrement it before you use it.

  2. “Why not just name the original variable `iConnector`?”

    Because a count is not an index. The whole point in this article is that variable names should reflect the semantics of their values.

  3. Pingback: Porting old posts, part 1 | Fabulous adventures in coding

Leave a Reply to ericlippert Cancel reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s