How not to code

This little gem came up in conversation last night, and it was suggested that it would make a rather amusing blog entry. A Solaris project had a command line utility with the following, unspeakably horrid, piece of code:

/*
* Use the dynamic linker to look up the function we should
* call.
*/
(void) snprintf(func_name, sizeof (func_name), "do_%s", cmd);
func_ptr = (int (*)(int, char **))
dlsym(RTLD_DEFAULT, func_name);
if (func_ptr == NULL) {
fprintf(stderr, "Unrecognized command %s", cmd);
usage();
}
return ((*func_ptr)(argc, argv));

So when you type “a.out foo”, the command would sprintf into a buffer to make “do_foo”, and rely on the dynamic linker to find the appropriate function to call. Before I get a stream of comments decrying the idiocy of Solaris programmers: the code will never reach the Solaris codebase, and the responsible party no longer works at Sun. The participants at the dinner table were equally disgusted that this piece of code came out of our organization. Suffice to say that this is much better served by a table:

for (i = 0; i < sizeof (func_table) / sizeof (func_table[0]); i++) {
if (strcmp(func_table[i].name, cmd) == 0)
return (func_table[i].func(argc, argv));
}
fprintf(stderr, "Unrecognized command %s", cmd);
usage();

I still can’t imagine the original motivation for this code. It is more code, harder to understand, and likely slower (depending on the number of commands and how much you trust the dynamic linker’s hash table). We continually preach software observability and transparency – but I never thought I’d see obfuscation of this magnitude within a 500 line command line utility. This prevents us from even searching for callers of do_foo() using cscope.

This serves as a good reminder that the most clever way of doing something is usually not the right answer. Unless you have a really good reason (such as performance), being overly clever will only make your code more difficult to maintain and more prone to error.

Update – Since some people seem a little confused, I thoght I’d elaborate two points. First off, there is no loadable library. This is a library linked directly to the application. There is no need to asynchronously update the commands. Second, the proposed function table does not have to live separately from the code. It would be quite simple to put the function table in the same file with the function definitions, which would improve maintainability and understability by an order of magnitude.