Coding Style Guidelines and Git/GitHub Workflow

About the actual programming of the game.

Coding Style Guidelines and Git/GitHub Workflow

Postby mrout » Fri Aug 16, 2013 1:02 pm

Okay folks, just announcing the coding style guidelines. So as to avoid a flame war or petty arguments, I've decided the easiest thing for me to do is just to decide on some fairly standard guidelines. Feel free to comment, but only on the substance of the guidelines, please. No arguments about tab widths, tabs vs. spaces, brace positions or indentation will be tolerated.

TL;DR: CONSISTENCY, CONST-CORRECTNESS AND EXCEPTION SAFETY

Basic Coding Style:

  • EDIT: Lines may be no longer than 80 characters, not including the newline character.
  • EDIT: Use lowercase with underscores, except for template parameters, which should be either T (for generic type), or T1, T2, etc. (for several generic types), F (for a function type) OR a descriptive PascalCase name (e.g. InputIterator).
    Code: Select all
    namespace foo {
        class point2f {
        public:
            float x, y;
            point2f(float x, float y) : x(x), y(y) {}
        };

        template <typename T>
        T foo(T t);   
    }
  • EDIT: Liberally use static_assert to enforce compile-time safety checks on template parameters (the examples here are fantastic - much better to get a static assertion than some horrific error message. One day we'll get concepts.. one day.
  • EDIT: Liberally use assert to sanity check your code. It is not error handling.
  • EDIT: Use meaningful variable, function and class names, but keep them reasonably succinct - words like Data, Processor, Visitor etc. are usually superfluous. If you've got several classes like blah1_visitor, blah2_visitor, blah3_visitor, etc. all in one namespace, sub-namespacing them to visitors::blah1, visitors::blah2, visitors::blah3 is usually cleaner.
  • Use meaningful comments that illustrate the meaning of code. Explain why the code does what it does. We can read code, we don't need really obvious comments like the one below.
    Code: Select all
    i++; // increment i
  • 4 space indentation. No tabs.
  • Newlines before braces for functions, but nowhere else.
    Code: Select all
    class A {
        int x;
    };

    int main(int argc, char **argv)
    {
        if (argc == 1) {
            exit(-1);
        }
        printf("%s", argv[1]);
        exit(0);
    }
  • Don't indent public, private, protected.
    Code: Select all
    class A : B {
    public:
        A(int a, int b, int c) : a(a), b(b), c(c) {}
        A() : A(0, 0, 0) {}
    private:
        int a, b, c;
    };
  • Use this style of switch. Always include a default, even if you think it's unnecessary.
    Code: Select all
    switch (x) {
        case 'a':
            // blah
        case 'b':
        case 'c':
            // blah
            break;
        default:
            // blah
    }
  • Use this style of if-elseif-else.
    Code: Select all
    if (a == b) {
        // blah
    } else if (a > b) {
        // blah
    } else {
        // blah
    }
  • EDIT: Prefix private members with an _.
  • EDIT: Write getters/setters in this fashion (inline):
    Code: Select all
    class foo {
    int _x;
    public:
        int x() const { return _x; }
        void x(int value) { _x = x }
    };
  • EDIT: Always initialise variables.
  • Destructors should always be virtual!
  • Don't use macros unless there's no other way to do it. There's nearly always another way to do it.
  • Don't be afraid to use the standard library. For the most part it's extremely powerful and well designed (with some exceptions), and it sure beats reinventing the wheel.
  • Raw pointers must not own resources. Ever. Learn RAII. Now. Read [url=klmr.me/slides/modern-cpp.pdf]this small presentation[/url]. Now. If you don't, your pull requests will be declined, because I hate memory leaks.
  • Learn how to maintain exception safety. Then maintain it.
  • It bears repeating: don't be afraid to use the standard library, including both the C standard library and the C++-specific parts.
  • Don't waste resources doing unnecessary copying.
  • Don't just fiddle with things until they work. Understand the code, understand the language.
  • EDIT: Never use using namespace std;

Each commit should be logically separate.
Each commit should do one and only one of the following:
  • Fix a single bug. The bug must be in the GitHub issues tracker and tagged as 'bug', along with optionally other tags. The commit message must include the text "Fixes #xx", where #xx is the issue number. resolve, resolves, resolved, close, closes, closed, fix and fixed are all equally valid as well. For more information, see here.
  • Add a single feature. This feature must be in the GitHub issues tracker and tagged as 'feature', along with optionally other tags. The commit message must include the text "Closes #xx", where #xx is the issue number. closes, close, closed are all equally valid. resolve, resolves, resolved, fix, fixes and fixed are accepted, but not preferred (they imply fixing or resolving of a bug, not addition of a feature).
  • If a single feature is too large to go in a single commit, it should go into a feature branch. Each commit in that branch must leave the project in a working state. The pull request message must include the text "Closes #xx", where #xx is the issue number. closes, close, closed are all equally valid. resolve, resolves, resolved, fix, fixes and fixed are accepted, but not preferred (they imply fixing or resolving of a bug, not addition of a feature).

You won't necessarily work like this on your local machine, but you should squash commits together with rebase until they're right. But DO NOT ALTER PUBLIC HISTORY! Once a commit has been pushed, it is public, and it should not be altered. Altering public history wreaks havoc with version control.

Note: Lack of documentation/commenting/style is considered a bug.

When we establish a test suite, run it before you commit. No pull request will be accepted if any commit in it fails the test suite, not just the final commit. Every commit must leave the project in a working state.

If something isn't stated here, or is unclear, make your decision based on these guidelines:

  • Local consistency. Try to make sure code is consistent with its surrounding code.
  • General readability. If you have to choose between two styles, choose the one that you think will be readable to the most people.
  • Choose something and carry on. If all else fails, it can always be changed later in code review.
Last edited by mrout on Thu Aug 29, 2013 1:38 pm, edited 8 times in total.
Reason: Clarified what I added.
mrout
 
Posts: 731
Joined: Mon Aug 12, 2013 10:49 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby thomas9459 » Fri Aug 16, 2013 3:13 pm

Are we going to use C++11? Boost?
thomas9459
 
Posts: 101
Joined: Thu Aug 15, 2013 6:18 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby chessmaster42 » Fri Aug 16, 2013 3:35 pm

Looks great! We should also establish code documentation guidelines around commenting. Nothing worse than poorly, or not at all, commented code. We don't need anything fancy, just some basics so the whole code team could look at a single function and know how it all works by reading the comments.

Personally, I comment code not for the sake of others but for my own sake to keep track of how all the pieces fit together and how the code flows.
chessmaster42
 
Posts: 11
Joined: Thu Aug 15, 2013 4:44 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby eat_pb » Fri Aug 16, 2013 4:38 pm

Correct me if I'm wrong, but based on that slide deck you've posted it seems like you're suggested that nothing should ever be allocated onto the heap.

Please tell me I'm reading that wrong.


EDIT:

Ok shared_ptr is acceptable. Panic over.
User avatar
eat_pb
 
Posts: 10
Joined: Tue Aug 13, 2013 11:56 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby avh » Fri Aug 16, 2013 8:05 pm

thomas9459 wrote:Are we going to use C++11? Boost?


I am quite sure smart pointers where only added in C++11, so that is a given, since the pointer template types used in the presentation are all in the std namespace.
avh
 
Posts: 2
Joined: Mon Aug 12, 2013 2:20 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby danix111 » Fri Aug 16, 2013 8:21 pm

thomas9459 wrote:Are we going to use C++11?

mrout said yes.
User avatar
danix111
 
Posts: 61
Joined: Mon Aug 12, 2013 2:05 pm
Location: Gdynia, Poland

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby Pseudonym » Sat Aug 17, 2013 2:27 am

I have one suggestion.

Code: Select all
switch (x) {
case 'a':
    // blah
case 'b':
case 'c':
    // blah
    break;
default:
    // blah
}


I often use braces for each case, because it makes C++ blocks more obvious (RAII etc). This unfortunately clashes with the above brace style:

Code: Select all
switch (x) {
case 'a': {
    some_raii_resource acquire;
    some_code();
    break;
}
default:
}


This seems better to me:

Code: Select all
switch (x) {
    case 'a': {
        some_raii_resource acquire;
        some_code();
        break;
    }
    default:
}
Pseudonym
 
Posts: 129
Joined: Tue Aug 13, 2013 3:54 am

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby Zardoz » Sat Aug 17, 2013 7:15 am

danix111 wrote:
thomas9459 wrote:Are we going to use C++11?

mrout said yes.

So hello to std::shared_ptr , not ?
Yep, I have a blog : http://zardoz.es
Emulator DCPU-16 VM
User avatar
Zardoz
 
Posts: 359
Joined: Mon Aug 12, 2013 8:54 pm
Location: Spain

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby mrout » Sat Aug 17, 2013 9:30 am

Re: C++11 and Boost:

Some Boost will be acceptable, but that can be evaluated on a case-by-case basis. It's a wonderful library overall, but there are certainly parts that aren't as good as others.

C++11 is definitely what we're using, yes. Even Visual Studio supports much of the standard now.

No C++14 features will be allowed unless they're fully supported in the latest stable versions of clang, visual studio and g++.

@Pseudonym:

Hmm, that's an interesting style. The only issue I have with it is that it could confuse people if they saw something like this:

Code: Select all
switch (n) {
    case 0: {
        puts("Nothing available.");
    }
    case 1: {
        puts("One item available.");
    }
    default: {
        puts("Multiple items available.");
    }
}


The cases definitely seem more separate there, and it could lead to bugs, perhaps?

Code: Select all
switch (n) {
case 0:
    puts("Nothing available.");
case 1:
    puts("One item available.");
default:
    puts("Multiple items available.");
}


In the second example, I can definitely tell that it's missing two breaks much more easily than I can in the first. Am I just weird like that?

But there's certainly nothing wrong with changing to the style with more indentation. I'm used to the style without it, because I usually use 8 space tabs, and indenting twice within a switch gets a bit much with 8 space tabs, haha. I've changed the OP.
mrout
 
Posts: 731
Joined: Mon Aug 12, 2013 10:49 pm

Re: Coding Style Guidelines and Git/GitHub Workflow

Postby Zardoz » Sat Aug 17, 2013 12:17 pm

For Documentation.... doxygen ?
Yep, I have a blog : http://zardoz.es
Emulator DCPU-16 VM
User avatar
Zardoz
 
Posts: 359
Joined: Mon Aug 12, 2013 8:54 pm
Location: Spain

Next

Return to Code

Who is online

Users browsing this forum: No registered users and 2 guests

cron