How to be a horrible programmer: OpenXcom coding guidelines

[SupSuper]

Apparently some people wanna be like me (the horror!), so here you go. The guidelines I follow when I'm writing OpenXcom. Except when I don't. In the end, consistency and readability rules.

Naming:

• In general, CamelCase, with words only separated by uppercases.
• ClassNames start with an uppercase.
• anyOther variable or method starts with a lowercase.
• _privateMembers start with an underscore.
• DEFINES_ENUMS_CONSTANTS are all uppercase with words separated by underscores.

Spacing:

• In general, space things like you were writing a sentence. Don't let the code get squished up (eg. "a = 5 * b(4, 3) + -3 / 80" vs "a=5*b(4,3)+-3/80").
• Spaces after commas, semicolons, variables, operators, etc.
• No need for spaces after opening parenthesis.
• Use linebreaks to keep large unrelated blocks of code separate.
• Use tabs for indenting code blocks between brackets, one tab per level.

Readability:

• Keep names nice and readable, specially if they're public, remember there's auto-complete. Abbreviations are ok occasionally.
• Short names are ok if it's just a minor variable or you'll be writing it a lot (eg. iterators called i, j, k).
• Use static consts for "magic numbers" that are hard to figure out on their own. Don't go replacing 0 with ZERO, but don't go leaving something like 8443.90438 either.
• Keep long operations intact if it improves readability, the compiler will automatically calculate them anyways. (eg. "margin = (60 + 2 * 12 + 25) / 2)" vs "margin = 54.5").
• Use comments to explain long boring code blocks.
• Use https:// for comments even if they're multi-line, it keeps them from getting in the way when you comment out code blocks with /* */.
• Split a comment into multiple lines if it gets too long. Same should apply to code, although I don't do it much.
• Avoid copy-pasting, avoid incredibly long code blocks, use spacing and comments and even more functions to keep everything neat.
• Keep the code clean, no useless includes, declarations, etc.

Documentation:

• Doxygen is used for OpenXcom, with the convention of https:/// for single-line documentation, /** for multi-line documentation and @ for attributes (@param, @return, etc).
• Classes should have a multi-line description of their purpose in their header declaration.
• Methods should have a one-sentence brief description (https:///) in their header declaration and a multi-line full description (/**) in their definition.
• Don't forget to document method parameters and return values.
• Write in proper English. Please. :P
• Always end your sentences with a dot. This is usually not relevant for comments, but I think Doxygen uses these to better tidy up the stuff.
• Remember Doxygen will warn you about errors / missing documentation so run it from time to time.

Misc:

• In general, OOP conventions apply.
• Keep each class in its own equally-named header/source file. Keeping related enums/structs in the same file is fine.
• Always use brackets to start a block, even if it's just one line. Yes yes I know it's so many space wasted for one line, but eventually inevitably you'll slip up and forget to add them when you extend it. I know I have.
  
• Opening/closing brackets always lined up in their own lines.
• Keep variable declarations where relevant, don't leave them all at the start of code blocks ala ANSI-C, it makes it harder to tell their purpose.
• When declaring pointers, keep the * on the side of the variable name. This prevents common mistakes like "int* a, b, c" (only a will be a pointer).
• Feel free to put multiple declarations in one line.
• No need to check for null pointers when deleting stuff.
• Don't use NULL, it's not actually in the C++ standard and will be later replaced with nullptr by C++0x.
• Use iterators for looping through STL containers (vectors, maps, etc.).
• Use pre-increment/decrement (++i and --i) instead of post-increment/decrement (i++ and i--) except when it produces a different result.
• Use const on methods that don't change its class (eg. getter methods).
• Don't pass objects to functions by copy, use pointers or references (eg. "const std::string &s" instead of just "std::string s").
• Remember the C++ standard requires an empty linebreak on the end of every file.
• Avoid making generic utility classes or functions, keep everything in the class it's relevant to (make more classes if you have to, can never have enough!).
• Use common sense!

No religious arguments. Need any more, just ask.

[DaiShiva]

What is your stance on typedefs? Mostly in relating to the STL containers.

[SupSuper]

What is your stance on typedefs? Mostly in relating to the STL containers.

Personally I don't bother, I abuse STL containers so much I'd probably end up with as many typedefs as I have class variables. I use them for ugly stuff like function pointers though.

[Fenyő]

@SupSuper:
Long time i want to suggest, please append the coding guidelines with this:

When comparing a variable with an expression, put the variable to the RIGHT, like this:
"if (1 == variable) ..."
INSTEAD OF
"if (variable == 1) ..."

You may ask: WHY.
I'm sure you know, there are some occasions, when the programmer want to put the operator ==, but instead he types = accidentally. And of course he does not notice it, and of course the code compiles (since it has a meaning), and of course it won't work as the programmer wanted it to. And he hunts the bug for hours of course.

By swapping the variable with the expression, this error is indicated immediately, because the code does not even compile!
Since "if (1 = variable) ..." does not make sense.

And since the guidelines contain a "++i instead of i++" rule, i think my suggestion has at least as much importance than ++i. (not performance, but safety property)

[djemon_lda]

@Fenyő:
I don't agree with your postulate. what if you compare two variables? doesn't change virtually nothing in that case if you make the ==/= mistake, but another thing WILL happen. If a programmer get's used to this rvalue on the leftside idiom, then he will be careless about it and will incorporate errors with ==/= when comparing two variables. this is a poisonous gift because of that. I personally haven't made such a mistake on any point - EVEN when I was porting from pascal to c++ on university ( not that in pascal, you used a single = for comparison, so I should have a grander potential to make this type of error, yet I never did - just because I was paying attention to my work ).

@SupSuper:
I have some suggestions if you'd like to consider them:

Use comments to explain long boring code blocks.

Why not considering to extract a method with a meaningful name in such situation? Such a method is easier to test and easier to debug, and extracting methods allows later on to see if "doing this is stil a responsibility of this class, or maybe we need a new one?" Also inlining code is more optimal only in certain situations, overall it is ok for performance to make a function call, because when bloating code, you put a lot of stuff into the processors 1st line cache, hence having less place in the 1L cache for the game's data, and increasing the cache misses rate, which in turn are very bad for performance.

Feel free to put multiple declarations in one line.

When declaring pointers, keep the * on the side of the variable name. This prevents common mistakes like "int* a, b, c" (only a will be a pointer).

I disagree with the first one, and the reasoning I have nullifies the second one. I propose to make it: "Never put multiple declarations. Each declaration has it's own line".

It is easier to read:

Code: [Select]

int a;
int* b;
int c;
int *d;
than:

Code: [Select]

int a,*b,c,*d;
The reasoning behind int* a; being better than int *a; is that int* gives a stronger intuition of a type - pointers are also distinct types after all, and you can also make references to pointers like int*& a; (which in turn are types as well) or pointers to pointers as well.

Another thing is padding. Making many fields make's you lose count easily - if you have five bools in a row, and then an int then you expose your classes for problems with padding memory -> objects take more memory than it actually is required -> you get more cache taken, but not usable -> performance goes down.

Use pre-increment/decrement (++i and --i) instead of post-increment/decrement (i++ and i--) except when it produces a different result.

Assembly code generated for ++i and --i for an int/long/char has equal complexity ( both generate 3 assembler instructions ) - this is valid only if the operator pre/post incrementation/decrementation is defined for big objects, but it is discouraged. Overloading of the ++ or -- for types other than BigInts etc is a bad practice and destroys readibility of the code.

Avoid making generic utility classes or functions, keep everything in the class it's relevant to (make more classes if you have to, can never have enough!).

In general it is a good practice to make generic stuff. If you make a private method, then it is worth considering if this functionality could be use for other classes.
If the answer is 'yes' it's good extract that method to another class as a static method (perhaps templated) and reuse the code. Less code == less places you need to maintain, test and track bugs in.

Use common sense!

It isn't worth to overestimate this

What I'd add is also to try to use polimorphic classes, to reduce the hordes of if/else statements and switches. The generated assembler code for a polimorphic method call is just a single assembler instruction more, which makes it more efficient than using if/else/switch because there is also no forking and branch prediction incorporated.

Btw, were you guys doing code reviews before commiting the changes to the repository ? it's worth doing, believe me.

[SupSuper]

Bare in mind that these guidelines were written years ago just to convey my style and try to keep it uniform (which didn't work ), so they might be dated.

@SupSuper:
I have some suggestions if you'd like to consider them:

Why not considering to extract a method with a meaningful name in such situation? Such a method is easier to test and easier to debug, and extracting methods allows later on to see if "doing this is stil a responsibility of this class, or maybe we need a new one?" Also inlining code is more optimal only in certain situations, overall it is ok for performance to make a function call, because when bloating code, you put a lot of stuff into the processors 1st line cache, hence having less place in the 1L cache for the game's data, and increasing the cache misses rate, which in turn are very bad for performance.

Sure, I didn't mean to put comments over refactoring, since I also wrote this:

Avoid copy-pasting, avoid incredibly long code blocks, use spacing and comments and even more functions to keep everything neat.

What I meant by that is sometimes you just have code that's just hairy and no amount of refactoring will make it look good (like really mathy stuff, edge cases, etc.) and it's good to have a comment to clarify that stuff. Programmers have a sixth-sense when code just looks "off" and it's good to clarify when it's intentional.

In general this is how I go about comments:
- By default code should be self-documenting. So clear names for functions, variables, etc.
- Use the one-line brief comment (in the header) to sum up what the function is for. This is normally unnecessary but it also shows up on Auto-Complete which looks cool.
- Use the descriptive comment (in the source) to sum up how the function does it, for people diving into it.
- As a last resort, use a regular in-line comment to explain any particularly weird code line.

I disagree with the first one, and the reasoning I have nullifies the second one. I propose to make it: "Never put multiple declarations. Each declaration has it's own line".

It is easier to read:

Code: [Select]

int a;
int* b;
int c;
int *d;
than:

Code: [Select]

int a,*b,c,*d;
The reasoning behind int* a; being better than int *a; is that int* gives a stronger intuition of a type - pointers are also distinct types after all, and you can also make references to pointers like int*& a; (which in turn are types as well) or pointers to pointers as well.

Another thing is padding. Making many fields make's you lose count easily - if you have five bools in a row, and then an int then you expose your classes for problems with padding memory -> objects take more memory than it actually is required -> you get more cache taken, but not usable -> performance goes down.

Ehhhh... I see your point, and I don't think there's ever a case where pointers/references are mixed all in one line (because that's just ugly).
But I hate having variable declarations all in separate lines. All in one line is ugly too, I think they are are two extremes. I prefer a good mix. Like if I have a bunch of related or similar vars, I would do "int x, y, width, height;". The grouping implies they make sense close together. I dunno it just seems neater to me, I wouldn't wanna find an "int x" at the top and then track down the "int y" at the bottom.

Assembly code generated for ++i and --i for an int/long/char has equal complexity ( both generate 3 assembler instructions ) - this is valid only if the operator pre/post incrementation/decrementation is defined for big objects, but it is discouraged. Overloading of the ++ or -- for types other than BigInts etc is a bad practice and destroys readibility of the code.

It's mainly for consistency with stuff like STL iterators where ++i and --i is slightly faster than i++ and i--.
I personally think i++ and i-- looks better though, and this whole pre/post is a whole bunch of nonsense and there should be just one type problem solved.

In general it is a good practice to make generic stuff. If you make a private method, then it is worth considering if this functionality could be use for other classes.
If the answer is 'yes' it's good extract that method to another class as a static method (perhaps templated) and reuse the code. Less code == less places you need to maintain, test and track bugs in.

I don't mean to knock generic code, just generic classes, like putting it all in a big Utility class or whatever.

What I'd add is also to try to use polimorphic classes, to reduce the hordes of if/else statements and switches. The generated assembler code for a polimorphic method call is just a single assembler instruction more, which makes it more efficient than using if/else/switch because there is also no forking and branch prediction incorporated.

We don't avoid polymorphic classes out of performance worries, believe me.
I'd say it's just convenience? Game concepts rarely fit nicely into "polymorphic objects", since there can be a lot of confusing and crazy variations of special behavior and so on. It's easier to work with flat value-based classes in that regard, even if it makes for if-else-nests, and they're easier to externalize for modding.
I'm not saying this is good, but it works. In retrospect a component-based architecture could've been more robust, but it could've also scared away everyone that doesn't have a clue what I'm on about. Software architecture is tricky, specially with an open-source project.

Btw, were you guys doing code reviews before commiting the changes to the repository ? it's worth doing, believe me.

We usually review any pull requests that come from contributors before merging them into the codebase. But within the lead team (SupSuper, Daiky, Warboy), we have very disparate schedules, tasks and lives (which is why pull requests are often left hanging, sorry ), so we just trust each other to code properly and discuss if we have any problems or questions when we get together.

[djemon_lda]

I would do "int x, y, width, height;". The grouping implies they make sense close together

then why not just making a class with all public fields named Rectangle ? the class that used those coordinates and dimensions would also have thinner code, and be easier to maintain/debug and this wouldn't affect the performance at all, you only would have to place it as a value field(not a pointer or reference) and propose that it's methods would be inline - NO overhead at all

it could've also scared away everyone that doesn't have a clue what I'm on about.

That would actually be a very positive thing if I am to be honest

if/else nests are more error prone and out of experience I know that people do a lot worse in chaos - brain works perfectly when it can categorize everything and comprehend the processes in terms of categories. That is why code like:

for( Agent& agent : agents )
{
    agent.Update(timeDelta);
    agent.HandleMessages(messages);
    agent.ComputeAction();
}

with classes RangedAgent and MeleeAgent is far superior to code that would do the same, but have if/else nests (or hives). if you want to correct RangedAgent -> you go to the class to a separate and categorized environment. You change 3 lines of code, et viola! done. if/else -> track and change all the 20 places in code and pray you didn't brake the existing logic ( even the conceptual flow of updating, handling messages and computing the actions, because your changes are not external to this simple algorithm, and might have changed it in case of if/else hive ).

Game concepts rarely fit nicely into "polymorphic objects"

That is false, sir.
Game concepts are as abstract as any other, and there are exactly the same rules, same constraints. making tall trees of inheritance is as bad and crippling in any case of programming as it is in games. just keep your hierarchies flat, and you'll stay safe, clean and effective. when games are compiled they is always a speed optimisation vs size optimization and it is not that rare, that size optimizations win. if/else rich code loses in that matter vs polimorphic calls in a well designed engine.

a concept that isn't working with polymorphism are any kind of optimized memory allocators, but as I've seen so far, you don't have any in the project.

[redv]

Perhaps, will be useful.
High Integrity C++ coding standard: https://dl.dropboxusercontent.com/u/73411168/PRQA_High_Integrity_C%2B%2B.pdf
I got it here: https://www.programmingresearch.com/high-integrity-cpp/

[djemon_lda]

I've read the document, and here are my comments:

C++ Guideline 3.1.13 Verify that all classes provide a minimal standard interface against a checklist comprising: a default constructor; a copy constructor; a copy assignment operator and a destructor.

"ALL" - this isn't ok. boost has it's noncopyable class for a reason, and this reason is that so much classes weren't meant to be copied at all.

C++ Guideline 3.2.4 An abstract class shall have no public constructors.

this is a purely useless rule.

Use public derivation only.

disables you from layering code common for each object instantiated for a template, which makes your compile generate tons of redundant code.

C++ Rule 3.3.5 Override all overloads of a base class virtual function.

if you really need  to do this because of "safety", then your classes need a redesign.

Rule 3.4.5 When publicly deriving from a base class, the base class should be abstract.

in such cases it's always a composition vs inheritance issue to talk through in the team and evaluate the profit of each solution, sometimes inheritance wins, so the rule isn't always viable, and by that shouldn't be called a rule.

Rule 3.5.4 Make binary operators non-members to allow implicit conversions of the left hand operand.

not always what you'd like to have actually...

Rule 5.2 For boolean expressions ('if', 'for', 'while', 'do' and the first operand of the ternary operator '?:') involving non-boolean values, always use an explicit test of equality or non-equality.

in case of 'if' and 'while' this is actually a very redundant rule.

C++ Rule 7.1 Always use casting forms: 'static_cast', 'const_cast', 'dynamic_cast' and 'reinterpret_cast' or explicit constructor call.  Do not use any other form.

this is actually CRAZY IMPORTANT. using these guys is not only safer and nicer, but it's a nice and free tool to make first general verification on the code quality.

C++ Guideline 10.6 When comparing variables and constants for equality always place the constant on the left hand side.

bad guideline, already explained why.

C++ Rule 12.7 Document that operator new and operator delete are static by declaring them static.

this is just silly...

Rule 15.1 Do not use variant structures (unions).

they are required to make a nice memory pool, and not die in the process but in general I agree with this.

Rule 16.3 Only instantiate templates with template arguments which fulfill the interface requirements of the 'template'.

disagree - if all you need from a template is the part of functionality that your object fulfills, then it's better to use it that way, than to write code without reason.

Guideline 16.4 Only use templates when the behaviour of the class or function template is completely independent of the type of object to which it is applied.

true, but only in cases when the template is for everyone to use. making private template methods that will handle a handful of types, to save a lot of code is ok, and it IS a way to go, rather than to duplicate the code.

Rule 17.8 Never create containers of auto_ptrs.

CRAZY IMPORTANT

[alienfood]

A lot of religion in this thread. This is the most important fact:

it could've also scared away everyone that doesn't have a clue what I'm on about. Software architecture is tricky, specially with an open-source project.

OpenXCom is already a successful architecture, given that it is up and running. I'm from the Hippocratic school of software architecture: first, do no harm.

[karadoc]

I've noticed that most functions declare all their variables at the start of the function, and initialise them later.

I'm a firm believer that variables should be initialised at the same time that they are declared (and that they should only be declared when it possible to initialise them).

I'm not suggesting that we go through all the code and change all that stuff, but I am wondering if the start-of-function declarations are a deliberate style decision, or just a historical artefact. ie. would anyone be upset if some declarations moved to the middle of functions when the code was updated?