Ugly Code

Unfortunately, I’m a sort of a purist when it comes to coding. Code that is not properly indented, global and local scopes garbled up, obscure naming, counter-intuitive interfaces… all conjure against my ability to read a source and causes a headache, acid stomach, and buboes. “Unfortunately,” I wrote, meaning that’s most unfortunate for the poor souls that have to work with me to whom I should appear as sort of source code Taliban.

Recently my unholy-code-alarm triggered when a colleague – trying unsuccessfully to compile an application produced by a contractor – asked me for advice.

More and more I delved into the code, more and more my programmer survival instinct screamed. The code was supposedly C++ and the problem was related to a class, that I would call – to save the privacy and dignity (?) of the unknown author – SepticTank. This class interface was defined inside a .cpp and then again in a .h. Many methods were inlined by implementing them in the class interface (and this possibly was part of the problem).

After resolving some differences, the code refused to link because there was a third implementation of the SepticTank destructor in a linked library. I claimed that such code couldn’t possibly work (even after disabling the dreaded precompiled headers – never seen a Visual Studio project working fine with precompiled headers), even if we could manage to get it compiled and linked the mess was so widespread that nothing good could come.

My colleague tried to save the day by removing the implementation of the SepticTank destructor so as to leave the implementation found in the linked library.

In the end, he had to give up because the code was broken beyond repair, and even if it compiled and linked it crashes on launch (not really surprising).

What stroke me most, basically because it caused a slight fit of dizziness, was the sight of the mysterious operator below –

class SepticTank
{
    public:
        // code removed for your comfort
        operator SepticTank* ()
        {
            return this;
        }
        // other code removed, again for your comfort
};

My brain had some hard moments trying to decode signals from my eyes. Then it figured out that the coder was redefining the implicit conversion to the class pointer so as to use instances and references where pointers were expected… why on the Earth one should want something like that?!?

Implicit conversions if not handled correctly are a sure way to kick yourself on the nose and this is enough a good reason to stay away. But… trying to enter the (criminal) mind that wrote that code, what’s the purpose? Just to avoid the need for the extra ‘&’ ? Or is it a Javism? Maybe it is better to stay out of such minds…