A good C programmer can write C in any language, especially C++. A bad C programmer can do the same, and a bad C programmer will do all sorts of terrifying things in the process.

Gaetan works with a terrible C programmer.

Let's say, for example, you wanted to see if an index existed in an array, and return its value- or return a sentinel value. What you definitely shouldn't do is this:

    double Module::GetModuleOutput(int numero) {
        double MAX = 1e+255 ;
        if (this->s.sorties+numero )
            return this->s.sorties[numero];
        else
            return MAX ;
    }

sorties is an array. In C, you may frequently do some pointer arithmetic operations, which is why sorties+numero is a valid operation. If we want to be pedantic, *(my_array+my_index) is the same thing as my_array[my_index]. Which, it's worth noting, both of those operations de-reference an array, which means you better hope that you haven't read off the end of the array.

Which is what I suspect their if statement is trying to check against. They're ensuring that this->s.sorties+numero is not a non-zero/false value. Which, if s.sorties is uninitialized and numero is zero, that check will work. Otherwise, that check is useless and does nothing to ensure you haven't read off the end of the array.

Which, Gaetan confirms. This code works "because in practice, GetModuleOutput is called for numero == 0 first. It never de-references off the end of the array, not because of defensive programming, but because it just never comes up in actual execution.

Regardless, if everything is null, we return 1e+255, which is not a meaningful value, and should be treated like a sentinel for "no real value". None of the calling code does that, however, but also, it turns out not to matter.

This pattern is used everywhere there is arrays, except the handful of places where this pattern is not used.

Then there's this one:

    if(nb_type_intervalle<1)    { }
    else 
        if((tab_intervalle=(double*)malloc(nb_lignes_trouvees*nb_type_intervalle*2 \
                                                        *sizeof(double)))==NULL)
            return(ERREUR_MALLOC);

First, I can't say I love the condition here. It's confusing to have an empty if clause. if (nb_type_intervalle>=1) strikes me as more readable.

But readability is boring. If we're in the else clause, we attempt a malloc. While using malloc in C++ isn't automatically wrong, it probably is. C++ has its own allocation methods that are better at handling things like sizes of datatypes. This code allocates memory for a large pile of doubles, and stores a pointer to that memory in tab_intervalle. We do all this inside of an if statement, so we can then check that the resulting pointer is not NULL; if it is, the malloc failed and we return an error code.

The most frustrating thing about this code is that it works. It's not going to blow up in surprising ways. I never love doing the "assignment and check" all in one statement, but I've seen it enough times that I'd have to admit it's idiomatic- to C style programming. But that bit of code golf coupled with the pointlessly inverted condition that puts our main logic in the else just grates against me.

Again, that pattern of the inverted conditional and the assignment and check is used everywhere in the code.

Gaetan leaves us with the following:

Not a world-class WTF. The code works, but is a pain in the ass to inspect and document

In some ways, that's the worst situation to be in: it's not bad enough to require real work to fix it, but it's bad enough to be frustrating at every turn.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.