Sunday, 16 March 2014

Coding structures that cause bugs

Bugs are usually caused by simple things like typos and oversights. As a programmer, this might make you think you need to become smarter and concentrate better, or maybe you conclude that bugs are simply part of any programmer's life. Both are of course partially true, but there is more to it than that. There are methods that can be used to reduce the chance of creating bugs (even though such methods won't fix all of them).

One of those is better coding structure. Ideally code is structured in such a way that mistakes and oversights are not possible, and that the code is easy to understand. In practice these goals are often not fully achievable, for example because they contradict with the amount of time you have to build it, or the performance requirements, or maybe the problem you are solving is just too complex for easy code. Nevertheless, striving for good structure does make a big difference. Today I would like to give a couple of examples of common coding practices in C++ that can easily cause bugs. Even if you want to keep using them, knowing the pitfalls can help avoid issues.

You might disagree with the examples given here, but I hope they at least help you think about how you structure your code and why. In the end there is no absolute answer to what good code is, but I do believe that to be a good programmer, one needs to think about what good code structure is, and which features of a language to use and which to avoid.

Initialise() functions

Some classes have a separate initialise() function. This means that after having constructed the class, you need to first call the initialise() function before you use it for anything. This is often done because constructors don't have return values (and can thus not return errors in the normal way), or because classes cross-reference each other and they both have to exist before either can be fully initialised.

However, using initialise() functions introduces a new situation that is prone to bugs: it is now possible to have an instance of a class that is not initialised and thus not ready for usage. All kinds of bugs might happen if you forget to call initialise() and then start using the class. This can be fixed by keeping a bool isInitialised in the class and checking for that before usage, but that of course introduces another thing that needs to be remembered and is thus ready to be overlooked by a future programmer working on that class, causing even more bugs.

For this reason it is generally wise to avoid separate initialise() functions altogether. Just create everything in the constructor. This principle is called RAII ("Resource Acquisition Is Initialisation"). If you want to do error checking in the constructor, you can solve the problem of not having a return value by using try/catch.

Default parameters

Default parameters are parameters in functions that have a default value, so that you can skip them if you don't need something special. Like for example:

void eatFruit(string fruitType = "banana");

//Can leave out the fruitType when calling this function
eatFruit();
eatFruit("kiwi");

Default parameters are a useful tool: some functions are often called with the same parameters so it is nice to be able to just skip them in those cases. However, default parameters can also be dangerous because it is easy to forget about them, especially when refactoring. In the development version of Awesomenauts I recently created a crash bug while refactoring a piece of code. Here is what I changed:

//Old code
TemporaryAnimation(ParticleFadeManager* particleFadeManager,
                   RumbleManager* rumbleManager,
                   unsigned int textLocalizationIndex,
                   RenderGroup renderGroup = RG_STANDARD);

//New code after refactoring, with one new parameter
TemporaryAnimation(ParticleFadeManager* particleFadeManager,
                   RumbleManager* rumbleManager,
                   ReplayFrameStorer* replayFrameStorer,
                   unsigned int textLocalizationIndex,
                   RenderGroup renderGroup = RG_STANDARD);

I use the compiler a lot while refactoring, so when I change something like adding a function parameter I often just hit compile to see where it is used. In this case however this was a bad choice because the default parameter caused the compiler to interpret my code in a different but still compilable way. Here is the line that caused the crash:

animation = new TemporaryAnimation(particleFadeManager,
                                   globalRumbleManager,
                                   0,
                                   RG_MENU);

Originally the '0' was the textLocalizationIndex, but after refactoring it was interpreted as a NULL pointer for the replayFrameStorer. The compiler didn't complain because RenderGroup is an enum and can thus be interpreted as an unsigned int. So RG_MENU was now interpreted as the textLocalizationIndex and the renderGroup now used the default parameter RG_STANDARD. Really broken code, no compiler error.

Had I not used default parameters here, the compiler would have given an error because I was passing in one parameter too few.

This is just one example of a situation where default parameters have given me such headaches: I have encountered several other situations where I overlooked things because of default parameters.

Also, I prefer if someone who calls a function actually thinks about what he is passing to it, and default parameters don't encourage thinking: they encourage ignoring. So despite their usefulness, I generally try to avoid default parameters.

Constructor overloading

When a class has several constructors, this is called constructor overloading. In university I was taught this is a useful tool for all kinds of things, but in practice it turns out it is often a headache. It creates situations where it is easy to overlook something and introduce new bugs. Let's have a look at an example of this:

class Kiwi
{
private:
    string colour;
    const float softness;
    const float tastiness;

public:
    Kiwi(const string& colour_):
        colour(colour_),
        softness(1),
        tastiness(1)
    {}

    Kiwi(float softness_):
        colour("green"),
        softness(softness_),
        tastiness(1)
    {}
};

The problem here is that I need to initialise everything in every constructor. The tastiness variable is always set to 1, but I have to set it in each function. This means code duplication. Code duplication can be avoided by using a common function that all constructors call, but this is not always possible. In this specific case for example the tastiness variable is const and can thus only be initialised in the initialiser list, not in a separate function.

Even worse worse than code duplication is that if I add a new variable to the class, I need to remember to initialise it in every constructor. Forgetting about one of the constructors has caused bugs in our code in several situations, so we now try to avoid constructor overloading altogether.

Conclusion

These three examples show how certain structures cause bugs. None of these structures are bad per se and they each have their use, despite the risks. In fact, I use these occasionally myself, simply because sometimes the alternatives are worse. However, knowing the risks of certain structures helps make good decisions on when to use them and when not to use them, and to first consider alternatives whenever you think you need them. It is important as a programmer to think about the pros and cons of anything you do, and to always look for better structure.