opentomb / OpenTomb

An open-source Tomb Raider 1-5 engine remake
http://opentomb.github.io/
GNU Lesser General Public License v3.0
1.38k stars 143 forks source link

Coding conventions #180

Open Lwmte opened 9 years ago

Lwmte commented 9 years ago

Please, do not write like this:

for(blah) {
    foo
    bar
}

Write it like this:

for(blah)
{
    foo;
    bar;
} 

However, if you have only one string enclosed (i. e. only foo), you can do it like this:

for(blah)
    { foo; }

The general rule is: if you have multiple lines in enclosed in brackets, then both beginning and ending bracket should be on its own new line. If you have only one line enclosed, you can merge it with both brackets.

For if...then conditions - if result is one-string only and short enough to fit in same string with condition, then write it like this:

if(blah) foo;

If result won't fit (i. e. 80 characters limit is very well passed), put it on a new tabbed string:

if(blah-blah-blah-blah)
     foo-foo-foo-foo;

If result is multiple-string, always treat it as multi-line described above, like this:

if(blah)
{
    foo;
    bar;
}
Lwmte commented 9 years ago

In a case switch, whole switch body must be one tab further opening and closing brackets, and additionally, each case body must be one tab further case's own label. Like this:

switch(blah)
{
    case foo:
        foo-foo-foo;
        foo-foo-foo-foo;
        break;
    case bar:
        bar-bar;
        bar-bar-bar;
        break;
}

If you need to enclose particular case into bracket block, put body one tab further:

switch(blah)
{
    case foo:
        {
            btScalar bar;
            bar = foo-foo-foo;
            foo-foo-foo-foo = bar;
            break;
        }
    case bar:
        bar-bar;
        bar-bar-bar;
        break;
}
vvs- commented 9 years ago

Don't count indents as a tabs, every editor has its own settings. At least there should be a convention how much spaces are in each tabulation. And should editor insert a tab character 0x09 or fill it with spaces instead?

Lwmte commented 9 years ago

Yes, forgot about that. No tab symbols, each "tab" must be 4 spaces.

When you have numerous assignment operations in a row, and/or their names are somewhat of equal length, and/or their data types is similar, then please align "left" and "right" parts of the assignment, like this:

    btScalar foo      = 1.0;
    btScalar bar-bar  = 1.0;
    btScalar foo-o-o  = 1.0;

Or:

    bar      = foo-foo-foo;
    foo-o    = bar;
    bar-r-r  = foo;

In case if you have pointers defined along with "normal" variables, then asterisk symbol must be placed instead of last tab's space symbol (this also applies for class declarations and/or implementations), like this:

    bar     = foo-foo;
   *foo     = &bar-bar;

Of course, if one's left part is way longer than another one's left part, there's no need in any such alignment, so you can leave it like this:

    *foo-foo = &bar-foo;
    bar->foo-foo-foo.blah-blah-bar-foo = 1.0;
    foo-bar = 1.0;
Lwmte commented 9 years ago

If code block gets too large, it's recommended to split it up in several "sub-blocks" with empty lines, taking each sub-block meaning into account. Like this:

    foo = foo-foo + foo-foo-foo;
    if(foo) foo-foo-foo-foo-foo;
    foo-foo = foo;

    bar = (bar-bar > bar)?(bar-bar):(0);
    bar-bar-bar = bar - bar-bar;
vvs- commented 9 years ago

There are different tools to beautify code, like indent, astyle or clang-format. It can be even included in CMake project and run automatically.

Lwmte commented 9 years ago

I note that as a remnant from "old" OpenTomb style, we have LOTS of "pascal-styled" or "basic-styled" variable declarations in the beginning of the function or code block. Like this:

    int foo, bar = 0;
    btScalar blah;
    ...
    blah = foo;
    foo = bar;
    ...

Please, get rid of this evil everywhere you see it. I again cite Google Style Guide here:

Place a function's variables in the narrowest scope possible, and initialize variables in the declaration.

It was suggested by @Cochrane earlier, but I was too dumb back then to take it into consideration.

stohrendorf commented 9 years ago

I have two suggestions:

  1. Never write "one-liners", i.e. if(....) return; or such things. The statement after the if can be easily missed, but at least I find it very confusing.
  2. Avoid excessive use of parenthesis. I have seen a lot of code like if((a==b)&&(a!=0)) c[(i+1)]=d;. Everytime I see this, I need to stop and carefully read it, because there might be necessary parenthesis. Spaces (or even double spaces) can usually do the same trick.
T4Larson commented 9 years ago

What's the policy regarding the "m_" prefix for class members? Google Style Guide suggests not to use hungarian notation, but often the member naming rule is preferred for readability (or without IDE highlighting) in bigger classes.

I'm fine with both, my rule of thumb proposal would be not to overdo it, though: POD-structures or simple/elegant classes don't need m_-noise, imo, but I also see the point of quicker distinction from local vars in more complex methods.

stohrendorf commented 9 years ago

I can only tell that most people I know accept a few simple rules regarding this: 1) Use classes with m_ member prefixes that contain logic/data processing/program state/etc., and make those members private. 2) Use structs without the prefix for data transfer structs, e.g. structs that are only needed when dealing with files or raw memory directly, and give these structs only the very necessary functions to help transferring the data. (Side note: the loader code isn't conforming to this; it should sometime be refactored to not use the old legacy TR structs, but instead use OT-specific encapsulated classes.) 3) Avoid hungarian notation. C++ rules for implicit casts are stricter than C ones (e.g., you can't implicitly cast a void* to an int* like in C), so a need for the variable type in the variable name is obsolete. If it compiles without explicit casts, then there are only logical errors left.

Of course, there are exceptions to these rules, but usually they do not occur that often.