Showing posts with label coding style. Show all posts
Showing posts with label coding style. Show all posts

Monday, 10 July 2017

The Ronimo coding style guide

Last week I showed the coding methodology that we use at Ronimo, which describes our workflow. This week we'll have a look at what our actual code looks like, which is defined in our coding style guide. The idea behind our style guide is that if all code is formatted in a similar manner then it's easier to read and edit each other's code. For example, reading a different bracing style or naming convention usually takes some getting used to, and we avoid that altogether by having a strict style guide that all programmers at Ronimo must follow.



I haven't seen a whole lot of style guides from other companies, but from what I've heard our style guide is quite a lot stricter than what's common elsewhere. I'm not sure whether that's true, but I can totally imagine it is since I'm known for being precise (sometimes maybe overly much). Our style guide isn't set in stone though: there's an exception to every rule. If our style guide really doesn't make sense in a particular situation, then it's okay if a coder ignores it somewhere. Just as long as there's a good reason for it.

Some of the choices in this document are quite arbitrary. Sometimes alternatives would have been equally good, but without clear choices you can't have similar formatting for all programmers. This is especially true for bracing style. I know that this is a heated subject and while I have a clear preference, good arguments can be made for alternative bracing styles. (Still, it would have been nice if advocates of the other major style wouldn't have called it the One True Bracing Style... ;) )

A key element in our style guide is that I want code to read like English wherever possible. Variable and function names should be descriptive and only the most commonly known abbreviations are allowed. Brevity isn't a concern of mine, readability is.

Not all points in our style guide are about formatting though. Others are about actual language constructions. C++ is a rich language with an enormous amount of possibilities, but quite a few are too confusing or have too much risk of bugs to actually use. For example, nesting ternary operators is totally possible in C++, but the result is rarely readable so we disallow it altogether.



Our style guide also contains some rules that are intended to make cross platform development easy. On consoles you usually can't choose your compiler, so you have to work with whatever Nintendo, Sony or Microsoft have chosen, including the limitations of their compilers. We've researched what features of C++ each supports and have forbidden some of the newer C++ constructions that we think might not work on one of the consoles. Since we're not currently actively developing on some of the consoles we went by documentation only though, but I'd rather be too strict here than too lenient.

Another thing you can see in our style guide is my dislike for complex language constructions. C++ allows for some highly impressive stuff, especially using templates and macros. While I appreciate that these tricks can sometimes be really useful, I generally dislike them whenever they become too difficult to read. In the rare cases where these tricks are truly needed they are allowed, but usually I prefer if complex language constructions are avoided.



One particularly hotly debated point in coding styles is whether to mark class member variables. If the Car class has a float speed, do we call that speed, mSpeed, _speed or something else still? I've chosen to simply call this speed. Here too the reason is that I want code to be as similar to English as possible. The more prefixes and underscores there are, the further it moves away from natural language and the more difficult it becomes to just read code and understand it like you would text.

However, there's a good reason many programmers mark their member variables: it's very important in code to know whether a variable is a class member, a function parameter or a local variable. This argument is true, but I think we have that covered elsewhere: our style guide contains limitations on how long a class or function can be. If a function is short and fits on one screen, then it's easy to immediately see where variables are coming from. I think if classes and functions are short enough, then markers for member variables aren't really needed.

Note by the way that the rule for how long functions and classes can be is the one broken most internally. Sometimes it's just really difficult to split a class or function in a neat way. In the end the goal of our style guide is to produce clear code, not to hinder that by forcing clumsy splits. Still, there's real skill in figuring out how to neatly split classes and functions into smaller, more maintainable units, so if you're not super experienced yet then more often than not a neat split is possible and you just don't see it. In my opinion the ideal size of a class is anywhere between 200 and 400 lines, but a rule that strict isn't feasible so what's listed in the style guide is more lenient.

Now that I've discussed the reasoning behind our coding style guide, let's finally have a look at what it's actually like!

The Ronimo Coding Style Guide


There is an exception to every rule. However, keep to these rules as much as possible to maintain a constant layout and style for all the code. A lot of this is taste and a constant code layout requires setting aside one's own taste and keeping to these rules. Once used to it, it's easier to read such code.

When working in another language than C++, try to keep as close as possible to the C++ coding standard, but of course within reason. There are some specific notes on C# at the bottom as well.

C++


  • All code and comments are in Great Britain English. Not American English. So these are right: colour, centre, initialiser. These are wrong: color, center, initializer.
  • Every comma is followed by a space, for example doStuff(5, 7, 8)
  • Spaces around operators, for example: 5 + 7 instead of 5+7
  • Tabs have the same size as four spaces. Tabs are stored as tabs, not as spaces.
  • Functions and static variables are in the same order in the .h and .cpp files.
  • Use #pragma once instead of include guards (we recently switched to this so you will still see a lot of old include guards in our code).
  • Try to make short functions, preferably no more than 50 lines.
  • Avoid making very large classes. Split a class whenever you anticipate the class will grow a lot and you can cleanly split it. In general, try to keep class sizes below 750 lines. However, don't split a class if the split would result in messy code. Some examples of messy divorces are: too tightly coupled classes, friend classes and complex class hierarchies.
  • Don't write lines that are longer than what fits on a normal 1920*1080 screen (keep in mind that the Solution Explorer is on that screen as well).
  • When splitting a long line over several lines, make sure indentation is correct with the relevant parenthesis. For example like this:
    myReallyLongFunctionName(Vector2(bananaXPos + xOffset,
                                     bananaYPos * multiplier),
                             explodingKiwi);
  • Use forward declaration where possible: put as few includes in header-files as possible. For example, use class Bert; instead and move the #include "Bert.h" to the .cpp-file.
  • Includes and forward declarations are ordered as follows:
    • Start with all forward declarations from our own code (in alphabetical order)
    • Then includes from our own code (in alphabetical order)
    • One empty line
    • Then per library:
      • Forward declarations from that library (in alphabetical order)
      • Includes from that library (in alphabetical order)
  • The include of the .h-file belonging to a .cpp file is always at the top.
  • Don't define static variables in a function. Use class member variables instead (possibly static ones).
  • Everything must be const correct.
  • Names of variables and functions should be descriptive. Long names are no problem, undescriptive names are. Only use abbreviations if they are very clear and commonly known.
  • The first letter of classes, structs and enum type names is a capital. Variables and functions start without a capital. Each further word in a name starts with a captical. Don't use underscores ( _ ) in variable names. So like this:
    class MyClass
    {
        void someFunction();
        int someVariable;
    };
  • Member variables aren't marked with something like an m in front of them or an _ behind. Functions should be short enough to keep track of what is declared in the function and what in the class. Don't mark member-variables with this-> either.
  • Implementations of functions are never in .h-files.
  • Templatised functions that can't be implemented in the .cpp file are implemented in an extra header file that is included from the main header file. Such a class can have 3 files: MyClass.h, MyClassImplementation.h and MyClass.cpp. So like this:
    class MyClass
    {
        template <typename T>
        void doStuff(T thingy);
    };
    
    
    #include "MyClassImplementation.h"
  • Start template typenames with T. If more info is relevant, you can add words after that, like TString.
  • Several classes can never be defined in the same header file, except if a class is part of another class (i.e. defined inside the other class).
  • Between functions there are two empty lines (in the .cpp file).
  • Use empty lines to structure and group code for readability.
  • Add loads of comments to code.
  • Write a short explanation of what a class does above each class. Especially make sure to explain relations there (e.g. “This class helps class X by doing Y for him”).
  • Curly braces { and } always get their own line, so they aren't put on the same line as the if or the for. They are also never left out. The only exception is if you have lots of similar single line if-statements right beneath each other. In that case it's okay to put them on a single line. Like in this example:
    if      ( banana &&  kiwi && length > 5) return cow;
    else if ( banana && !kiwi && length > 9) return pig;
    else if (!banana && !kiwi && length < 7) return duck;
    else                                     return dragon;
  • When writing a do-while function, put the while on the same line as the closing brace:
    do
    {
        blabla;
    } while (bleble);
  • Indent switch statements like this:
    switch (giraffeCount)
    {
        case 1: text = "one giraffe";  break;
        case 2: text = "two giraffes"; break;
        case 3:
            // If it's more than one line of code
            doStuffOnSeveralLines;
            text = "three giraffes";
            break;
        case 4:
        {
            // Can add curly braces for readability
            int x = getComplexThing();
            text = "quadruple giraffe";
            break;
        }
    }
  • Function parameters have the exact same names in the .h and the .cpp files.
  • If a function parameter and a class member variable have the same name, then either think of another name, or add an _ to the end of the function parameter. For example like this:
    void setHealth(float health_)
    {
        health = health_;
    }
  • Precompiler instructions (everything starting with #) should be kept to a minimum, except of course for #include and #pragma once.
  • Don't write macros.
  • Variables inside functions are declared at the point where they are needed, not all of them at the start of the function.
  • In constructors, prefer using initialiser lists over setting variables in the body of the constructor. Each initialisation in the initialiser list gets its own line. Make sure variables in the initialiser list are in the same order as in the class definition in the .h-file.
  • Don't use exceptions (unless you're using a library that requires it).
  • Don't use RTTI (so don't use dynamic_cast). RTTI costs a little bit of performance, but more important is that RTTI is almost always an indication of bad object oriented design.
  • Use reinterpret_cast and const_cast only when absolutely necessary.
  • Don't commit code that doesn't compile without errors or warnings (and don't disable warnings/errors either).
  • Don't commit code that breaks existing functionality.
  • No global variables. Use static member variables instead.
  • Use our own MathTools::abs instead of std::abs. This is because std::abs is inconsistently implemented between platforms, causing hard-to-find bugs.
  • Always use namespaces explicitely. Don't put things like using namespace std in code.
  • Never ever even think of using go-to statements. We have a thought detector and will electrocute you if you do.
  • Don't use the comma-operator, like in if (i += 7, i < 10)
  • Don't use unions.
  • Don't use function pointers, except where other libraries (like STL sort) require it.
  • Only use the ternary operator in extremely simple cases. Never nest the ternary operator. Example of a simple case where it can be used:
    print(i > 5 ? "big" : "small");
  • When exposing a counter for an artist or designer it starts at 0, just like it does for normal arrays in code. Some old tools might still start at 1 but anything newly developed for artists starts at 0.
  • When checking whether a pointer exists, make this explicit. So use if (myPointer != nullptr) instead of if (myPointer)
  • Use RAII (Resource Acquisition Is Initialization) where possible. So instead of creating a separate function initialise(), fully initialise the class in its constructor so that it's never in an incomplete state.
  • Write the constructor and destructor together: write the corresponding delete for every new immediately, so you don't forget to do it later on.
  • If you add temporary debugging code, then add a comment with QQQ to it. Never commit code with QQQ: remove the debugging stuff before committing.
  • If you want to leave a marker for something that needs to be done later on, use QQToDo. If it's something you need to do yourself then add your letter to it, for example QQToDoJ. Only commit QQToDo if it's really impractical to finish it right away.
  • Class definitions begin with all the functions and then all the variables. The order is public/protected/private. This might sometimes mean you have to use the keywords public/protected/private several times in a single header file (first for functions, then again for variables).
  • A class starts with its constructors, followed by its destructor. If these are private or protected, then they should still be at the top.
  • When something has two options, but isn't clearly true/false, then consider using an enum class instead of a boolean. For example, for direction don't use bool isRight. Instead, use enum Direction with the values Left and Right.
  • Instead of std::string and std::stringstream, use our own classes RString, RStringstream, WString and WStringstream (these tie into our own memory management).
  • The variable float time always stands for the time since the last frame, in seconds. If time means something else, make this explicit, for example by naming it timeExisting instead.
  • Make sure all code is framerate-independent, so use the variable float time often to achieve this.
  • Make the intended structure explicit and clear. For example, avoid doing things like this:
    if (yellow)
    {
        return banana;
    }
    return kiwi;
    What was really intended here, was that depending on yellow, either banana or kiwi is returned. It's more readable to make this explicit like this:
    if (yellow)
    {
        return banana;
    }
    else
    {
        return kiwi;
    }
  • Use nullptr instead of NULL.
  • We only use auto for things like complex iterator types. For everything else we explicitly type the type.
  • Use range-based for where applicable, for example:
    for (const Banana& ultimateFruit : myList)
    (Note that it's important to type that reference when not working with pointers, since otherwise the Banana would be copied in this case.)
  • Use override and final wherever applicable.
  • If a function is virtual, the virtual keyword should always be added to it, so not only in the parent class, but also in each version of the function in the child classes.
  • Use strongly typed enums, so with the class word. Their casing is the same as for classes. For example:
    enum class Fruit
    {
        Banana,
        Kiwi,
        ApplePie
    };
  • Don't use rvalue references (&&) unless you really really really need them.
  • Use unique_ptr wherever possible. If an object isn't owned, then it's stored as a normal pointer.
  • Avoid creating instances of complex types in the initialiser list. Simple copying and setting goes into the initialiser list, while more complex code like calling new goes into the body of the constructor. Here's an example of how that works:
    FruitManager::FruitManager(Kiwi* notMyFruit):
        notMyFruit(notMyFruit)
    {
        bestFruitOwnedHere.reset(new Banana());
    }
  • Only use shared_ptr in cases where shared ownership is truly needed. In general, try to avoid shared ownership and use unique_ptr.
  • Lambda's that are too big for a single line are indented like this:
    auto it = std::find_if(myVec.begin(),
                           myVec.end(),
                           [id = 42] (const Element& e)
                           { 
                               return e.id() == id;
                           });
  • Don't use MyConstructor = delete (this doesn't seem supported on some compilers we might need).
  • Don't use the C++11 feature for list initialization (this doesn't seem supported on some compilers we might need). So we don't use this:
    std::vector<int> thingy = {1, 2, 3};
  • Don't use any features that were added in C++14.

C#

  • Variables at the top of the file instead of at the bottom. We do a strict split between functions and variables (like we do in C++), so here it's first all variables, then all functions.
  • async functions must always end their name with Async, for example eatBananaAsync.

That's it, our coding style guide! ^_^ While I imagine you might disagree with a bunch of the specific rules, I think it's useful for any company that does programming to have some form of a style guide. Our style guide can be a good starting point for creating your own. I'm quite curious: what's the style guide like at your own company and how do you like it? Do you have one at all?

Sunday, 2 July 2017

The Ronimo coding methodology

I'm a strong believer in working in a structured way. As a game programmer you need to build complex systems and just going with the flow isn't good enough. That's why I've written two documents to describe how we code at Ronimo, which every programmer and intern gets to read on their first day. Our Methodology document explains the workflow while the Style Guide explains the layout of our code. Today I'd like to show our Methodology and describe the reasoning behind the rules in this document.



Note that the contents of this document aren't very original: most if it is a combination of common agile practices that I like.

Let's start by having a look at the actual methodology document:

The Ronimo coding methodology


General method of implementing a new feature:

  1. Analyse what should be made. Discuss approach with end users (usually artists and/or designers) and lead programmer.
  2. Split into small tasks of a day at most.
  3. Make a basic planning for the small tasks. Put the core of the functionality and the most difficult parts to implement at the front of the planning.
  4. Implement each small task.
  5. Evaluate result with end users and lead programmer.
  6. Explain to end user what the new functionality does, so that it's actually used.

Implementing a small task:

  1. Analyse what should be made.
  2. Verify that related existing code is bug-free (perform tests!).
  3. Research and experiment with any new technology required for this feature.
  4. Code-design for new functionality. Don't focus too much on theoretical future use, but do keep it in mind.
  5. Refactor existing code to make room for your new feature.
  6. Test whether older functionality is still intact.
  7. Implement new functionality.
  8. Test whether new functionality works.
  9. Finish code: add comments, clean up code and check the destructors.
  10. Test whether new functionality still works.
  11. Test whether older functionality still works.
  12. Evaluate result. If showable, also show this intermediate result to the end user.

Various other rules:

  • Keep a personal list of all the small things that you should do. If you come across some issue that you can't immediately take care of, or if someone asks for a feature that you promise to build later, then write that down in your list. Don't think you can always remember everything.
  • Never continue to work on something if you have encountered a crash or major bug. Always fix the crash first.
  • "Premature optimization is the root of all evil" (Donald Knuth). First implement a new feature in the simplest / clearest way possible, then analyse whether it works, then analyse the performance and only if necessary implement optimisations.
  • Don't worry too much about making your code in such a way that unknown future extensions might be easy to add. When new functionality is needed, the code can still be refactored. Of course, if it's little work to do, then do make things as generic as possible.

Most of these are quite clear, but it's interesting to discuss some of the reasons behind these rules. Often enough I've seen coding interns have loose ends everywhere in their code because they were working on five things at the same time and forgot to test, clean up and finish some of them. That's why our coding methodology requires that you finish what you were doing before you get to the next thing. This is also why I require that big tasks are split into smaller ones: a person can only remember so much and the more things you're working on simultaneously, the bigger the chance that you'll overlook something important.

At the same time I prefer the agile way: only make what you actually need and expand the codebase as you go. During early development you don't know all the features you'll need, nor do you know for all problems how you'll solve them. However, adding things one at a time will often make code bloated and without focus, and will muddy class responsibilities. That's why code needs to be refactored often. Refactoring requires discipline. Often you can hack in a new feature in just a few hours, or first spend half a day refactoring to make room for that feature in a good way. It's easy to skip or postpone refactoring, but doing so often produces unworkable code in the long run. That's why refactoring is an explicit step in our methodology.

Focusing only on one small new thing at a time shouldn't be taken too literally though. While I think it's important to not work on other things until what you're doing is finished and clean, that doesn't mean you shouldn't look ahead. When building something complex it's important to think about how you're going to make the whole system work. I once had an intern who had to rebuild a large part of a tool because he had taken our coding methodology too literally and hadn't thought ahead at all. The most complex features of the tool were not possible at all with what he had build. The key here is to find the right balance between thinking ahead and focusing on one thing at a time.



Another staple of mine is that programmers need to communicate directly with designers and artists. We believe extensive design documents are rarely a good idea, so the only way to know what's needed exactly is to talk to the designer or artist who needs a new feature. Often that person hasn't defined the exact details of the feature, so the coder needs to discuss it with them and think about the caveats, also from a design and art perspective. To smoothen this communication it helps a lot if the programmer has a little bit of experience in design and art, but even if that's not the case I think it's the programmer's job to talk the language of the designer or artist, not the other way around. It's really difficult for a designer to speak code, but a programmer should be able to talk about his work in comprehensible English (or Dutch in our case).

One thing that's surprisingly missing in our Coding Methodology is unit testing. We have a strong focus on testing our own code extensively, but the document doesn't say you need to write unit tests. This is because I think gameplay is often too chaotic and unpredictable to test well with unit tests. Certain things are testable with unit tests, but the bugs we encounter are often not things where I can imagine how a unit test would have found them. Often it's things that function fine but result in undesired gameplay.

I do realise that not making units tests makes us more vulnerable to bugs than a team that always writes unit tests, so we emphasise that if a crash or major bug is found, it needs to be fixed right away. We might have more bugs than software developers who write extensive unit tests, but at least we fix them quickly. I do think we ought to use unit tests more for things like server architecture. Unit testing isn't in our blood at all and it probably should be at least a little bit. I'm curious though: do you use unit tests for your gameplay or engine code?

Regardless of whether you agree with the particular rules in our methodology document, I think it's important that all programmers think about their workflow. Just doing whatever you feel like doing isn't good enough. Discipline and structure are important for anyone who works on larger, more complex systems. What's you're coding methodology like? If you happen to work at a company, is there an official document like the one I've shown today?

So that's it, the Ronimo Coding Methodology! Next week I'll show our Coding Style Guide, which is quite a bit more strict than most coders are used to.

Sunday, 4 January 2015

What most young programmers need to learn

In the past 7.5 years I have supervised over a dozen programming interns at Ronimo and have seen hundreds of portfolios of students and graduates. In almost all of those I saw the same things that they needed to learn. One might expect that I think they need to learn specific techniques, algorithms or math, or other forms of specific knowledge. And of course they do, but in my opinion that is never the main thing. The main thing they need to learn is self discipline. The discipline to always write the clearest code you can, the discipline to refactor code if it becomes muddy through changes later in development, the discipline to remove unused code and add comments.

Most of the time I spend supervising programming interns is spent on these topics. Not on explaining advanced technologies or the details of our engine, but on making them write better code in general. I always ask applicants what they think is important in being a good programmer and they usually answer that code should be clear, understandable and maintainable. That is indeed what I want to hear, but it is very rare for a young programmer to actually consistently follow through with that.

Keeping this in mind requires self discipline, because it means not stopping "when it works". If all the variables would have the wrong name the code could still function perfectly, but the code would be super confusing. The step from functional code to clear code brings very little reward in the short term: it worked already and after cleaning it up it still works. That is why discipline is required to take this step. That is also why doing an internship is so useful: a good supervisor is vigilant on code quality (even though the definition of "good code" might of course differ per programmer) and thus forces the intern or junior to always take that next step.

Let me give a few examples of the kinds of things I often see in code written by starting programmers:

Liar functions/variables/classes

These are functions, classes or variables that do something else than their name suggests. Their name is a lie. It is very obvious that names should be correct, but to my surprise it is quite common for names to be completely off.

An example I recently encountered in code written by a former intern was two classes: EditorGUI and EditorObjectCreatorGUI. This is code that handles the interface in our editors. To my surprise it turned out that the code that handled the button for creating new objects was in EditorGUI, while EditorObjectCreatorGUI only handled navigating through different objects. The exact opposite of what the naming suggests! Even though the code was relatively simple, it took me quite a while to understand it, simply because I started with a completely wrong assumption based on the class names. The solution in this case is really simple: rename EditorObjectCreatorGUI to EditorObjectNavigationGUI and it is already much, much more understandable.

This is something I see a lot: names that are simply incorrect. I think this often happens because code evolves while working on it. When the name was chosen it might have been correct, but by the time the code was finished it had become wrong. The trick is to constantly keep naming in mind. You have to always wonder whether what you are adding still fits the name of the function or class.

Muddy classes

Another problem I see is muddy classes: classes that do a lot of unrelated things. Again this is something that happens as you keep working on the same code. New features are added in the easiest spots and at some point classes become bloated with all kinds of unrelated behaviour. Sometimes the bloating is not even in the size of the classes: a class might be only a few hundred lines but still contain code that does not belong there.

An example of how this can happen is if for some reason a GUI class needs to analyse what textures are available (maybe because there is a button to select a texture). If the GUI class is the only class that needs the results of this analysis, then it makes sense to do that in the GUI class. However, then some totally unrelated gameplay class for some reason also needs that info. So you pass the GUI class to that gameplay class to query the texture information. At this point the GUI class has grown to be something more: it is also the TextureAnalyser class. The solution is simple: split off the TextureAnalyser class into a separate class that can be used by both the GUI class and the gameplay class.

The general rule of thumb to avoid this problem is to always wonder: does the functionality that I am adding here still fit the name of the class? If not, then the class either needs to be renamed, or it needs to be split into separate classes or the code needs to go into a different class.

It is usually a Bad Smell if you cannot come up with a fitting name for your class. If you cannot describe what a class does in its name, then maybe what it does is too muddy. It might need to be split into parts that make more sense and can actually be described with a proper name.

Oversized classes

This one is really similar to the muddy classes above: over time more and more is added to a class and it gets bloated. In this case however it all still makes sense to be in one class, but the class simply grows too big. Gigantic classes are cumbersome to work with. Bugs slip in easily as there is a lot of code manipulating the same private member variables, so there are a lot of details one can easily overlook.

Splitting a class that has grown too big is quite boring work. It can also be a challenge if the code in the class is highly intertwined. Add to this that it already works and that fixing it adds no new functionality. The result is again that it requires serious self discipline to split a class whenever it becomes too big.

As a general rule of thumb at Ronimo we try to keep classes below 500 lines and functions below 50 lines. Sometimes this is just not feasible or sensible, but in general whenever a class or function grows beyond that we look for ways to refactor and split it into smaller, more manageable pieces. (This makes me curious: where do you draw the line? Let me know in the comments!)

Code in comments

Almost all sample code that applicants send us contains pieces of code that have been commented out, without any information on why. Is this broken code that needs to be fixed? Old code that has been replaced? Why is that code there? When asked applicants are usually well aware that commented-out-code is confusing, but somehow they almost always have it in their code.

Parallel logic and code duplication

Another problem that I often see occurring is to have similar logic in several spots.

For example, maybe the name of a texture gives some information as to what it is intended for, like “TreeBackground.dds”. To know whether a texture can be used for a tree we check the filename to see whether it starts with the word “Tree”. Maybe with the SDK being used we can check that really quickly by just using filename.beginsWith(”Tree”). This code is so short that if we need it in various spots, we can just paste it there. Of course this is code duplication and everyone knows that code duplication should be avoided, but if the code being duplicated is so short, then it is tempting to just copy it instead. The problem we face here is obvious: maybe later the way we check whether a texture is fit for a tree changes. We then need to apply shotgun surgery and fix each spot separately.

A general rule of thumb here is that if code is very specific, then it should not be copied but put in a function. Even if it is super short and calling a function requires more code than doing it directly.

All of the things discussed in this blogpost are really obvious. Most of these things are even taught in first year at university. The challenge is to make the step from knowing them to actually spending the time to always follow through with them, to always keep them in mind. This is why the most important thing that all programming interns learn at Ronimo is not knowledge, but self discipline.

Sunday, 6 July 2014

Why composition is often better than inheritance

An important question in code structure is whether to make classes work together through composition or inheritance. The "has a" relationship versus the "is a" relationship. For example: a seat has a cushion and a seat is a piece of furniture. While in this example the difference is really obvious, there are in practice many cases where both could make sense. Does a character in a game have a collision box, or is it a collidable object? These are not the same, but each can be used as the main structure for collision handling (or even both together) and it is not always clear which is better. In my experience intuition often favours inheritance, but it gives so many problems that in many cases composition is better.

Let's first have a look at an example of how the same problem can often be solved in both ways. In Awesomenauts we have a separate class that handles the 'physics' of a character. This class handles things like gravity, knockback, sliding and jumping. Since a character is a physics object, it would make sense to say that Character inherits from PhysicsObject. It is also possible to say that a character has a PhysicsObject that handles its collision. Instead of has a we might also word this as uses a.



Let's see what this relation would look like in code. This is a highly simplified version, but it shows the basic concepts nicely:

class PhysicsObject
{
 Vector2D position;

 void updatePhysics();
 void applyKnockback(Vector2D force);
 Vector2D getPosition() const;
};

//Using PhysicsObject through inheritance
class CharacterInheritance: PhysicsObject
{
 void update()
 {
  updatePhysics();
 }
};

//Using PhysicsObject through composition
class CharacterComposition
{
 PhysicsObject physicsObject;
 
 void update()
 {
  physicsObject.updatePhysics();
 }
 
 void applyKnockback(Vector2D force)
 {
  physicsObject.applyKnockback(force);
 }
 
 void getPosition() const
 {
  return physicsObject.getPosition();
 }
};

As you can see in this code, CharacterInheritance is shorter. It also feels more natural, since we don't have to write these extra accessor functions for applyKnockback and getPosition. However, after years of creating both of these kinds of structures I have learned that in a situation like this, using composition is actually more flexible, less sensitive to bugs and even more understandable than using inheritance.

Flexibility

Let's start with the flexibility argument. What if we want to make an enemy that consists of two blocks linked by an energy chain. This enemy does damage to anyone who touches the chain. The blocks can move separately, making for some interesting positional gameplay when fighting this enemy. The two blocks this object consists of move entirely separately and each have their own physics. Knocking back one block does not influence the other block. Yet they really are one character: they have one healthbar, one AI, one entry on the minimap and can only exist together.



With a composition structure it would be pretty straightforward to create this enemy, since we could just create a character that has several PhysicsObjects. With inheritance however we cannot make this as a single character, since we cannot inherit from PhysicsObject twice. We could probably work around this somehow and make it work with inheritance, but it quickly becomes much less simple and intuitive.

If you read this and think this is a far-fetched situation and not all that relevant, then you have probably never been in a project of significant size where gameplay was king. Game designers constantly come up with game mechanics that are exceptions to what you already programmed. Saying no to these just because your code structure cannot handle them will seriously damage your game quality, since in the end the only argument should be whether it is fun to play (okay, and maybe whether it is achievable within the scope of the project).

Just take a look at the diversity of the hundreds of upgrades in Awesomenauts and you will realise how many of those were likely exceptions to what our code could do. Our designers came up with those upgrades and the code had to make it work somehow. An important goal in game programming is flexibility: making your code in such a way that it is relatively easy to add whatever weird whim the game designers come up with today. In most cases composition is much more flexible than inheritance.

Readability

My next argument against inheritance is readability. "Readability" is always accomponied by "sensitivity to bugs", since if a programmer does not really understand how something works, then he will likely break it when working on it.

At Ronimo one of the more important rules in our coding standard is that we strive to keep the size of all classes below 500 lines of code. We don't always see ways to keep to that, but the goal is clear: keep classes relatively short so that they are easy to understand and a programmer can fit the workings of the entire class in his head.

Let's say our code has grown over time as more and more features were added to our game, and Character and PhysicsObject have both grown to 500 lines of code each. We have also added two more classes that use PhysicsObject: Pickup and Projectile. These are also 500 lines of code each.



In such a situation inheritance will usually make all of this very confusing. We might have strived to keep as much private as possible, but in the end inheritance almost always introduces a bunch of virtual and protected functions. In other words: PhysicsObject has become pretty intertwined with its three child classes Character, Pickup and Projectile. In my experience this nearly always happens: inherited classes work together to create complex behaviour and intertwine more and more over time.

By itself this might not be that much of a problem, but to really understand any of these classes we now need to know them all. If we restructure something in PhysicsObject because Projectile needs a new feature, then Character and Pickup will also be involved. To understand the entire situation, the programmer now needs to think about four different classes and fit all 2000 lines of code in her head simultaneously. In my experience this quickly becomes impossible: this is just too much code to really grasp it all at once without starting to mix things up. The result is that readability decreases and the programmer becomes more likely to introduce bugs because she overlooked something.

Of course a composition based structure does not magically fix all of this, but it does help a lot in keeping this structure simple and understandable. With composition there is no virtual or protected, so the separation between PhysicsObject on the one side and Character, Projectile and Pickup on the other is much clearer. This keeps the classes from intertwining over time and makes it easier to keep them truly separate. While they could in theory also have been kept separate with inheritance, in my experience this is too difficult to maintain, while composition enforces it. We have numerous cases in our code where an inheritance structure of classes that all grew too big is hardly understandable any more.

Diamond problem

Any discussion of inheritance versus composition is incomplete without mentioning the famous diamond problem. What happens when a class A inherits from two classes B and C that both inherit from a single parent D? A now has a D twice and chaos ensues.



There are several solutions to the diamond problem in C++. For example, you might just accept that A has double D. (Haha, double D! Ahum.) Or you might use virtual inheritance to solve it. However, both solutions cause all kinds of problems and potential bugs, so just avoiding the diamond problem altogether is highly preferable.

This problem is usually not around in the initial design of a gameplay code structure, but as features are added it might pop up occasionally. The problem is that when it does, it can often be very difficult to come up with a good solution for how to get rid of it without doing a lot of refactoring.

Nevertheless, the diamond problem has only popped up a couple of times in my 12 years of object oriented programming, so I don't think the diamond problem is a very strong argument against inheritance.

(By the way, even without the diamond problem complex multiple inheritance structures tend to get a little sleazy, as I explained in this previous blogpost about why this is not always this.)

Use inheritance, but use it less

Does all of this mean that I am against inheritance in general? Nope, absolutely not! Inheritance is very useful for a lot things, for example in polymorphism and in design patters like listeners and factories. My point is just that inheritance is not as generally useful as it might seem. There are lots of cases where inheritance may seem the cleanest solution, while in practice composition would be better. So use inheritance, but don't overuse it.

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.

Saturday, 12 May 2012

Programming with pasta

Today brings the finale to this epic series of two posts on the many connections between Italian eating habits and creating games. Whereas last week I discussed how lasagne influences game design in a good way, today I will talk about all kinds of Italian food. I will explain why programmers should not only fear the dreaded spaghetti, but also the less-known evil of lasagne. And how about pizza and ravioli? What can they learn us about the art of code?

Note that I realise that the food metaphors might be slightly lame, but the point of this post is that I think it is important to think about the implications of various structures and methods of programming, and this blogpost is basically just an analysis of some pitfalls and approaches.

Most programmers know the term spaghetti code. It refers to a larger code base that is so intertwined, that is it impossible to know how the rest of the code reacts when you change something somewhere. Every line of code might be referencing almost any variable anywhere and maintaining the code becomes all but impossible, because the risk of introducing bugs elsewhere is too great.



The common solution to spaghetti code is also well known: object oriented programming. By using classes and making them as self-contained as possible, you can make sure that as long as the class behaves the same to the outside world, it doesn't matter if you change how it works internally. Each class is a world of its own. Of course, in practice this is more an ideal than reality, because during development classes often also need to change how they behave towards the outside world. Still, object oriented programming can help a lot to separate large code bases into smaller, more maintainable chunks.

However, there is also the other end of the spectrum. Object oriented programming can be taken too far. Let's have a look at an example from a real part of Awesomenauts. In Awesomenauts, when the player dies, the music fades out and decreases in pitch. The real situation is a bit more complex, but to keep this post readable, I'm simplifying it a bit. An intern was to implement the logic for the music. So he came up with something like this set of classes:



However, due to his internship ending, he didn't finish this set of code and I had to complete it. Problem is, that these are so many classes for such a simple task, that I almost got lost in all all the class management code and could hardly make any sense of what was working and what was not. Because the problem was so simple, I decided to just throw away his code altogether and write one simple class instead. It looks like this (again, simplified for clarity):



As you can see, this is actually really simple and understandable. No need for all these classes here. It became a little bit more complicated when shuffle and killing spree music were added, but still, the MusicManager class is short and understandable.

So what the intern did, is an example of lasagne coding: too many layers. Especially programmers who are relatively new to object oriented programming often overreact to its benefits by creating way too many classes. I belive there is something like an ideal class size. This varies per class, but I think in general classes above 500 lines are almost always too long, causing spaghetti within the class, while classes beneath 50 lines are quite often too small and thus part of lasagne. Not always of course: some classes are perfectly fine for doing one clear thing in a small number of lines.



Lasagne coding is of course a common term, but I recently discovered that few programmers are aware of this name. Most coders I meet do know that having too many classes is a bad thing, of course, but I think it helps communication and thinking to also have terms for good and bad practices.

To finish this post, I'd like to mention two more food metaphors. Pizza code and ravioli code. Unlike spaghetti code and lasagne code, these are terms I just made up on the spot because I like to talk about food. The concepts are real, though.

When programming large projects, interrelations between classes often become more and more complex. Awesomenauts has around 1,200 classes, which to me is quite a lot. Even if all of those classes are proper self-contained units, adding or changing things can still become very complex, as any number of classes might be using any number of other classes. If care is not taken, the class diagram starts to look like a pizza: a big plate of dough with all kinds of stuff randomly thrown on it and no idea what goes where or why. All the classes just happen to be there.



So to make the class diagram more manageable, I think it is better to make it resemble ravioli. Ravioli are little dough wraps with good stuff in them. They are closed off and you put the entire thing in your mouth at once and it tastes good. I'd like my code to be like that. Groups of classes work together, but can be represented to the rest of the code base as a whole. Not all of them need to even be usable outside the group at all.



The nicest example of this in Awesomenauts is the input system. This consists of some two dozen classes that together handle input from devices like WiiMotes, keyboards, mice and Xbox 360 controllers. It also handles this for five different platforms (Playstation 3, Xbox 360, Wii, PC, Mac). To the outside, however, only one of the twenty classes is accessible: the InputManager class. All other code that wants to know whether a button is pressed on some device just asks this to simple functions like InputManager::getIsMouseButtonDown(). InputManager is the dough and the two dozen classes that actually handle the hardware on all the various platforms are hidden inside the ravioli ball. If all those 1,200 classes in Awesomenauts were this clearly grouped, you would get a much more manageable 50 groups to understand the relations between, instead of 1,200 separate classes.

This concludes my food series for now. Of course, none of the concepts above are revolutionary, but I think it is really important for any programmer to think about what he thinks is good programming and what is not. In my opinion programmers who never stop coding to take some time to think about whether what they used was a good or a bad method, will never become really good. This post just outlines some of my own thoughts on the matter, and I hope they help you define Good Code (tm) for yourself.

Sunday, 30 January 2011

Resetting versus re-creating

One weird aspect of my career as a programmer is that I never worked under someone with more experience. I started Ronimo Games right out of school and was the only programmer, and thus directly the lead. Since I did my internship as a 3D artist, this means I never saw how other companies handle their programming. The result is that all of the coding style decisions at Ronimo are just based on my own opinion, and I may be missing some important things that I simply never thought about.

Interestingly, we have been hiring some programmers in the past year, growing the team from just me to five full-time programmers next March. Although the two programmers who have already been working with us for the past year (Ted and Maarten) are former interns of mine, it turned out that as soon as we hired them as real employees, they actually had Opinions. Which is great, because not just blindly doing what I come up with greatly helps against tunnel visions!

One important thing I learned that way recently, is that resetting and reusing objects is error prone, and that a structural change can avoid this.

Let me explain this through an example. In Ronimo's Secret New Game (tm), the player can control a character that when killed, will respawn after a while. My method for doing this, was to hide the character on death, and then reset him when he is respawned, and make him visible again so that the player can continue playing with him.



The problem with this is, that it is easy for the programmer to forget some elements when hiding and resetting. The character consists of many graphical elements, and since they are in very different categories (like healthbar, shadow, character, special effects, etc.), it is not feasible to put all of them in a single list. So when programming the hiding, I need to think of all these elements. The same kind of issue occurs when resetting: many aspects need to be reset (like combo counters, cooldowns on actions, poison time remaining, etc.), and again I need to think of all of them when resetting.

Adding new features is even worse: I always need to remember to add them to the hiding function and to the resetting function. Forgetting one of them results in a bug. Not hiding something is an easy bug to spot, but not resetting something is a lot more difficult to notice, since many effects may usually time out before the respawn timer is over. So it is easy to overlook that sometimes a cooldown may not have expired yet when the player respawns.

This may seem like a subtle and irrelevant issue, but it is very easy to accidentally introduce bugs here when developing a game with more then one programmer, since not everyone may know about these details. So I had been struggling with this for a while, without being able to think of a good solution.

Recenlty, my colleague Maarten came up with a really simple fix: why not completely delete the character on death, and re-create him on respawn? Now we don't need to hide or reset anything. Win!



It turns out that this simple principle actually applies to a lot of things. For example, in Swords & Soldiers, we create all the menus when the game starts, and then just hide them all, except for the one that is currently in view. But why not simply delete them entirely?

Kind of strange that I hadn't thought of this myself, really!

Experienced C++ programmers may counter this argument by saying that dynamic memory allocation costs performance, so resetting is more efficient. This is probably true in most cases. However, I consider design that prohibits bugs to be a lot more important than performance. Current console hardware is very fast and we are not trying to make the next Killzone or Uncharted here.

Anyway, this could also be implemented using placement new (which constructs objects on a piece of memory that you allocated before, instead of requesting new memory). Finally, we have a really fast custom memory manager at Ronimo, so small allocations (up to 256 bytes) are very fast anyway.

Although I didn't think of this re-creation trick myself, it very much fits the coding style at Ronimo: we always try to find structures that make it as difficult as possible to accidentally introduce bugs. Destroying and re-creating objects instead of resetting and reusing them is a great example of this philosophy! Although this example is no super-complex science, I think that one of the most important skills for any programmer is to constantly look at your own code for these kinds of small improvements.

Wednesday, 20 October 2010

Dirty collision detection trickery in Proun

Some people seem to read Pr0n instead of Proun. I think this may be due to the dirtiness of some of the coding tricks I used in Proun.

Or maybe it is because these words are written so similarly. No, really, I don't think so.

Anyway, programmers who worked with me might agree that I have pretty radical ideas on what good programming is, especially in comparison to how large console games are usually made. I almost always choose simplicity and understandability over performance, over memory use and often even over amount of work.

Most coding interns need to get used to the idea that sometimes when they consider something done, but there is still a way of structuring it in a better way, I actually make them go back and change their already working code, just to make the code itself better. Since the games at Ronimo are around 100.000 lines of code, keeping the code clean is very important. No way will we find our bugs if such an amount of code is also a mess!

An extreme example of choosing simplicity over performance can be found in the collision detection in Proun.

Collision detection in games is usually done per frame. So 60 times per second, all objects are moved a bit and then the game checks whether they collide. This means that the time between two frames is completely ignored!

This becomes a problem when fast moving objects (e.g. bullets) pass through thin obstacles: in one frame they are on one side of the obstacle, and in the next frame they already passed it.



Common solutions to this problem are to cast a ray between the position at the previous frame and the current position. An alternative is to do sub-steps just for the collision detection and only when moving really fast.



Both of these solutions share that they would need additional work in the collision detection of vehicles in Proun. When doing the solution with subframes, the collision detection would run at a different framerate from the rest of the game, which increases the complexity of the code. Not dramatically, but it is more complex nevertheless.

For Proun, I implemented a simpler solution: all gameplay in Proun runs at a fixed 3000 frames per second. Three thousand. Three thousand! The graphics still runs at 60fps, of course (or less if your videocard is too slow).

The result is that even when you race extremely fast (grab a turbo on Impossible difficulty to do that), you still don't move too fast to pass through objects without collision detection detecting this.

Any reasonable programmer would say this is idiotic. So much performance wasted! I even update all obstacle animations at this framerate. Unnecessary! However, Proun's performance is almost entirely in the graphics. The gameplay is very simple and can be processed really quickly, so why would I bother implementing something more efficient and more complex? Doing a lot of gameplay updates per frame is really simple to implement and everything just works. I don't need to add any math for the time-in-between or anything like that, I don't need to think about having different framerates for collision detection and for other gameplay aspects. Any other solution would have made the collision detection code more complex than it is now.

I tested the actual performance impact of this and it turns out that this only impacts the framerate in a relevant way when a really slow processor is combined with a very fast videocard. No one actually has such a combination, so I don't consider that relevant.

So. Simple! Clear!

(And in fact not dirty at all, so maybe the words Pr0n and Proun do look pretty similar...)