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
Anonymous
May 31, 2006
The comment has been removedAnonymous
May 31, 2006
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.)Anonymous
May 31, 2006
The former for loop does have one point in favor of it... it doesn't mess with j in local scope.Anonymous
May 31, 2006
while (*p++ = *s++);Anonymous
May 31, 2006
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.Anonymous
May 31, 2006
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.Anonymous
May 31, 2006
The comment has been removedAnonymous
May 31, 2006
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.Anonymous
June 01, 2006
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);
}Anonymous
June 01, 2006
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 IfAnonymous
June 01, 2006
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:
if(false)
{
// .. 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.Anonymous
June 02, 2006
"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.Anonymous
June 06, 2006
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.)Anonymous
June 09, 2006
You don't need
if (true)
{
...
}
to introduce an extra level of scope. Just add braces:
Console.WriteLine ("Outer");
{
Console.WriteLine ("Inner");
}Anonymous
June 14, 2006
The comment has been removedAnonymous
June 20, 2006
The comment has been removedAnonymous
June 23, 2006
Best of all people w can talk...Anonymous
July 24, 2006
PingBack from http://technote.thedeveloperside.com/?p=56Anonymous
August 04, 2006
So, the time has come for the worst sin.
Just to recap - and so there is one post that lists them all...Anonymous
August 05, 2006
PingBack from http://www.magerquark.de/blog/archive/374Anonymous
August 07, 2006
PingBack from http://pyre.third-bit.com/blog/archives/598.htmlAnonymous
October 10, 2006
PingBack from http://bagofbeans.tsangal.org/archives/203Anonymous
June 13, 2009
PingBack from http://ebeanbagchair.info/story.php?id=203