Seven Deadly Sins of Programming – #6

Seven Deadly Sins of Programming – #6

Way back in my early career, I worked on a graphics program written in FORTRAN that was used to plot engineering data.

It ran on Tektronix 4014 terminals.

Our application – known as E.G.G. (Engineering Graphics Generator, one of a whole series of not-very-creative punny names used by the group) – used a menu-based approach. The user would place the cursor (moved by the thumbwheels on the right) over a menu option, hit the spacebar, and then the code would figure out what to do.

The internal architecture of the app was really pretty good. There were, if I recall correctly, 39 different screens, and each item on the screen had an id associated with it. The code would find the right item on the page, come up with a numerical code (menu number * 100 + item number), find that in an array of function pointers, and then call the appropriate function.

It worked fine, and allowed us to have a batch mode as well.

But after I started playing around with it a bit, I noticed that the command dispatch was a bit slow. After you selected an item on a screen, it took at least a couple of seconds for the screen to erase and redraw (this was running on shared Vaxen, so you didn’t get much CPU time, and what you got was fairly slow).

I did some digging, and found that the original developer (no longer with the group) had written the item lookup code using a Fibonaccian search.  This is a search that is similar to binary search, but doesn’t require complex operations such as divide by two or shift right (that alone should tell you the vintage of this search).

Such a search *may* have made sense on earlier systems, but on Vaxes, both the divide by two or shift right are in the hardware. So, we were using a search method optimized for constraints that we didn’t have that nobody in the group understood, in an attempt to be faster than binary search.

But it was a *clever* algorithm.

At this time, we had about 1200 items in our dispatch table. The first thing I did was build sub-index that stored the starting index of the first item in each menu, which meant the search space for a typical menu item went from 1200 items to about 30 items. The second thing I did was switch to interpolation search, which, given the nearly perfect distribution of items numbers within a menu (most menus had item numbers from 1-n, with very few holes), gave an almost perfect search. And it was code that most people in the group could understand.

So, the dispatch time went from a few seconds to about half a second, and I wrote up a “cost savings” in which I multiplied the number of users we had by the amount of time they used the application by the number of menu picks per hour, and figured out that I had saved the company around $30K per year. For which I got a $100 gift certificate.

So, what’s the point of that story?

Well, it makes me look good, which is the primary goal. It also illustrates that cleverness is an over-rated attribute of developers. How many times have you seen:

for (int i = 0, j =bounds; i < upper; i++, j += JUMP_SIZE)

That’s somebody who is trying to be clever. They should have written:

int j = bounds;
for (int i = 0; i < upper; i++)
    j += JUMP_SIZE

Which takes us on a long and tortuous trip to

Sin #6 – Inappropriately clever code

What examples of this have you seen?

Comments (22)

  1. We have certain people in our development shop who love to write stuff like this:

    this.Foo = new( this.GetBillerName( custId));

    this.result = this.Foo.Execute(this.BuildRequestMessage(), this.FindFavoriteColor( custId ));

    I’m not sure this is all that clever but they think it is.  Debugging it is a colossal pain since it’s impossible to see a very clear line through the code.

  2. jonskeet says:

    Use of the double-checked locking algorithm is almost always inappropriately clever, IMO. (And this plays a reasonably prominent part of my own seven deadly sins article, which I’m hoping to complete sometime in the next week or so.)

  3. Maurits says:

    The former for loop does have one point in favor of it… it doesn’t mess with j in local scope.

  4. Right'yo says:

    while (*p++ = *s++);

  5. wpoust says:

    I hate to say this, but some of the code I wrote was inappropriately clever.  We had an external player, an integrated player, and the dreaded null player.  All of the players implemented a couple of interfaces.  A nice class factory instantiated the appropriate player.  

    No one asked what the null player did (nothing).  It took 4 different versions of our software before I yanked the null player out and instead used this magical value called NULL when there wasn’t a player.

  6. wpoust says:

    I hate to say this, but some of the code I wrote was inappropriately clever.  We had an external player, an integrated player, and the dreaded null player.  All of the players implemented a couple of interfaces.  A nice class factory instantiated the appropriate player.  

    No one asked what the null player did (nothing).  It took 4 different versions of our software before I yanked the null player out and instead used this magical value called NULL when there wasn’t a player.

  7. Shahar says:

    We used to have a dev in a previous company I worked at (circa 2000) who had his own special way of doing HRESULT-COM error checking..

    We ended up calling it the "ok = ok &&" curse.

    We used COM heavily and his code used to look like this:

    bool ok = true;

    ok = ok && SUCCEEDED(object1->CallMethod(&object2));

    ok = ok && SUCCEEDED(object2->CallMethod2());

    ok = ok && SUCCEEDED(object2->CallOtherMethod());

    The idea being that the first failed call will turn "ok" into false and then the rest of the code wont run.

    This is of course a horrible way of doing things and is a royal pain in the ass to debug.

  8. Tomer Gabel says:

    It’s funny, I keep running into arguments as to the pros and cons of the sort of "codespace-optimized" loops you mentioned. I’m actually very much for them – they’re usually used where the algorithm is very simple (for example, when writing to a frame buffer and adding a stride value to your target pointer) and tend to make the code "look" a lot more elegant.

    Proper indentation, spacing, scoping, loop constructs etc. are vital to a good program. That said, "proper" is an evidently subjective term.

  9. Peter Ritchie says:

    RE: while (*p++ = *s++);

    I hate pointer value access/increment shorthand!

    I find religiously following const-correctness often leads to unnecessarily cleaver code:  for example:

    void Method(const MyEnumOne o, const MyEnumTwo t)


     const int someValue = (o == MEO_SOMEVALUE ? 1 : 0) | (t == MET_SOMEVALUE ? 2 : 0);


  10. What about

    Dim x as Integer

    x = 6

    … a bunch o lines of code which do not deal with the x variable and then,

    If x = 3 Then

       … some confusing code which will never be run, ’cause x=6

    End If

  11. Peter Ritchie says:

    I don’t know if I call that Inappropriately clever (or clever at all); but, it reminds me of some code that I encountered once:



    // .. dozens of lines of code


    When I approached the author about it, he said he did it on purpose so he could keep track of the changed code.  There was no comment, no #ifdef, and this code was checked into source control.

  12. BobHy says:

    "Inappropriate" is relative to time (as your first story illustrates) and point-of-view ("cui bono").  Would you agree that cleverness which benefits the user who runs the code (efficiency) or the team who maintains it (maintainability) is appropriate?  And if the clever bits trade off one of these against the other, who gets to decide the direction?  

    OTOH, even when we consider cleverness whose only benefit is to amuse its author, it’s fair to ask whether the author’s happiness is itself a benefit to the user community ("a happy programmer is a cooperative, bug-fixing programmer") or to the team ("a happy programmer remains on the team and trains his/her successor").  

    I’m beginning to think "inappropriate cleverness" may be a vacuous term. But there certainly is "incorrect" cleverness, where the claimed benefit of the design is refutable.

  13. Reinier Post says:

    I do occasionally use

    if (true)




    just to provide local scope to an auxiliary variable that isn’t used in the rest of the body.

    (This means of course that the body is too long, but I don’t always get the time to rewrite such things.)

  14. jonskeet says:

    You don’t need

    if (true)




    to introduce an extra level of scope. Just add braces:

    Console.WriteLine ("Outer");


       Console.WriteLine ("Inner");


  15. jrista says:

    I must sadly admit that I am guilty of this one (on a not so often, but regular still, basis):

    StringReader sr = new StringReader(someString);

    string line;

    while ((line = sr.ReadLine()) != null) // Inappropriate macho cleverness here 😛


       // Code…


    However, the most common alternate to this that I see people use is:

    string line;



     line = sr.ReadLine();

     if (line == null)


     // Code…


    Which goes back to the inappropriate cleverness of having a while(true) in your code. Few people try to do things the way that is probably the most understandable and correct (practically speaking):

    string line = sr.ReadLine();

    while (line != null)


     // Code…

     line = sr.ReadLine();


    Despite being an offender of the first example myself, its one of my pet peeves. Go figure. 😛

  16. while (*p++ = *s++);

    Ah, what a beauty that is!

    And the truly scary thing for me is that it’s so ingrained into the idiomatic knowledge of every old-time C developer that I knew immediately that it’s a (simplistic) strcpy function.

    In the mid-80s I worked with a young programmer (we’ll call him F). F was a gifted mathematician, descended from a long line of eccentric but brilliant mathematicians. We were writing programs to calculate the yields on bonds. We would start with Newton-Raphson algorithms that converged to 6 decimal places in 5+ iterations, and after some massaging by F they would magically converge in 2 or 3 iterations. Amazing.

    Somewhere along the way, F discovered that _ (a single underscore) was a perfectly legal variable name in C. And so, of course, is __ (two underscore characters). Etcetera. F would write code that had lines like:


    Far and away the most maddening code I have ever tried to read.

  17. lesbian rape says:

    Best of all people w can talk…

  18. So, the time has come for the worst sin.

    Just to recap – and so there is one post that lists them all…