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?

12 comments:

  1. nice post, lots of good tips.
    About static members. What if your class/lib is header only (e.g. templated classes often are).
    Do you permit this trick for getting static member (it uses local static variable)?

    class Bla {
    public:
    static Stuff& getStuff() {
    static Stuff stuff;
    return stuff;
    }
    };

    ReplyDelete
    Replies
    1. That's a very particular trick, not something that would be commonly used I think. We do allow tricks like that when needed though, but they need to be really really needed since it's also kind of dirty and obscure. Really useful in some cases though. :)

      Delete
    2. I've used this trick to formalize singleton access to a class (which I use a lot of).

      https://github.com/vovoid/vsxu/blob/master/lib/common/include/tools/vsx_singleton.h

      Is used like this:

      class shot_manager
      : public vsx::singleton
      {
      vsx_nw_vector< shot > shots;
      public:
      ...
      };

      --

      The use case looks like:

      shot_manager::get()->is_a_shot_within_rect(my_rect);

      There is also the managed one (to control order of creation / destruction which is problematic with static variables, especially in games):

      https://github.com/vovoid/vsxu/blob/master/lib/common/include/tools/vsx_managed_singleton.h

      Delete
  2. you should investigate clang-format for 1/3 of these rules.

    ReplyDelete
  3. "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."

    This is not as relevant if you have access to C++11. Then just go:
    class foo
    {
    public:
    size_t bar = 0;
    ...
    };

    For me at least, not adding things to the initialiser list is my most common error. With C++11 this way I can get rid of most default constructors.

    ReplyDelete
    Replies
    1. That's a neat feature! I actually didn't even know that one. :)

      Delete
    2. You didn't like it, but initializer lists can go there as well. Useful for some things like color, where you have a fixed number of member variables...

      vsx_color my_color = {1.0, 0.5, 0.5, 1.0};

      Delete
    3. That particular feature is disallowed because one of the console compilers that need to be able to run our code list it as unsupported.

      Delete
  4. It's always interesting to see the code style guide of other companies.
    Because I've help write the style guide for Two Tribes I’ve thought about this topic a lot, and I had some feedback on the Ronimo style guide.
    So much in fact, that it was too long for the comments here.
    I’ve updated my reply here: https://pastebin.com/9fqzaFaw

    ReplyDelete
    Replies
    1. I agree that this style guide is a mix of things intended for juniors and things that are also relevant to seniors. Even for a senior it's not a problem to read through this once though, so I don't see much use for us in actually splitting it up. Also, some of the things that might seem for juniors are things where seniors might disagree, so I do want them to read them because I am pretty strict on these.

      The lack of reasons for a lot of the points in this document is to keep it somewhat brief. Good explanations for all the points would at least double the length of the document. When a new coder joins the team I go through this document together with them and give verbal explanations for a lot of these things, so they do get the explanations.

      Operator overloading and "so many other things" are mostly not mentioned because the document isn't intended to describe the entire language. We use operator overloading where applicable and we don't have any particular rules for it so it's not in the document.

      I won't go into full detail on the other things you mentioned, but it was really interesting to read your reply (although I disagree with half of it ;) ). Thanks for giving such an extensive reply! :)

      Delete
    2. I don’t understand why it would be a problem to have a longer document. With the right formatting it should be possible to read through just the guidelines while skipping the reasoning, if that’s what the reader wants. Having the reasoning in the document itself also scales way better in bigger teams. I would also not expect anyone who’s just starting out to remember all of that when you’re going through the guidelines with them. And finally it looks less like a “because I told you so” list and could invite feedback based on new insight that might change things for the better. (e.g. “We don’t do this because compiler X doesn’t support it” could change when compiler X suddenly does support it.)

      Speaking of inviting feedback, I would love to hear what you disagree with. At Two Tribes I always said that nothing in the Guide is set in stone and I tried to encourage feedback from everyone. Especially interns might be shy about such things otherwise.

      Delete