-
Notifications
You must be signed in to change notification settings - Fork 2
coding standards
"The best thing about standards is that there are so many to choose from."
A common style is good for everyone because consistency makes code easier to read, maintain and discuss. Following a standard such as this will result in improved collaboration and higher quality code, reflecting our belief in the core value "Insist on the Highest Standards".
The document is divided into sections based on high-level C++ language features, with sub-sections to discuss more specific syntax and style issues. Statements marked Required should always be adhered to. Statements marked Recommended should be adhered to unless doing so would be detrimental to the code for some technical reason (i.e. beyond personal preference). Statements marked Reason exist for some entries to clarify intent and explain contentious or non-obvious decisions.
In common with many standards documents, e.g. ISO C++, all inline examples are provided for illustrative and informational purposes, and are not considered normative
Some of the style choices described in this document are motivated by safety or performance or features, while others are arbitrarily disambiguating choices; sometimes the only benefit to picking one particular style over another is code consistency.
If you encounter something that is not covered you will need to use your best judgment; a safe approach is generally to do as the existing code does.
In an ideal world, specifying a language standard would mean that compliant code would work as expected with any compiler and on any platform that implements that standard. In some cases compilers may be behind the standard and in other cases the implementation may differ from the standard in a way that affects cross-platform portability. This section of the document attempts to deal with the practical implementations of this.
Required: Use modern standard-compliant C++17 where possible.
Required: Your code must compile without errors and warnings on all compilers used by the project.
In engines from the past STL was avoided and ended up reimplementing a lot of STL's functionality. Most of the original reasons for this came down to performance issues. These issues rarely hold true anymore.
Required: Use the functionality provided by STL unless there is a genuine reason why rolling your own would be better.
File names and paths have to be carefully managed to avoid both accidentical typos and problems with case-sensitive file systems.
Required: Use forward slashes always in paths.
Required: All file and folder names should be lowercase and seperated using underscores not spaces.
Namespacing is important in modern C++, its an unfortunately fact of life that a lot of third party libraries (and even the windows headers!) pollute the global namespace with identifiers that can easily cause conflicts.
Required: All code (except third party code) should be contained within the "workshop" namespace.
Required: Namespaces should not be nested. The exception is "workshop::detail" for private implementation details. Nested namespaces are rarely beneficial for avoiding internal identifier clashes, and add unneccessary work for the developer to manage and type.
Rather than specify detailed formatting guidelines in this document we are using a code formatting tool (Uncrustify) that converts code to the desired formatting. See this document for details of how to use it: Code Standards
Required: Indent with 4 spaces, do not use tabs.
Reason: There is no clearly better solution when it comes to tabs vs. spaces, there is only zeal. This topic has generated countless hours of debate – The choice of spaces over tabs is made to ensure consistency, i.e. to address the most important goal of the Coding Standard.
Required:
- Files should start without any indentation – only add indentation if the standard indicates it is needed.
- Use a single additional level of indentation for each nested block of code.
- Note: Nested blocks are usually started by entering a new scope, i.e. whenever you would have used a brace, but there are other situations (see examples below).
- Indent all lines of a block by the same amount (see "Make lines reasonably long" for advice on continued lines).
Exception:
- Exceptions for indenting is made for the "workshop" namespace as that would cause all files to always be indented one level.
namespace workshop {
// One level of indentation added for namespace block
void example()
{
// One level of indentation added for the function block
if (foo())
{
// One level of indentation added for the if-block
bar()
// Tricky ...
switch()
{
// One level added for the block
case enum.unknown:
// Extra level added for the statements in the "case"
baz();
break;
}
// etc...
}
}
}
namespace workshop
{
void BadExample()
{
// Indentation missing...
int a = 10;
if (foo())
{
bar();
// Too much indentation added
if (bar())
{
}
}
}
}Recommended: Indent preprocessor statements in a similar way to regular code.
Reason: This is purely for ease of reading.
Recommended: Where possible preprocessor macros should be replaced with if constexpr statements.
Reason: Preprocessor statements have a lot of downsides due to their lack of scoping and can cause problematic situations. constexpr's are a full semanted part of the language and don't have the same flaws.
Required: Preprocessor statements should always be capitalized to make them distinct.
// Fairly obvious
#if defined(XXX)
#include <Something.h>
#define SOMETHING
#else
#include <SomethingElse.h>
#define SOMETHING_ELSE
#ifdef WINDOWS
#include <WindowsSpecific.h>
#endif
#endif
// Not very nice
#if defined(XXX)
#include <Something.h>
#define SOMETHING
#else
#include <SomethingElse.h>
#define SOMETHING_ELSE
#ifdef WINDOWS
#include <WindowsSpecific.h>
#endif
#endif
// Ok...
void foo()
{
if (bar())
{
#ifdef _DEBUG
baz();
#else
zab();
#endif
}
}
// Arguably more pleasing on the eye:
void foo()
{
if (bar())
{
// (Use sparingly! Best practice is to avoid conditional compilation in the first place)
#ifdef _DEBUG
baz(a,b,c)
#else
zab(x,y,z);
#endif
}
}Required: Open braces on a new line, and flush with the outer block's indentation
void good_function()
{
if (condition)
{
}
}
class correct_class
{
};
void wrong_function(){
if ( condition ) {
}
}
class evil_class {
};Required: Always use braces for flow control statements
Reason: Control statements like if-else, while, for and do used without braces can lead to serious unintentional coding errors where statements execute at the wrong scope or in the wrong order (e.g. CVE-2014-1266, aka Apple's SSL goto fail). Additionally, consider that auto-merges exist, and do not have the context to guess how to merge single statement blocks.
// Unacceptable: Naked if/else/while/do/for etc...
if(foo)
bar();
// Acceptable:
if(foo)
{
bar();
}Required: Each line should contain no more than a single statement.
Reason: It is easy to misread lines with multiple statements compounded together. It also helps with debugging in some tools e.g. breakpoint placement in Visual Studio is per line, not per statement.
// Unacceptable: Single line if statements
if (foo) bar();
if (foo) bar(); else baz();
if (foo) { bar(); baz(); }
// Acceptable: Expand the statement and follow the advice for braces
if (foo)
{
bar();
}
else
{
baz();
}
// Unacceptable: Single line case statements
switch(foo)
{
case bar: baz(); break;
}
// Acceptable:
switch(foo)
{
case bar:
baz();
break;
}
// Unacceptable: Single line method/function definition
bool bad_method() { return false; }
// Acceptable: Method/function definition with the body on its own line (following bracing convention)
bool good_method()
{
return false;
}
// Unacceptable: Compounding unrelated assignments
x=foo(); y=bar(); z=baz();
// Acceptable:
x=foo();
y=bar();
z=baz();
// Acceptable: Chained assignments
x = y = z = foo();
// Unacceptable: Using the comma operator to get around the single statement rule
a = foo(), bar();
// Acceptable:
a = foo();
bar();Required:
- Do not write excessively long lines.
- When splitting a line apply one level of indentation.
Recommended: If the line is longer than 100-120 characters consider splitting it at around column 80.
Reason: It is easy to misread statements when they extend off the page by a significant amount.
// This is too long
if (first_function() && second_function() && third_function() && forth_function() && fifth_function() && sixth_function() && seventh_function())
{
}
// This is more readable
if (first_function() && second_function() && third_function() && forth_function()
&& fifth_function() && sixth_function() && seventh_function())
{
}
// This is overkill; flow control looks bad like this:
// (Find a better way to express your logic!)
if (first_function()
&& second_function()
&& third_function()
&& forth_function()
&& fifth_function()
&& sixth_function()
&& seventh_function())
{
}
// This is ok; arguments generally look ok when written like this:
printf("This is a long format string followed by a lot of parameters %d, %d, %d",
first_argument(),
second_argument(),
third_argument());-
Required: When declaring variables of pointer or reference type, the '*' and '&' symbol should be placed on the left; i.e. grouped with the type identifier.
-
Reason: There are three schools of thought on this matter; "type& value", "type &value", or "type & value". There is little difference between the styles aesthetically so the standard arbitrarily chooses "type& value" to ensure consistency – code which mixes the 2 styles is considered to look sloppy and can lead to comprehension errors.
void* buffer; // Ok
void *buffer; // Not Ok
void foo(const string& bar); // Ok
void foo(const string &bar); // Not Ok
void foo(const string & bar); // Not Ok
const char* get_name(); // Ok
const char *get_name(); // Not Ok
const char * get_name(); // Not Ok
// Pointer-to-pointer declarations follow the same pattern and the base rule:
const char* const* buffer; // Ok
const char* const *buffer; // Not Ok
const char * const * buffer; // Not OkSee https://en.wikipedia.org/wiki/Snake_case for an explanation of the "Snake Case" used below.
See http://en.wikipedia.org/wiki/Hungarian_notation for an explanation of "Hungarian Notation" mentioned below (Note: This is merely here for reference, our style does NOT use Hungarian).
Required: Do not let your names become misleading! Think carefully about what a name represents and what expectations it carries before using it. If the thing you're naming evolves out of its original usage, then strongly consider renaming it.
matrix m_position;
/* Confusing: a position is usually 3 floats in 3D. Why is it a matrix? Is it actually a transform, or is it using an inappropriate storage type? */
string m_age;
/* Confusing: an age is usually a numeric value, how is this a string? */
map m_list_of_names;
/* Confusing: the name implies that it is a list, but it is actually a map */
class player: ui_string {};
/* Confusing: player implies at least an actor of some kind, so why is it a string? */
enum town_names { town_names_boston };
/* Confusing: names are usually strings, but enums are numbers */- Recommended: Make your names verbose. That way people will not need to guess, the code will be more readable and you will need less documentation. Typing is not your enemy, copy and paste is. Don't try to shorten words too much either. 'i', 'inst' and 'instance' are not the same and while 'inst' could be common enough for people to know you mean instance, this is not always the case. Remember that programmers from many different domains might come across your code, try not to take anything for granted.
int m_cnt; // <-- bad. It's not clear whether it's "count" nor what is it that it counts
int m_use_count; // <-- better. It's clearly a count, and it's clearly counting usesRequired: All class/struct names should be written in snake_case
Required: Don't use 'C','T','I' or any other letter to prefix a class.
class example_class {...};Reason: This is the standard C++ identifier formatting, as we heavily use STL this keeps the code consistent.
- Required: Follow the rules for class naming.
- Recommended*: We do encourage adding a '_t' at the end.
using mytype_t = a_class;Required:
- All variables including local and function parameters should be named in snake_case.
- Hungarian Notation for type information is not to be used (i.e. no prefixes of i, f, p, sz, str, etc...)
- Storage class prefixes are not considered Hungarian and should be used as follows:
- Member variables, for both classes and structs, should begin with an 'm_' prefix, e.g. m_value_of_stuff
- Static member variables should begin with the 's_' prefix, e.g. s_monotonic_counter
- Global variables should begin with a 'g_' prefix, e.g. g_data_for_everyone
- Global variables are discouraged and forbidden for non-POD types
- Make sure that you use and name your variables appropriately. Use verbose names that accurately reflect what the stored value represents. Avoid using abbreviations and acronyms.
The use of constants is necessary in many situations. It is almost always better to declare such constants as variables rather than using literals.
Recommended: Use variables wherever possible; they are type safe. Use #define only where absolutely necessary.
Required:
- If you use a variable for a constant, make sure it is declared "constexpr" or "static constexpr" and follow the rules for variable naming
- If you use #define follow the rules for macro naming
- Required: All functions should be written in snake_case and be as verbose as possible. Given that we require namespaces, do not prefix function and classes with the system name. Do write get_value(), do not write workshop_get_value().
NOTE: This includes RPCs or public functors on a structure. These are supposed to look and act like public methods, and they should be named as such.
Recommended:
- If the function only modifies a member variable, we do recommend using the "get_x"/"set_x" naming pattern.
- When one function operates on an object and another function just returns a copy of it, we recommend naming the functions differently. A "get" prefix is recommended on the function that returns a copy.
// These accessors are not recommended, because the identical naming can lead to subtle bugs.
void speed(float speed);
float speed() const;
// These accessors are recommended, because their names are not ambiguous
void set_speed(float speed);
float get_speed() const;
// The following function would be expected to mutate the object itself
void normalize();
// This would be expected to to return a normalized version of the object, without changing the object itself
vector get_normalized() const;Required: All namespaces should be written in snake_case.
Recommended: Add the name of the namespace as a comment after the closing brace of a namespace.
namespace workshop
{
...
} // namespace workshopRequired: Enumerator names and values should both be written in snake_case.
Recommended:
- Prefer scoped enums over unscoped enums where possible
- For code consistency use only "enum class" for scoped enum declarations, i.e. never "enum struct"
- The default scoped enum type is "int", so if using "int" you should omit it from the declaration to reduce redundancy
- For unscoped enums, you should name the values of the enum with a prefix derived from the enum name
- Try to contain the enum declaration in an appropriate outer namespace or struct declaration (see examples)
Required: Avoid adding unscoped enums to the global namespace
Reason: Unscoped enums can be accessed directly without including the enum name, so the prefixing reduces the chance of name clashes, and helps code assistance tools like Intellisense or VisualAssist display more useful suggestions by keeping related flags grouped. For scoped enums prefixing is not needed as you need to refer to the enum before accessing its members.
struct my_data
{
enum flags
{
flag_weapon = (1 << 0),
flag_fast = (1 << 1),
flag_flying = (1 << 2),
};
// Flags can be referred to in code as my_data::flag_weapon
}
or
// In this variation a namespace is used to contain the enum
namespace weapon_flags
{
// An unnamed, unscoped, enum is used for a bitfield
enum
{
two_ganded = 1,
has_ammunition = 2,
has_recoil = 4,
};
// A type is introduced in the namespace to explicitly control the representation
// (this may be useful in some serialization and layout contexts)
typedef int type;
}
or
enum class asset_status
{
not_loaded,
loading,
ready,
error,
};
// Used from code asset_status::loadingRequired: All macros should be written in CAPITAL_LETTERS. They should be prefixed with the names of the engine and/or system in which they are defined. Macros should NOT contain any occurrence of "__" (double underscore) and they should NOT begin with any "_" (underscore) character(s) as these are reserved exclusively for the compiler's use.
Recommended: Use macros as a last resort. Seriously! They're bug prone and difficult to debug.
#define WS_ASSERT ...Reason: While macros can be really useful, they do have serious pitfalls.
The first one is name clashing: Macros are always in the global namespace, so you are basically reserving this name for everyone in the same compilation unit. Macros also match before any function does, and they match signatures more primitively than functions do. For an example of how this can backfire, look at the windef.h min and max macro issues: because these are pulled in as a consequence of using the extremely common windows.h, codebases often have to undef them to free up the use of those very common names for types that don't work with those macros, even when the replacements have the same intent. For one, there is an obvious clash between these and std::min and std::max.
Reserving a common name like "Assert" without a prefix is very bad, as you are likely to also have functions named "Assert". Such issues are hard to debug, as very often there will be no compile error.
The second big pitfall is with macro expansion and its safety. There are many documents online that discuss that issue, so please be familiar with them before deciding to use a macro in LY.
Required: All files need to begin with the following copyright header.
// ================================================================================================
// workshop
// Copyright (C) Tim Leonard. All Rights Reserved
// ================================================================================================Required: All header files must include the directive, "#pragma once".
Related: Do not add the old school "#ifndef SYSTEMNAME_FILENAME_H" form.
Reason: Include guards avoid problems with having a single file appear multiple times in an include chain. All the compilers we care about currently support the #pragma once directive.
Required: All include file paths must use forward slashes and be lowercase e.g. #include <az_core/std/containers/unordered_map.h>
Reason: Backslashes for paths are only supported on Windows and will causing compilation errors on other platforms. Some platforms also have case sensitive file systems so ensuring paths are lowercase avoids that failure case.
Required: Compile times are always a problem, so we require people put in effort to minimize their include chains. The closer we get to the core layer, the more important this is, but no header file should underestimate its contribution.
You should forward declare anything that isn't directly required in a header. That is to say you should avoid all #include directives in header file X, except for those that would prevent a CPP file that only includes header X from compiling.
You should use the PIMPL idiom where possible. (http://www.boost.org/doc/libs/1_49_0/libs/serialization/doc/pimpl.html )
Useful discussions on forward declaration can be found at http://stackoverflow.com/questions/4757565/c-forward-declaration and http://stackoverflow.com/questions/553682/when-to-use-forward-declaration)
Bear in mind that you can forward declare templated classes too.
Required: We do use precompiled headers, but they are NOT an unlimited resource. Actually using IncrediBuild (which we do use) and/or gcc compiler, very quickly reaches a point where the compile times become slower with a bigger PCH file. This is why you should make sure your include chain is as simple as possible and that's it! We have tools that analyze the code and make empirical tests to determine what should go into the precompiled header. Don't add files to the precompiled headers unless you are really sure about what you are doing. Make sure to include every file needed by your file. Do not rely on the precompiled header since it may be disabled for certain platforms.
Recommended: Despite available directives, function inlining is something on which the compiler has the last word. Even if you use "__forceinline" on MSVC, the compiler can still ignore it and output a warning instead.
Do not use __forceinline or any other inline hinting. Use the inline keyword only when you absolutely must to hint the linker.
Rationale: Inlining must be left to the compiler. In all experiments without our codebase, force-inlined code was demonstrably more instructions and equal or worse performance. Force inlining reduces the tools the compiler can use to reason about the code and optimize it.
// In these trivial cases the compiler will output the same code
// so don't worry about inlining such functions.
int get_speed() const
{
return m_speed;
}
inline int get_mass() const
{
return m_mass;
}
// DO NOT DO THIS
__forceinline int get_mass() const
{
return m_mass;
}Recommended: Along with inlining above, be careful with implicit inlining (functions defined inside class declarations). Also, please restrict implementation of functions in headers to trivial getters and setters and template functions.
Rationale: We have a large code base. Minimizing compilation overhead and maximizing iteration time are priorities for us. If you have to change a header to iterate on your implementation, your iteration times will suffer. Further, if the compiler has to deal with your code every time it ingests your header, you've added a little bit to everyone's compile time. Multiply that across a large team, and it's death by mosquito.
Required: The following syntax should be used when including header files:
#include <package/subdirectory_chain/header.h>
Reason: This rule helps disambiguate files from different packages that have the same name. <object.h> might appear relatively often, but <workshop.render/object.h> is far less likely to.
Notice the use of angle brackets, and the inclusion of the package name. For this schema to work, the package's include directory will have to have an extra subdirectory with the name of the package in it. The compiler will then be told to include the package include directory as an additional location to search for headers from.
.../package_root/includes/package_name/SomeFile.h // <- a file at this location
.../package_root/includes // <- is part of a package given to the compiler this way
#include <package_name/some_file.h> // <- and included with this statement// Correct
#include <Package/string/string.h>
// Incorrect
#include <string.h>
#include "string.h"Requirement: You must define a default constructor if your class defines member variables and has no other constructors. Prefer to use the = default; over {} default constructor. Unless you have a very specifically targeted optimization, always initialize all the variables to a known state (even if it's invalid).
Recommended: Prefer to specify default values for class members at the declaration site in the header (C++11 style), rather than in constructor initializer lists or in the bodies of constructors. This way, the value only has to be specified once, has no requirement for constructor chaining, and is not susceptible to re-ordering causing warnings. If members are initialized in a constructor based on arguments or other members, use initializer lists wherever possible and ensure that members are initialized in declaration order. Warnings are enabled to enforce this.
Required:
- Do not assume any specific properties based on the choice of struct vs class; always use <type_traits> to check the actual properties
-
- e.g. Use "is_pod" to check for POD types; do not assume structs are PODs or assume classes are not PODs
-
- Other useful checks include: "is_trivial" and "is_standard_layout"
-
- Remember that "is_class" will be true for structs as well as classes
Reason:
The only actual difference between class and struct is the default visibility of members, which is required by the C++ language standard to ensure forwards compatibility from C to C++. The compiler regards both constructs as being of "class type" and treats them the same way for syntax and language purposes. However, it is notable that a convention exists whereby some engineers use struct to denote types that contain only POD members and class for everything else. This standard does not endorse (or prevent) this convention, but it does caution adherents that the practice is not universally accepted and code should never assume POD or non-POD properties based solely on the presence of struct vs class.
Recommended: Public declarations come before private declarations. Methods should be declared before data members. When laying out data members, consider the packing size and order requirements of your structure. Put data members which are used together near each other as much as alignment allows, for cache friendliness. Always prefer fixed size integral types (uint32_t rather than unsigned, for instance). Guarantee consistent structure size and layout, unless you have a very good reason not to.
Recommended: All methods that do not modify internal state should be const.
Required:
- All function parameters passed by pointer or reference should be marked const unless they are output parameters.
- Do not use const for by value parameters (unless it is part of a template in which the generic treatment of a parameter leads to it being expressed as const)
- Exception: Feel free to const parameters in the definition of a function (e.g. in the cpp file). This will be more semantically clear to someone reading the implementation later.
- Do not mark a function as const if it is const-broken, that is if it returns a non-const pointer or reference to another object.
// Unacceptable: Accessor method should be const since it doesn't change the state of the object
int get_number_of_players()
{
return m_numberOfPlayers;
}
// Acceptable
int get_number_of_players() const;
// Unacceptable: non-const reference where the parameter is not an output
float get_magnitude(vector3& v);
// Acceptable: const reference
float get_magnitude(const vector3& v);
// Unacceptable: const on by-value parameters restricts the implementation unnecessarily while doing nothing for the caller
void my_function(const int a, const int b);
class some_class;
class my_class
{
// Unacceptable: This is a const-broken function, so const-ing it isn't actually helping other programmers or the compiler ensure correctness
some_class* get_some_thing() const { return m_some_thing; }
some_class* m_some_thing;
}Required: Use the override keyword wherever possible
Recommended: Omit the keyword virtual when using override (virtual is implicit whenever override is used; follow the mantra "No unnecessary syntax").
Reason: The override keyword was added in C++11 so that a method can clarify its intent to override an existing virtual method on a base class (i.e. as opposed to declaring a new virtual method); it generates a compiler error when it is used on a method that is not overriding something.This will help catch errors of intent when the code is initially written and also in the future should the base class be modified.
(see also: http://en.cppreference.com/w/cpp/language/override)
Recommended: Use the final keyword where its use can be justified.
It is justified if overriding a class or method would e.g. allow an invariant to be violated or it enforces consistency
It would not be justified if the only difference it makes is restricting client code
http://en.cppreference.com/w/cpp/language/final
Required: All of your code should be in the "workshop" namespace Required: You may not use the "using namespace" directive in a header OR source files. It can cause issues with unity builds.
Required: Place a function's variable declarations in the narrowest possible scope, and always initialize variables in their declaration. Global variables
Required: Static member or global variables that are concrete class objects are completely forbidden. If you must have a global object it should be a pointer, and it must be constructed and destroyed via appropriate functions.
Reason: Static class objects cause hard-to-find bugs due to indeterminate order of construction and destruction. They can also circumvent the system's effort to log, track memory, or generally intercept their actions in useful ways.
Required: All platform specific code should be located in a separate project, with a generic project interface shared between all platforms.
eg. workshop.core contains the definitions for platform code, workshop.core.win32 contains all the implementations for windows of the defined functions.
Projects are then selectively linked based on platform. This avoids a lot of nasty macro-including in files based on platform and reduces the maintenance burden when adding new platforms. It also provides a clean segregation of the code and prevents platform defines or code leaking out.
This is one of the harder standards topics to address objectively, since what is considered appropriate for this keyword varies considerably across the development community. We will periodically revisit the topic to assess the positive and negative impacts of ‘auto’ on code quality.
Required: When using ‘auto’ make sure you are optimizing your code for correctness and maintenance rather than authoring. i.e. all things being equal, if using ‘auto’ would decrease either correctness or maintainability then you should not use it in that scenario.
Recommended: If using ‘auto’ would increase either correctness or maintainability then you may use it, but you are not required to do so.
Reason: Over the lifecycle of a product your code will be read many more times than it was written, so it is important for us to consider maintenance ahead of authoring concerns.
The following examples cover some common scenarios for illustrative purposes; for other situations some burden is placed on developers and reviewers to consider if the use-case is appropriate under these guidelines.
// Using auto for literal assignment is portable and safe – The compiler
// will choose the most appropriate compatible type. Type information is
// easily inferred by the reader at the location of the declaration.
const auto n = 42LL;
const auto s = L"wide"; // i.e. "wchar const * const"
// Using auto to store the result of a typed expression is safe and
// efficient. Type information is easily inferred by the reader.
auto ptr = std::make_shared<Foo>();
auto value = static_cast<int>(number);
// Using auto is the most efficient way to capture a Lambda; since each
// lambda has a compiler generated closure-type, it is impossible to
// specify the type of the lambda manually.
auto op = [](int a, int b){ return a+b; };
// Note: std::function is a library feature not a language feature, and is
// actually a much less efficient way to capture a lambda. This example
// shows an unnecessary use of std::function:
std::function<void(int)> op = [](int a){ std::cout << a; };
std::for_each(m_list.begin(), m_list.end(), op);
// It would be more efficient and less verbose with auto:
auto op = [](int a){ std::cout << a; };
std::for_each(m_list.begin(), m_list.end(), op);
// Using auto for iterator values in algorithms is often clearer because
// it is less verbose.
auto begin = m_list.begin();
auto end = m_list.end();
// Using auto for the results of a generic algorithm or container operation
// is also often much clearer
auto it = std::find(begin, end, value);
// However, in both cases above type information is hidden, so the author
// should consider carefully whether they need to explicitly type some
// later part of the expression for clarity ...
if (it != m_list.end())
{
// This is a problem; it is not clear to the reader what the assumed
// type of the iterator is because it was never stated
it->call_some_method();
// Clearer version of the same thing
IActor* actor = *it;
actor->call_some_method();
}
// ... or consider self-descriptive naming instead; this isn't explicit
// but still leaves enough cues for the reader to understand the code
// without extensive research
auto actor_iterator = std::find(begin, end, value);
if (actor_iterator != m_list.end())
{
actor_iterator->call_some_method();
}
// Using auto in for-range statements may seem convenient, but it is
// not always as clear as its alternatives overall:
// e.g. dealing with "vector<int> m_list"
// Not a good use of auto; type hiding without any obvious gains
// (This also makes a copy of the list element which may not be your intention)
for (auto element : m_list)
// Obviously clearer version
for (int element : m_list)
// For templates explicitly using T is usually clearer too:
// Note: Remembering to use "const" and "auto&" as appropriate with for-ranges
for (const T& element : m_list)
// Complex types are more justifiable:
// e.g. dealing with "vector<pair<int, IActor*>> m_pairs"
for (const auto& element : m_pairs)
{
// Type hidden version
element.second->call_some_method();
// Clearer version through explicit local
IActor* actor = element.second;
actor->call_some_method();
}Required: ‘auto&&’ should be restricted to template and generic programming situations where it is the only correct solution. i.e. Do not use ‘auto&&’ in any situation where it could be correctly substituted for a more specific form such as ‘const auto&’.
You will generally find that this rule prevents you using it for locals, except in some specific template programming situations.
Reason: The construct ‘auto&&’ is generally undesirable because it manages to hide both type and const information at the same time. It is syntactically correct in most expressions – by virtue of being a catch-all – but this does not meet the definition of correctness the standard is trying to promote.
Required: Use nullptr in place of the macro NULL and the literal 0 for pointer values.
Reason: The keyword nullptr addresses a long standing deficiency in C++ by allowing a type safe way of unambiguously specifying an empty pointer value. (Other solutions are either ambiguous or not type safe).
Required: Make use of standard type conversion rules, explicit bool operators and the safe bool idiom in preference to explicit default value comparisons when constructing boolean expressions from non-boolean types.
Reason: Explicit comparisons bypass both standard and user defined boolean conversions and can be both less efficient and dangerous in practice. This is especially relevant to smart pointers, where a comparison to nullptr forces the smart pointer to either unbox the raw pointer it contains, or define operator== and operator!= (which can cause their own issues when type a==type b has either no operator or an asymmetric definition for type b==type a).
Note that C++11 introduces the notion of explicit operators which removes the need for the safe bool idiom and addresses the issues that came with implicit conversions in boolean expressions.
// Unacceptable: Use of default in a comparison where a standard conversion exists
void* ptr;
if (ptr == nullptr)
if (ptr != nullptr)
int i;
if (i == 0)
if (i != 0)
// Acceptable: Use of standard conversion rules
if (ptr)
if (!ptr)
if (i)
if (!i)
// Acceptable: Only true for some non-zero integers and therefore differs from the standard conversion
int i;
if (i > 0)
// Unacceptable: Use of default in a comparison where an explicit operator exists (C++11)
smart_ptr<T> ptr;
if (ptr == nullptr)
if (ptr != nullptr)
// Acceptable: Use of explicit operator (or safe bool idiom)
if (ptr)
if (!ptr)
// Unacceptable (special case): Use class specific methods if they exist ...
std::string s;
if (s != std::string())
if (s != "")
if (s.length() > 0)
// Acceptable:
if (!s.empty())
// Suspect: Depending on the definition of this type there may be an issue here
Foo f;
if (f == Foo())
if (f != Foo())
// Unacceptable:
float f;
if (f == 0)
// Acceptable (but suspicious): Floating point comparison to zero (or any explicit value) may fail due to rounding/precision issues
if (f)
// Safer: Compare to a range to account for precision issues (Epsilon is some arbitrary small constant, e.g. 1E-9f)
if (fabs(f) < Epsilon)Required: Do NOT use them. None of the core engine is exception safe and exceptions may be disabled for any workshop projects.
For tools or platform specific code (like WinRT), feel free to utilize them.
Recommended: Use is permitted where a valid use case for it exists.
Required: Always use C++ casts, they are more explicit than C casts and avoid some unexpected edge-case problems that can result from C casts. Recommended: Avoid the use of dynamic_cast, it usually indicates a code smell and potential issues with your architecture.
Recommended:
- Using const when passing parameters by value is not recommended.
- Using const when returning by value is not recommended.
- It is recommended in all other situations where const can be applied (such as reference parameters, class data members or methods).
Reason: It is better to be restrictive than permissive, and it may help you catch or prevent some types of bugs. Clarity on the intent to modify a variable can also allow the compiler to do more optimizations.
Recommended: We have source control if you want to restore some of your old code, no need to clutter up the files.
Great documentation is critical for the long term success of any project. Documentation comes in many forms, from blog posts to tutorials, and from many sources, from engineers to artists. For engineers the most significant area of documentation is the code itself, so this section of the Coding Standard is dedicated to recommendations intended to improve the overall consistency, depth and usefulness of code documentation.
Since code documentation is produced and consumed almost exclusively by engineers, it is important that all engineers consider themselves responsible for contributing to and maintaining it.
We maintain a github wiki for high level information on the project, keep it up to date with information about user-visible changes in the engine.
Recommended: Try to be specific. Your first priority should be to give good names to everything in the code
Bad Code -- Requires a comment to explain itself
//! Gets the size of the object in bytes.
int size() { return m_size; }
Good Code -- Uses names that tell you what you need to know
int get_byte_size() { return m_byte_size; }Recommended: Full sentences with good grammar are preferable to heavily compacted or abbreviated notes. This improves readability and reduces ambiguity.
Leave your feelings out of it
Recommended: Write your comments in an objective style; be free of emotion, conjecture and machinations. It is usually never appropriate to add an exclamation mark , if you're shouting in your comments you're probably emotional
Use non-gendered pronouns In the spirit of inclusivity and objective documentation, avoid gendering the reader or parts of the code.
See https://chromium.googlesource.com/chromium/src/+/master/styleguide/gender_neutral_code.md for more information
Recommended: The code intrinsically represents what it being done, but it rarely communicates why it is being done. Adding a short comment to explain the reason for choosing a specific algorithm, data structure or other implementation detail adds something that the code alone can't provide. Of course, sometimes you need to explain "what" too, e.g. if the algorithm is non-obvious or complex.
Recommended: Public members are obviously very important, since they form the interface that other code will use to access an object, but you should not forget that private and protected members also benefit from being documented. Never assume you will be the only person maintaining the code and leave some clues for your successor.
Recommended: Documentation should always add value. If the documentation adds nothing there is no reason to include it
Not useful - it just repeats what the code already says
// Iterates through the player list
for (int i=0; i<players.size(); ++i) { ... }
Not useful - non-specific warning may as well not be here
// This will assert if the state is not correct
void OnEventFired()Recommended: Generally TODO is not very useful in code, unless it's an assert. Use the bug tracking software for this. Do not leave ticket numbers or other TODO-like messages in comments. If the comment isn't useful to the maintainer or the customer, it shouldn't be there.
Introduction
Standards
Architecture
- Code Structure
- Build Configurations
- Engine Structure
- Virtual File System
- Entity Component System
- Renderer
- Assets
Helpful Notes