Little shop of coding horrors

[Crivens]

Let us learn from other mistakes and give some sympathy to the unsung heroes of code hygiene, the reviewers. Some things you encounter there are worthy of hades.

So let me start with a gem from a USB stack:

If (base->next->next->next->next->next->next)
  free(base->next->next->next->next->next->next);

If (base->next->next->next->next->next)
  free(base->next->next->next->next->next);

If (base->next->next->next->next)
  free(base->next->next->next->next);

...
free(base);

 

[ralphbsz]

The worst I've ever seen I don't have the source code for any longer; it is proprietary to an employer from 15 years ago.

It was a C++ function that had 5 parameters, and the last one was defaulted:

    int foo(int a, int b, int c, int d, int e=0);

Unfortunately, the way it was used, every argument was a complicated function call, and even some of the parameter's parameters were themselves function evaluations, and some expressions throw in, with extra parentheses added for "clarity". This is what happens if a very junior programmer has taken a class in object-oriented design, and thinks that it is the answer to all problems. The whole line ended up being about 200 or 300 characters long, with no line break:

  results = foo(long_function_one(parameter_1_a, parameter_1_b), long_function_two(long_function_two_x(parameter_2_a), parameter_2_b), (long_function_three_a(parameter_3) + long_function_three_b(parameter_3_other), long_function_four(parameter_four_a, parameter_four_b)), (long_function_five(parameter_5) + long_function_five_and_a_half(parameter_5_b/parameter_5_c))

If you look at this mess, it does very much look like the function foo is being called with 5 parameters, right? WRONG! We discovered after a while of debugging (this was industrial control, so debugging had to be done one multi-million dollar equipment) that we were always using the default value of the last parameter to foo, yet all 7 functions that created the 5 arguments were being called. The bug is very subtly hidden, and you need to break down this code and indent it correctly in order to understand the problem:

  results = foo( // First parameter comes from function one:
                 long_function_one(parameter_1_a, parameter_1_b),
                 // Second parameter comes from function two, which in turn has to call sub function two X:
                 long_function_two(long_function_two_x(parameter_2_a), parameter_2_b),
                 // Third parameter is the sum of functions three A and B:
                 ( long_function_three_a(parameter_3) +
                   long_function_three_b(parameter_3_other),
                 // The first half of the bug is here: We forgot to close the opening paren
                 // Fourth parameter comes from function four:
                 long_function_four(parameter_four_a, parameter_four_b)),
                 // Second half of the bug is here: we closed an extra paren
                 // Fifth argument is the sum of function 5 and 5.5 (with ratio of parameters)
                 ( long_function_five(parameter_5) +
                   long_function_five_and_a_half(parameter_5_b/parameter_5_c))

Is the bug clear now? Instead of calling foo with 5 parameters foo(a, b, c, d, e), we called it with 4 as foo(a, b,(c,d), e), causing the last parameter "e" to be always defaulted to zero. And the third parameter is wrongly a "comma expression", namely "(c,d)", which is unfortunately a valid expression in C and C++, which means evaluate the expression c, throw the result away, evaluate d and keep the result.

One could say that all of the following things contributed to the problem:

• The comma operator in C and C++ is dangerous and should have never been allowed: it allows to silently throw away an expression. That's true, but Kernaghan and Ritchie needed it as a hack to allow for loops that simultaneously modify multiple indices: "for (i=0, p=&a; i<10 && *p!=0; i++, p++)" which allows a quick implementation of strncpy. Lesson from this: never allow cute hacks into a commonly used language.
• Default arguments are very dangerous. Definitely true, because they make the code harder to read. They should really be restricted to simple cases, like a single optional argument, because foo() looks different enough from foo(x). In our case, it was actually even worse: The valid values of the last arguments were 0...3; the default was intended to mean "unknown", but the code by mistake used a valid in-band value as the default. A cleaner solution would have been to use an out-of-band value (like -1), or to make two different functions so the debug stack traces show exactly what was being called.
• There is a reason that coding conventions have a maximum line length: code that is this complex is unreadable on a single line.
• But even in the second version above, the bug is not easy to see, the whole problem is one closing paren in the wrong place.
• Adding extra unneeded parentheses around expressions doesn't help, and can create bugs.

Here would have been a cleaner way to write the same code:

// Calculate the 5 parameters for the function call:
int a = long_function_one(parameter_1_a, parameter_1_b);
int b = long_function_two(long_function_two_x(parameter_2_a), parameter_2_b);
int c = long_function_three_a(parameter_3) +
        long_function_three_b(parameter_3_other);
int d = long_function_four(parameter_four_a, parameter_four_b);
int e = long_function_five(parameter_5) +
        long_function_five_and_a_half(parameter_5_b/parameter_5_c);
result = foo(a, b, c, d, e);

Since this was an innocent mistake, the programmer responsible for this mess was not fired. But since within the next year it became clear that he was immune to learning common sense and good coding style, we managed to encourage him to transfer to another place. Interestingly, he was the cubicle neighbor of another programmer who did get fired on the spot (for twice checking known broken code into the source control system, after getting a stern warning the first time).

 

[Bobi B.]

ralphbsz, nice example of how code should not be written

Here is a pseudocode of what you can find in a proprietary SDK of a large set-top box chipset vendor:

type1 res1;
type2 res2;
type3 res3;
type4 res4;
// ...

res1 = allocate_res1(...);
if (NULL == res1) {
    // ... other cleanup
    return failure;
}

res2 = allocate_res2(...);
if (NULL == res2) {
    // ... other cleanup
    free_res1(res1);
    return failure;
}

res3 = allocate_res3(...);
if (NULL == res3) {
    // ... other cleanup
    free_res2(res2);
    free_res1(res1);
    return failure;
}

res4 = allocate_res4(...);
if (NULL == res4) {
    // ... other cleanup
    free_res3(res3);
    free_res2(res2);
    free_res1(res1);
    return failure;
}

// ... do this and that...

... you got the idea. Source code for most SDK functions were, on average, between several hundred and few thousand lines of code, written like this. Allocation of tens of resources in sequence were not uncommon.

Another sample:

int libprefix_read(struct xxx *ctx, void *buf, int size)
{
    switch (ctx->mode) {
        case mode1: return fread(buf, 1, size, ctx->fp); break;
        case mode2: return read(ctx->fd, buf, size); break;
        case mode3: return extlibfoo_read(ctx->foo_ptr, buf, size); break;
        case mode4: return extlibbar_read(ctx->bar_ptr, buf, size); break;
        // ...
    }
}

int libprefix_write(struct xxx *ctx, const void *buf, int size)
{
    switch (ctx->mode) {
        case mode1: return fwrite(buf, 1, size, ctx->fp); break;
        case mode2: return write(ctx->fd, buf, size); break;
        case mode3: return extlibfoo_write(ctx->foo_ptr, buf, size); break;
        case mode4: return extlibbar_write(ctx->bar_ptr, buf, size); break;
        // ...
    }
}

// ... snip

Can you recognise the pattern?

 

[Crivens]

... pasta?

Here is something from the device driver handling code from an operating system many of us use, few know of and no one sees:

if (0 < req->len <= dev->max) {
  dev->operation(req);
}

Head, meet desk. You already know each other?

 

[ralphbsz]

People who write that kind of spaghetti should be indented themselves. With knives. This kind of repetitive and pointless junk may work for a little while, but it is so error-prone and hard to maintain, it needs to be prevented. If code like that goes into production, it really is a symptom of bad management and people who can code but not think.

I love Crivens' double comparison statement. "It worked fine in my in my math book, and the compiler didn't complain". Ha ha.