Developer guidelines

Doomsday Engine is a diverse project that is free and open-source. As such, one of many ways to give back to Doomsday is by contributing code.

Be it a bugfix, new feature, or even just cleanup, your work can help make Doomsday the most technically advanced Doom engine available today.

When working with multiple persons on a project, especially one with a large amount of code, such as Doomsday or the Linux kernel, it is imperative that there exist guidelines for that code which is submitted by collaborators such that the project can be approached and understood by all involved and third parties.

This document is meant to be a summary of both requirements and best practices for submitting code. It covers, to the extent deemed necessary various guidelines for submitting code, such as:

• Code formatting
• Language features
• Naming conventions
• Version control practices

If you, as a prospective contributor, should have any questions that you feel were unanswered by this document, you are encouraged to contact a project manager for answers, which may then be added to this document.

Doomsday is composed of various components, the majority of which are written in both C and C++. Doomsday aims to support the C99 and C++11 (C++0x) language sets respectively, with the caveat that the features used must be supported by MSVC 2013. New code units (as in files) should be written in C++, as it is currently the intention to migrate towards C++.

When writing or modifying C++ for Doomsday, the following guidelines must be adhered to.

Indentation should reflect scope such that code residing in a scope n times more restricted should be indented n levels. Spaces must be used instead of tab stops, and one indentation level must be 4 spaces wide.

The following code demonstrates indentation in accordance with the scope:

    class Something : public Thing 
    {
    public:
        // Implementation
 
        Something(string param)
            : Thing(param)
        { 
            // Do things
        }
 
    private:
        // Implementation details            
    };
 
    static void doSomething(bool predicate)
    {
        // Nested type
        // This is ideally at the top of the method. Use your judgement.
        struct NestedStructure
        {
            // Members
        };
 
        { // Nested scope block
            doSomethingOther();
        }
 
        if (predicate)
        {
            doSomething();
        } 
    }
 
    { // Scope block
        int scopedInt = 2;
        doSomethingMore(scopedInt);
    } // New line

As far as switches go, it must be indented in a readable manner. How this is done is up to the writer. The following is an example of a well-indented switch.

    switch (predicate)
    {
    case CASE_1:
        doSomething();
        break;
 
    case CASE_SCOPED: { // Scope block
            doSomething();
        }
        break; // Break resides outside the scope block. 
 
    case CASE_FALL_THROUGH_CODE:
        doSomething();
    case CASE_FALL_THROUGH:
    default:
        break;
    }

Doomsday, for the most part, follows Allman style brace formatting.

Braces should not reside at a level of indentation less than that of the declaration to which they belong. They must always begin on the line proceeding the statement to which they correspond.

Once more, in regards to switches, should you need a scoped case, you may either place the beginning of the scope block immediately proceeding the case (case THING: {), or following the case statement.

You should use a space to separate the predicate following an if, while, for and the like. For long predicates, the predicate may be broken at an appropriate place, and the rest put on the following line. Be sure that multiline predicates are aligned sensibly — use your intuition.

    if (option && option2 && (compound && option)
        || nextLineOption)
    {
        // do something
    }

In regards to other flow control keywords, such as break or continue you should avoid nesting in order to create more readable code. If you have excessive return points in a void method, in the interest of readability you should look at reorganizing to use conditionals, as the compiler will optimize it all the same (in most cases).

    void tooManyReturns()
    {
        // some code up here
 
        if (!thing1) return;
        if (!thing2) return;
        if (!thing3 == VALUE) return;
 
        // more code here
    }
 
    // This can be rewritten as
 
    void conditionalExecution()
    {
        // some code up here
 
        if (thing1 && thin2 && (thing3 == VALUE))
        { 
            // more code here
        }
    }

Regarding conditional flow control, such as if and while avoid using a predicate like this:

    int that = 10;
 
    if (that)
    {
        // code 
    }
 
    // Better rewritten as 
 
    if (that != 0)
    {
        // code
    }
 
    // Or, in some cases `that` should always be positive
 
    if (that > 0)
    {
        // code
    }

This allows a reader to infer that it is neither a boolean value or a pointer, but rather a number or something which overrides one of the pictured operators.

Regarding assigment or field declaration in predicates, this practice should be avoided wherever possible. If the need for this is apparent, then it should be strictly for a field declared in the predicate. For instance, the following code demonstrates proper assignment in a predicate:

    if (Thing *thingPointer = getThingPointer())
    {
        // do things with thingPointer
    }

Whereas the following code demonstrates cases where this behavior is either unecessary, or inappropriate.

    // Innapropriate use: 
 
    Thing *thingPointer = getThingPointer();
    // do stuff with thingPointer
    if (thingPointer = getAnotherThingPointer())
    {
        // Don't do this!
        // Erroneous field reuse should be avoided at all costs
    }
 
    // do more with thingPointer 
    // Don't do the above either, 
    // If a field is needed outside the scope of the
    // conditional expressions branch(es), then it should
    // not be modified within the expressions predicate 
 
    // Unecessary use 
    Thing *thingPointer = getThingPointer();
    if (Foo *fooPointer = doBar()) 
    {
        // use thing pointer in here
        // use foo pointer in here 
    }    
 
    // don't use thingPointer past this point

In the former of the above two situations, placing both declarations in a scope block along with the if statement would be preferrable.

In summary, the predicate should never modify an existing field, and should only declare one if it is syntactically clear and leads to more idiomatic code. Otherwise, alternative approaches should be preferred, especially if they do not pollute the parent scope.

goto should be avoided at all costs, as it is both hard to debug and may cause unexpected behavior. If you encounter a method using goto, see if it can be refactored out. The same applies for other such similar operators.

As far as preprocessor formatting goes, it is preferable that the pound (#) symbol be placed on column 0, however, directives may reside at an indentation level with respect to surrounding code.

    #if condition
    #   if condtion2
    #       define MACRO(x) thing1(x);
    #   else
    #       define MACRO(x)
    #   endif
    #endif

It is advisible that the preprocessor not be used to define constants when they could be defined natively using const or static const This is advantageous as native constants respect scope, and may furthermore be debugged more easily.

We recommend that C++ classes use the PIMPL pattern as it encourages correctly separating the public and internal parts of a class, and makes it possible to retain ABI compatibility even when the implementation of the class changes. You can find macros in de/libcore.h that make it more convenient to work with private implementations.

Example of a class declaration in a .h file:

class MyClass
{
public:
    MyClass();
 
private:
    DENG2_PRIVATE(d)
};

The implementation in a .cpp file:

DENG2_PIMPL(MyClass)
{
    // private members
    int a = 0;
 
    Impl(Public *i) : Base(i)
    {
        // `Public` is a typedef for `MyClass`, `Base::self` is a reference to `*i`
    }
 
    ~Impl()
    {
        // destruct
    }
};
 
MyClass::MyClass() : d(new Impl(this))
{}
 
// ~MyClass not necessary because `d` will be automatically deleted.

In class definitions, headers, and source units it is advisable that methods be ordered primarily with respect to visibility – e.g. static methods at the top of the file, and secondly with respect to concern. Type declarations and constants isolated to a single source unit may be placed either at the beginning, or within the narrowest scope possible.

Classes, Enumerations, Structures, and Unions declared in C++ units should be named using UpperCamelCase with an appropriate name. Local variables, methods, and method parameters should be named using lowerCamelCase. It is preferrable that method names contain a descriptive verb (e.g. joinServer or fetchDescriptor). We generally follow a Qt-like naming convention, where getters do no have a verb in the method name.

• int someValue()
• void setSomeValue(int)

An underscore prefix is used for private members:

• int _someValue; is a private member in a class
• int someValue; is a public member (including members in a PIMPL struct)

Constants, regardless of scope, and macros, should be named using UPPER_SNAKE_CASE. There should be no need for use of lower_snake_case or Mixed_snake_case in C++.

const must be used consistently in your classes: getters and all other members that do not modify the (effective) state of the class should always be const. Prefer to use const references to pass arguments to methods.

The const keyword should be placed after type identifiers:

String const &str static int const CONSTANT = 1;

C code should be formatted similarly to C++, with some exceptions.

Enumerations, Structures, and Unions, being types, should be named using lower_snake_case with a suffixed _t. All other symbol names should be formatted in accordance with the aforementioned C++ guidelines.

Comments in Doxygen format describing the purpose, usage, parameters, return values, errors, and other information about functions is encouraged everywhere. Place Doxygen documentation about methods/functions in the header files. The api_documentation is generated automatically by the autobuilder once per day.

For example:

/**
 * Does magical things to foobars.
 *
 * @param fooBar  Foobar to magic.
 *
 * @return  @c true, if the foobar is magicked.
 *          Otherwise @c false.
 */
bool Foobar_DoMagic(foobar_t *fooBar);

Doomsday uses Git and GitHub. In order to submit code, you must have ordered it in logic units, known as commits, and pushed it to a fork of the official Doomsday Engine repository. Once you are ready to have your work reintegrated, open a pull request with a description of your work.

This section contains guidelines to which you should adhere in order to insure that your work can be integrated without issue.

When writing a commit message, you should keep in mind that the intended audience for the message is not only other developers but also the end users who read build reports to see what has changed in each build. Therefore, the message should contain enough detail to be understood on its own. If nothing else, the commit message should explain motivations behind the changes (if they are not obvious, like a bugfix, cleanup, or referencing a tracker issue).

Your commit message should contain tags that describe the type of commit (e.g. Fixed) and those components which have been affected by the commit. You can see many examples of the tags by examining the Git history. The commit tags serve a couple of purposes:

• At-a-glance summary of what the commit is about.
• Group commits in the build reports generated by the autobuilder. Untagged commits will appear under a “Miscellaneous” heading in the build reports.
• Index the commits in the codex.

While the tags are not checked for validity, you should generally use one of the commonly used tags as listed in Codex: Tags by size. The most common tags are: Fixed, Refactor, Added, and Cleanup.

Doomsday Engine uses an external issue tracker to manage issues. The tracker is integrated with the git repository, and will parse commit messages for information about commits. Should you need to reference an issue ID in your commit message, you may do so by placing the text IssueID #0 in your commit message, where 0 is your issue ID. When your commit is merged, the issue tracker will attach your commit to the relevant issue.

An example commit might look like this:

Fixed|libcommon|Doom: Foobar is no longer crashing the game

Turns out the foobar was accessing a null pointer.

IssueID #666

Avoid keeping work-in-progress commits in your commit history. If you need to save your work before checking out a commit or branch, you may use git stash to store your progress rather than a commit, or keep your work-in-progress commits in a private work branch and rebase/squash them later.

Commits should generally represent the completion of a goal. As such, it is preferable that they compile, but not critical, though it should be noted that whether or not a desired commit may be built successfully can and will effect the change of getting your pull request merged.

When working on multiple tasks at once, you can work on feature branches should you wish to avoid cross-contaminating code. A feature branch is a branch from the working master that contains a set of changes specific to one task or feature. An added advantage of feature branches is the ability to open a pull request specifically with that feature branch's changes.

In order to insure the smoothest merge, among other things, try to pull from your base often (e.g. origin master). If you must make a merge commit (without conflicts), not that it will be ignored by the tracker. If you have to resolve conflicts, it is your decision whether they are trivial enough (i.e. some newlines in a comment) to be lumped in to a commit to the likes of Merge origin/master into … or if a more descriptive commit is needed.

Failure to integrate regularly means that, should you choose to integrate changes from upstream in to your working base, you can run in to unpleasant merge conflicts; alternatively, should a project manager wish to merge your work, they might run in to similar merge conflicts.

Always write appropriate API documentation for your class APIs using Doxygen.

While/after implementing a feature, document it here in the wiki:

• User/gameplay features should be categorized into .
• Modding/map author features should be categorized into .
• Technical/internal/API features should be categorized into .

Certain documentation is processed with Amethyst. This includes the readme included in distribution packages, Unix manual pages, and runtime console commands and variables.