-
Notifications
You must be signed in to change notification settings - Fork 47
CPP Style Guide
This guide describes preferred C++ style, in an attempt to guide the code towards maintainability, readability, and robustness. Anything is subject to debate and revision based on feedback.
Do NOT use the legacy code as a guide for good (or even acceptable) style. The legacy code is terrible.
Names should be meaningful.
Functions should be named such that another coder can decide whether the function does what they want, without them needing to read the function code. If this is not possible then either the name is bad (it doesn't describe the function well enough), or the function code is bad (it can't be easily described). In the former case you can just improve the name, in the latter you should consider if the function should be split into multiple functions with simpler responsibilities.
Examples of good names: unitIndex
, maxAllowedDistance
, getPlotFromUnit
.
Examples of bad names: data
, i
, value
.
Classes and Enums use PascalCase
.
Functions and variables use camelCase
. Member variables should have m_
prefix. Hungarian Notation should NOT be used (although it is used extensively in the legacy code).
Macros use SCREAMING_SNAKE_CASE
.
Namespaces use lowercase
, and should be short (preferably less than 6 letters).
Prefer to keep one class per file in most cases. Sometimes it makes sense to keep helper classes in a single file, or multiple classes with a similar purpose in a single file (e.g. a set of parameter classes that all hold a different parameter type, although templates would make more sense here probably).
Prefer composition to inheritance.
Prefer small interfaces. Large interfaces indicate that a class may have too many responsibilities, or that independent or optional aspects of the object definition could be factored out. e.g. The CvUnit
class has an extensive interface, a large portion of which is only relevant for units of certain types. This can be improved by factoring out optional components from the CvUnit
class into separate classes, which can then be accessed via a single member function.
Prefer standalone functions to private class functions. Class functions are only necessary when access to class internals is required, otherwise they are just cluttering the class definition. Instead implement standalone functions in the classes .cpp file, preferably in a namespace {}
.
Prefer short functions. It is very rare that a function length of > 100 lines is justifiable, it almost always indicates a poor implementation of the authors intent. This is seen everywhere in the legacy code, e.g. many hand rolled algorithms (find, accumulate, transform) that could easily use the stl
or boost
ones instead, poorly formed branching where clauses are repeated, much copy pasted code.
Functions should perform a single logical operation.
Prefer declaring variables where they are used. The legacy code tends to declare variables at the start of a function. This is very bad practice. Instead declare variables as close to where they are used as possible, including within loop scope. This goes for most types, including strings, temporaries, pointers, integrals.
// Bad:
int value;
value = calculateValue();
// Good:
int value = calculateValue();
Use for
for an iteration over a known range, only use while
when not iterating over a range. while
is only appropriate when each iteration of the loop is not operating on a consecutive item in a range. The legacy code uses while
loops in many places where a for
loop would be the correct choice.
Prefer foreach_
over for
. foreach_
can iterate over a range and provide the items directly. Range iterators are implemented in most places, and more can be added easily.
Prefer using algorithms over loops. There are a large array of existing algorithm implementations for doing common operations on ranges of items (e.g. vectors/arrays/range iterators). These are almost always preferable to writing a for
/while
loop as they more directly express intent in the code (providing better readability and easier understanding), lead to more concise code, and often give better performance. Generally if you want to find items by some criteria (e.g. find enemies on a plot), find the best/worst item (e.g. find best defender on a plot), convert a list of items into another list (e.g. calculate score for all buildable buildings), or combinations of these things, you should be using an algorithm (find_if
, max_element
, transform
etc.)
Prefer shallow nesting. If a function goes beyond 3 levels of indentation it is a strong hint that there is a problem, such as a function having too broad responsibility, missed chances to use existing algorithms, overly complex logic, or missed abstraction opportunities. Deep nesting leads to poor readability, and poor maintainability, and is noted as a "bad thing" (tm) in many coding style guides.
Prefer to avoid boolean parameters. They don't convey any meaning at the call site, and require the reader to either know the function, or investigate it to know what the meaning of the parameter is. Instead you should prefer to use a boolean enum, enum flags, or different functions.
kill(Kill::Immediate)
andkill(Kill::Delayed)
instead ofkill(true)
andkill(false)
killImmediate()
andkillDelayed()
instead ofkill(true)
andkill(false)
pathfind(PathFind::AvoidEnemy | PathFind::StayInBorders)
instead ofpathFile(true, true)
Prefer to keep the number of parameters low. More than 5 parameters makes function calls hard to understand. A good function name will make the meaning of parameters clear, but the more parameters, the less that is possible.
findBestDefenderWithMinStrength(plot, 5, UnitAI::Defensive, -1)
-- it is impossible to say what the two numbers indicate without looking at the function definition.
findBestDefenderWithMinStrength(plot, 5, UnitAI::Defensive)
-- it is easy to assume that the min strength is 5.
Nightinggale: Vanilla uses pointers in cases where references should be considered instead. They compile to the same thing, but references can't be NULL. This way having an argument or return value as a reference instead of a pointer is telling future programmers that this function doesn't handle NULL.