-
Notifications
You must be signed in to change notification settings - Fork 143
Code Style Guide
I've been meaning to write up a small code style guide for a while, now we have more contributors it's probably best that we do it now.
The coding style in this guide should be applied to new code only: we can go around and fix older code when there's not much activity.
This guide is loosely based on OpenMW's code style (at https://wiki.openmw.org/index.php?title=Naming_Conventions and https://wiki.openmw.org/index.php?title=Code_Formatting_Conventions)
In general things should be capitalized properly, eg. CommandMap.cpp, Utils::Strings::Base64Decode, etc.
The exceptions to this rule are class members. Public class members (methods and variables alike) should be capitalized properly, but private members should be in camelCase.
One other exception are method parameters, they should also be done in camelCase eg. void SomeMethod(float ourFirstParam, void* ourSecondParam);
Variables inside a method (local variables) should also be written in camelCase, don't use hungarian notation or anything like that, there's no need to waste extra bytes on the variable name just to describe its type when we have IDEs that can show us the type easily.
So instead of "int32_t dwSizeOfData" it would just be "int32_t sizeOfData"
Enums should have each enum member begin with the enums name, having enum members that are named just with the purpose of the member can cause problems in the code. For example:
enum StringFormattingFuncs
{
printf,
sprintf,
cout
}
Would cause problems, because if you try to use the printf function later it would see the printf enum member you defined instead, and the compiler will complain about you trying to use an enum member as a function.
Instead you should define enum members with the starting character e, followed by the enums name, followed by the enums member name. If your enum name is a plural un-pluralize it for the member names.
Eg:
enum StringFormattingFuncs
{
eStringFormattingFuncPrintf,
eStringFormattingFuncSprintf,
eStringFormattingFuncCout,
}
(I think there might be a way with "enum class" to stop the error above from happening, but right now we use standard enums a lot throughout ElDewrito, so it's best to stick with normal enums until we move things over)
A cpp file should begin by including it's own header. This header file should begin with "#pragma once" to ensure it's only included in the project once.
The header file should only include other headers that are needed for things in that header file, eg. if a header defines a class which uses std::string it's fine to include "string" in the header file.
Headers that are needed for the cpp code should only be included in the cpp file, however if the header is already included in the cpp files h file then you shouldn't include it in the cpp again.
The includes section shouldn't really have many comments, it should be plain to see the type of include (STL/external/etc) just by looking at the includes. Only include comments if they're necessary to explain something.
Other includes should be included in groups, detailed below:
Headers that are a part of the STL should be included before any others, if C headers are used they should be included before C++ ones, eg:
#include <stdio.h>
#include <string>
Headers that are from third-party code should be included here, headers from different third party libraries should be split into separate groups, eg:
#include <rapidjson/writer.h>
#include <rapidjson/stringbuffer.h>
#include <openssl/rsa.h>
#include <openssl/sha.h>
Headers from our project can be included here, if the code uses headers from a different directory they should be included before headers inside the local directory, eg:
#include "../Patches/Network.h"
#include "Console.h"
An example of the includes for a fictional "MapLoader.cpp" file:
#include "MapLoader.h"
#include <string>
#include <sstream>
#include <vector>
#include <rapidjson/reader.h>
#include <openssl/rsa.h>
#include <openssl/sha.h>
#include "../Patches/MapLoad.h"
#include "../Utils/FileReader.h"
#include "Logger.h"
As said above private class members should be camelCase while public members should be capitalized properly.
Private class members should come before public members, and variables should come before methods. Static variables/methods should come before the actual class members themselves.
Eg: An example of a class:
namespace SomeSpace
{
class SomeClass
{
private:
int someVar;
float somePrivateMethod(float p1, float p2) const;
public:
static int SomeVariable;
static bool SomePublicStaticMethod() const;
float SomePublicMethod(float p1, float p2) const;
};
}
namespace SomeSpace
{
float SomeClass::SomePublicMethod(float p1, float p2) const
{
return p1 + p2;
}
}
In general if your class is only created once in the project it's best not to use statics and instead make your class be based on Utils::Singleton, your class can then be accessed at ClassName::Instance()
Code inside methods shouldn't be done C style where every variable is defined at the start of the method, instead define variables only when they get used. That way eg. if a branch instruction or return statement skips over some variables they won't be initialized, saving some CPU cycles.
For indentation we stick with 4 spaces instead of tabs, as 4 spaces will be the same in any text editor whereas tabs can change size (and hence change how you want your code to be displayed). Visual Studio defaults to using 4 spaces too, making it easier for newcomers to contribute to the project and easier on us as we wouldn't have to waste time fixing up their contributions.
We also use the Allman indentation style (see http://www.terminally-incoherent.com/blog/2009/04/10/the-only-correct-indent-style/), where curly braces at the end of statements are put on their own line, making it easier to see where a block of code starts/begins from a simple skim over the code.
Try and minimize the use of function calls. eg. if your code calls a function which returns a pointer/reference then store that as a variable in the method and act on that variable instead. eg. instead of:
if(getBlehPtr()->SomeMethod())
{
getBlehPtr()->Variable1 = false;
getBlehPtr()->SomeOtherMethod(true);
}
else
{
getBlehPtr()->SomeOtherMethod(false);
someOtherThing(getBlehPtr());
}
Do this instead:
auto bleh = getBlehPtr();
if(!bleh)
{
show_error_to_user();
return;
}
if(bleh->SomeMethod())
{
bleh->Variable1 = false;
bleh->SomeOtherMethod(true);
}
else
{
bleh->SomeOtherMethod(false);
someOtherThing(bleh);
}
I don't really comment that much, I'm more of a believer that code is self-documenting. Excessive commenting is for high school programming students and people trying to build up their line count.
In general if your code is clean and readable there shouldn't be much of a need to write comments for it, please don't write needless comments explaining every line your method does while the code itself can explain that for you!
If you do something confusing in your code maybe then write a small note explaining what's happening, or if your code is using a hardcoded number or offset maybe write a note explaining where that number came from.
Also when you finish writing a new method perhaps look back over your code from a newcomer (to the project, not a programming novice) perspective. We want to try and aim for our code to be readable, to help encourage others to contribute to it. If your code is using a lot of project specific things, that only others in the project would understand, you should probably write a small note explaining it.
If you have a block of code inside curly braces you should omit the curly braces, for most statements like ifs/whiles/fors it should work fine and helps not only reduce the number of lines in the code but also makes it look a bit neater. Eg. instead of
if(someCondition)
{
someAction();
}
Do instead:
if(someCondition)
someAction();
This can also work with elses in if statements too, the following should compile fine:
if(someCondition)
someAction();
else
someOtherAction();
As should:
if(someCondition)
someAction();
else
{
someOtherAction();
wew = "lad";
}
And other variants, if you mix single-line and multi-line statements nested inside each other it's recommended you do some testing to make sure each statement acts as you expect it.
The usage of preprocessor constants/macros should be minimized, in general try and only use the preprocessor for conditional things, like "#IF _DEBUG" and such.
Shame on you!
Get out.