Theme: auto

Style Guide

This document outlines the ITP 380 coding style and describes what we look at when looking at your code quality during a code review. While many style items are automatically tested by GitHub Actions via clang-format and/or clang-tidy, quite a few can only be flagged by manual code review.

Obviously, there is a lot of content on this page. We do not expect you to memorize this style guide. Instead, use this page as a reference when looking at your code review feedback.

Table of Contents

Formatting
Naming
Constants
Header Files
Conditionals
Loops
Variables
Classes
Writing Better/Modern C++
Anti-patterns
1. Avoid copy-paste coding
2. Avoid spaghetti code
ITP 380-Specific

In this style guide, you’ll see different labels which note whether the item is automatically detected by clang-format or clang-tidy, or if it is part of our manual code review when grading.

Formatting

Here is a good explanation from the Google C++ Style Guide about why formatting rules exist:

Coding style and formatting are pretty arbitrary, but a project is much easier to follow if everyone uses the same style. Individuals may not agree with every aspect of the formatting rules, and some of the rules may take some getting used to, but it is important that all project contributors follow the style rules so that they can all read and understand everyone’s code easily.

These formatting rules are all automatically applied by the .clang-format file, so if you fail to follow the style, your IDE can help quickly reformat.

Line length

clang-format

The maximum line length is 100 characters. Any lines longer than 100 characters should be split up into multiple lines.

Use tabs

clang-format

Use tabs for default indentation, not spaces. Tab width is set to 4. Spaces are used only as needed to align continuations (where a long line is spit up into multiple lines).

Braces

clang-format

In almost every case, the opening brace should appear on the next line of code, for example:

class MyClass
{
	int mMemberVariable;
};

int MyFunction(int param)
{
	if (param > 0)
	{
		return -5;
	}
	else
	{
		int x = 0;
		for (int i = 0; i < 5; i++)
		{
			x++;
		}
		return x;
	}
}

Lambda expressions should use an opening brace on the same line, but then a closing brace flush with the indentation of the lambda declaration or function using the lambda:

auto lambda = []() {
	return 0;
};

If you are using a C++11 style initializer for your variable, you should keep the opening brace on the same line, as well:

std::vector<int> myVector{5, 10, 13, 27};

Additionally, one-line member functions that fit on a single line may be implemented on one line without requiring the brace on a new line:

class MyClass
{
public:
	int GetZero() const { return 0; }
};

Indentation

clang-format

Ensure the code is indented properly based on scope, as in the examples in the braces section.

The public:, private:, and protected: access modifiers should be indented to line up with the leftmost brace of the class declaration, like:

class MyClass
{
public:
	MyClass();

private:
	int mMemberVariable;
};

Similarly, case blocks in a switch should also line up with the opening brace:

std::string MyFunction(int param)
{
	std::string retVal;

	switch (param)
	{
	case 0:
		retVal = "zero";
		break;
	case 1:
		retVal = "one";
		break;
	default:
		retVal = "n/a";
		break;
	}

	return retVal;
}

Spacing and parenthesis

clang-format

There should always be a space between a control-flow keyword such as if or for and the opening parenthesis, for example:

if (value)
{
	// Do something
}

Function declarations and function calls should not have a space between the function name and the parenthesis:

int MyFunction(int param);

Blank lines

clang-format

Do not have more than one consecutive blank line.

Do not put a blank line immediately following an opening brace or immediately preceding a closing brace.

For function implementations in a .cpp, you should have a blank line in between the implementations.

In a class declaration, you should have a blank line leading into a new privacy section as in:

class MyClass
{
public:
	MyClass();

private:
	int mMemberVariable;
};

Floating-point literals

Code Review

All floating-point variables should be of type float. To properly denote that a literal value is a float, you need a f after the decimal, for example:

float f1 = 10.0f; // Correct
float f2 = 10.0; // Incorrect, 10.0 is a double
float f3 = 10.f; // OK, but not preferred

Naming

Clang-tidy can enforce that your identifiers follow the expected naming convention, though it cannot identify poorly-named variables that violate the “General Naming Rules.”

Occasionally, the naming conventions are not followed by the provided libraries for usability reasons. For example, the Vector3 class has three member variables for each component named x, y, and z, which does not follow the member variable naming convention. However, any such exceptions are discussed and agreed upon by the team.

General naming rules

Code Review

The Google C++ Style Guide has a good explanation of how to name your variables sensibly:

Optimize for readability using names that would be clear even to people on a different team.

Use names that describe the purpose or intent of the object. Do not worry about saving horizontal space as it is far more important to make your code immediately understandable by a new reader. Minimize the use of abbreviations that would likely be unknown to someone outside your project (especially acronyms and initialisms). Do not abbreviate by deleting letters within a word. As a rule of thumb, an abbreviation is probably OK if it’s listed in Wikipedia. Generally speaking, descriptiveness should be proportional to the name’s scope of visibility. For example, n may be a fine name within a 5-line function, but within the scope of a class, it’s likely too vague.

File names

Code Review

Files names are in CamelCase followed by the appropriate extension. The extensions are:

.h for a typical header file
.cpp for an implementation file
.hpp for a templated class header where both the declaration and implementation must be in the header file

Class naming

clang-tidy

Class/struct names use CamelCase.

Member and static function names within a class/struct use CamelCase.

Member variables should begin with a lowercase m (which denotes it is a member variable) and then use CamelCase like mMemberVar.

class MyClass
{
public:
	int MemberFunction();
	static void StaticFunction();

private:
	bool mIsAlive = false;
};

Class variables (static) use CamelCase, with the exception of constants which follow the constant naming rules.

Function naming

clang-tidy

All functions, whether standalone or within a class/struct use CamelCase.

Function parameters use camelBack.

void MyFunction(int paramOne, int paramTwo);

Local variable naming

clang-tidy

All local variables use camelBack.

Constant naming

clang-tidy

Constants (with the exception of const references or pointers) should use UPPER_CASE, like:

const int WINDOW_WIDTH = 1024;
const int WINDOW_HEIGHT = 768;

Variables which are const references or pointers should instead use whatever the expected naming convention is for that type of variable.

Booleans

Code Review

For Boolean variables, try to use variable names which are a short phrase that can be answered with a yes/no question. For example, isAlive is a better name for a Boolean than alive.

Constants

Avoid magic numbers

Code Review

For example, rather than writing 1024 and 768 wherever you need to reference the width and height of the window, declare constants. This both makes the code easier to read, and allows for more flexibility if you later need to change the value. In general, any number which a reasonable developer would look at and wonder where it came from should be a constant.

Where to declare

Code Review

If your constant might reasonably be needed by another class (such as the window width/height example), you can declare it as a public static const member of a class. For example:

class Game
{
public:
	static const int WINDOW_WIDTH = 1024;
	static const int WINDOW_HEIGHT = 768;
};

This allows others to use the variables without needing an instance of the class, such as Game::WINDOW_WIDTH.

However, note that C++ only allows you to declare and assign static const class variables in the header for integer and enum types. This means that if you want to have a float or other type as a constant that’s static, you can’t assign it in the header. You have two options:

You can declare the static const in the header and assign it in the .cpp. This would look something like this:

// In MyClass.h
class MyClass
{
public:
	static const float MAX_SPEED;
};
  
// In MyClass.cpp
const float MyClass::MAX_SPEED = 400.0f;

You can declare the variable using static constexpr instead of static const, which does allow you to still assign it in the header:
```
class MyClass
{
public:
	static constexpr float MAX_SPEED = 400.0f;
};
```

If the constant is only needed inside the implementation of one particular class, you can make it a private variable in that class or declare it at the top of the .cpp file containing the implementation.

Use const, not #define

Code Review

Do not use #define to declare simple constants. Instead, use const. The #define directive should only be used for macros or preprocessor declarations.

Header Files

Implementations in headers

Code Review

Header files should only contain implementations of short (e.g. 1-2 line) functions. Keep in mind that a function which is not a class member function can only be implemented in a header if it is marked inline.

Templates do not have this restriction because templates which are declared in a header must also be implemented in the header.

Use `#pragma once` instead of include guards

Code Review

The first line of a header file should always be #pragma once. This protects you from erroneously including the same header file multiple times.

Since we are using #pragma once, do not use an “include guard” pattern.

Use forward declarations

Code Review

Whenever possible, use forward declarations to reduce the number of headers you need to include from another header.

For example, instead of this Player.h:

#pragma once
#include "Actor.h"
#include "Game.h"
#include "CollisionComponent.h"

class Player : public Actor
{
public:
	Player(Game* game);

private:
	CollisionComponent* mCollider = nullptr;
};

You can forward declare both CollisionComponent and Game. (You can’t forward declare Actor because the compiler needs to know what an Actor is to be able to subclass it). This way, you can avoid including both Game.h and CollisionComponent.h from Player.h.

One option for forward declarations is to use class in front of every use of the name:

#pragma once
#include "Actor.h"

class Player : public Actor
{
public:
	Player(class Game* game);

private:
	class CollisionComponent* mCollider = nullptr;
};

Alternatively, if you are going to reference the same forward-declared class multiple times in the same header file, you can instead forward declare like this:

#pragma once
#include "Actor.h"

class Game;
class CollisionComponent;

class Player : public Actor
{
public:
	Player(Game* game);

private:
	CollisionComponent* mCollider = nullptr;
};

Remember that forward declarations generally only work for class or struct, and you must be using it as a pointer or reference to a class or struct.

Other types (such as enums or typedefs) generally cannot be forward-declared.

Remember you DO still need to include the headers for the forward-declared classes in the .cpp file that uses them. In this example, Player.cpp would need to include Game.h and CollisionComponent.h

Conditionals

No empty blocks

Code Review

You should never have an if or else block with no code inside of it. For example, do not do this:

if (condition)
{
}
else
{
	SomeFunction();
}

Instead, the above code can be rewritten in this more succinct form:

if (!condition)
{
	SomeFunction();
}

No if and statement on same line

clang-format

Putting both the if and statement on the same line makes it more difficult to debug with breakpoints, as you can’t place the breakpoint “inside” of the if statement.

Do not do:

if (condition) SomeFunction();

Instead, place the function on the next line, indented properly:

if (condition)
	SomeFunction();

Single line blocks and indentation

clang-format

Braces are not required for single line blocks following an if or else (though you can use braces if you want to). However, you must indent the code properly to denote the scope, like:

if (condition)
	SomeFunction();
else
	OtherFunction();

Simplify Boolean expressions

clang-tidy

This is checked by clang-tidy’s readability-simplify-boolean-expr check. The general idea is that you should eliminate superfluous conditional checks when possible. For instance, a function like this:

bool IsPositive(int value)
{
	if (value > 0)
		return true;
	else
		return false;
}

Should instead be written as:

bool IsPositive(int value)
{
	return value > 0;
}

The other main idea is that Booleans should just be tested directly rather than compared to true/false. So, don’t write conditionals like this:

if (isAlive == true)
	DoSomething();

And instead write it as:

if (isAlive)
	DoSomething();

Test STL container emptiness with `empty()`

clang-tidy

This is checked by clang-tidy’s readability-container-size-empty check. When testing if a container is empty, don’t test if the container.size() == 0 and instead test if container.empty().

Avoid code duplication between conditions

Code Review

For example, this code has two function calls which occur in both the if and else block, which is an error-prone practice:

if (condition)
{
	FunctionOne();
	FunctionTwo();
	FunctionThree();
}
else
{
	FunctionOne();
	FunctionTwo();
	FunctionFour();
}

The above code should be refactored so that only the unique code is inside the conditions:

FunctionOne();
FunctionTwo();

if (condition)
	FunctionThree();
else
	FunctionFour();

Use `||` instead of duplicated blocks

Code Review

For example, instead of:

if (condition)
	DoSomething();
else if (otherCondition)
	DoSomething();

You should write this as:

if (condition || otherCondition)
	DoSomething();

Prefer `&&` instead of simple nested conditions

Code Review

For simple conditional checks, an if or similar generally should not be followed immediately by another nested if. This can be rewritten to use &&. For example, instead of:

if (condition)
{
	if (otherCondition)
		DoSomething();
}

You should write this as:

if (condition && otherCondition)
	DoSomething();

However, this is not a hard rule and can be a judgement call. For example, nesting the if may be better in a scenario where otherwise you have to repeat several of the conditions in an else if or if it means you are going to have 4 or 5 conditions in an complex && statement.

An `else` immediately followed by `if` should be an `else if`

Code Review

For example, instead of:

if (condition)
	DoSomething();
else
{
	if (otherCondition)
		SomethingElse();
}

You should write this as:

if (condition)
	DoSomething();
else if (otherCondition)
	SomethingElse();

Avoid using `std::string` for conditions

Code Review

Using std::string to test for conditions is inefficient. For example, suppose your AI has two states: idle and attacking. You should not store the state in a string and use checks like if (state == "idle") as this requires a slow string comparison.

Instead, you should prefer using an enum class to store the state (or, alternatively, an enum declared inside the scope of a class). This is because enums are compared as integers, which is much more efficient. An enum can also be used with a switch statement, whereas strings cannot.

Prefer switches to chained `else if`s

Code Review

If you have more than one else if chained in a row, use a switch statement if possible. switch statements are easier to read intent and can be better optimized by the compiler.

For example, instead of:

if (state == State::Idle)
	UpdateIdle(deltaTime);
else if (state == State::Patrol)
	UpdatePatrol(deltaTime);
else if (state == State::Attack)
	UpdatePatrol(deltaTime);
else if (state == State::Hide)
	UpdateHide(deltaTime);

You should write this as a switch statement:

switch (state)
{
case State::Idle:
	UpdateIdle(deltaTime);
	break;
case State::Patrol:
	UpdatePatrol(deltaTime);
	break;
case State::Attack:
	UpdateAttack(deltaTime);
	break;
case State::Hide:
	UpdateHide(deltaTime);
	break;
}

That being said, not every chain of else if statements can be rewritten as a switch, since switches are only allowed for integer or enum types.

Make switch case fall-through clear

Code Review

Keep in mind that in C++, the case does not end until it hits a break statement. This allows you to do case fall-through where the same code is reached on multiple cases. However, to make the case fall-through clear, you should only do this when you can put the cases on consecutive lines.

For example, this switch does not make clear there’s a fall-through, and should be avoided:

switch (state)
{
case State::Idle:
	UpdateIdle(deltaTime);
	break;
case State::Patrol:
	UpdatePatrol(deltaTime);
	break;
case State::Attack:
	if (condition)
		CheckSomething();
	// This fall-through is unclear!
case State::Hide:
	UpdateAttackOrHide(deltaTime);
	break;
}

However, this fall-through is clear because the case statements are on consecutive lines:

switch (state)
{
case State::Idle:
	UpdateIdle(deltaTime);
	break;
case State::Patrol:
	UpdatePatrol(deltaTime);
	break;
case State::Attack:
case State::Hide:
	UpdateAttackOrHide(deltaTime);
	break;
}

Loops

No loop keyword and statement on same line

clang-format

For example, do not write code like this:

while (condition) DoSomething();

Instead, you should put the body on a subsequent line as it makes it easier to debug with breakpoints:

while (condition)
	DoSomething();

Single line blocks and indentation

clang-format

Braces are not required for single line blocks following a loop keyword (though you can use braces if you want to). However, you must indent the code properly to denote the scope, like:

while (condition)
	DoSomething();

Regular for loops over a vector should use `size_t`

Code Review

When looping over a std::vector using subscripts, the correct type to use for the index variable is size_t, as that is the type returned by size(). Here’s an example:

for (size_t i = 0; i < v.size(); i++)
	v[i] = i * 2;

You can also use a range-based for loop for vectors, though you aren’t required to.

Use range-based `for` loops instead of iterators

Code Review

For containers which do not support random access (such as std::map), you either have to use iterators or a range-based for loops. In most cases, you should use a range-based for loop as it is more succinct.

For example, instead of:

// Map declared earlier
std::map<std::string, struct Mix_Chunk*> soundMap;

// Loop over map
for (auto iter = soundMap.begin(); iter != soundMap.end(); ++iter)
{
	SDL_Log("%s: %p", iter->first.c_str(), iter->second);
}

You should use:

// Map declared earlier
std::map<std::string, struct Mix_Chunk*> soundMap;

// Loop over map
for (const auto& kv : soundMap)
{
	SDL_Log("%s: %p", kv.first.c_str(), kv.second);
}

In most cases, the type of the variable should be either auto& if you intend to modify elements or const auto& if you do not intend to modify elements. Keep in mind that without the &, you are making a copy of the element, which is a common error when using a range-based for loop.

For containers such as std::vector, which can use subscripts to access elements, you can still use a regular for loop.

Try to avoid using `continue` and `break`

Code Review

In many cases, continue and break actually make the control flow longer and more difficult to understand, especially if a loop contains several of them. This is doubly the case when the loops are nested.

For example, in this case the continue is not really needed:

for (auto block : GetBlocks())
{
	if (block == this)
		continue;
	
	if (Intersect(block))
		block->SetState(ActorState::Destroy);
}

Instead, this code can be rewritten as:

for (auto block : GetBlocks())
{
	if (block != this && Intersect(block))
		block->SetState(ActorState::Destroy);
}

It is okay to use break or continue in cases where the loop is quite long and it would require a lot of indentation or convoluted logic to not use them. However, if you find that you are using continue or break statements everywhere, you are probably abusing them.

Variables

Avoid using global variables (except for constants)

Code Review

While there absolutely are legitimate uses for globals, generally you do not want to declare non-constant global variables. If you’re declaring global variables, take a moment to think whether they should be member variables, local variables, or possibly a static.

Only one declaration per line

clang-tidy

This is checked by clang-tidy’s readability-isolate-declaration check.

Don’t declare multiple variables on the same line. For example, instead of:

int x = 0, y = 0;

You should write this as:

int x = 0;
int y = 0;

Explicitly initialize all non-class variables

clang-tidy

This is checked by clang-tidy’s cppcoreguidelines-init-variables check.

Keep in mind that in C++, variables which are basic types or pointers are not guaranteed to be initialized to a sensible value. For example, in this code the value of x is undefined:

int x;

This means that x could be any arbitrary value.

To fix this code, initialize x to something:

int x = 0;

Class variables do not need to be explicitly initialized to anything as their default constructor is called. For example, you should not initialize a string like this:

std::string myString = "";

In this case, the = "" is unnecessary because the default constructor of myString already will construct it as an empty string, so in fact it’s less efficient to add the = "".

Use `auto` sparingly

Code Review

In general, you should only use auto in contexts such as saving a lambda, an iterator, or another complex type in a local variable. You can also use it for the variable of a range-based for loop. However, do not make all your variables auto (or auto &&) because it just makes the code more difficult to follow at a glance. In most cases, stick to using the actual type.

Remember that if you have a function that returns by reference and you use auto to save the returned value, you will make a copy instead of getting a reference.

Use temporary variables instead of repeat computations

Code Review

Instead of copy/pasting the same computation multiple times, you should save the result in a temporary.

For example, instead of:

if (Math::Acos(Vector3::Dot(facing, Vector3::UnitX)) < Math::ToRadians(30.0f))
{
	// Do something
	// ...
}
else if (Math::Acos(Vector3::Dot(facing, Vector3::UnitX)) < Math::ToRadians(45.0f))
{
	// Do something else
	// ...
}

It is both more efficient and more readable to instead write the code as:

float angle = Math::Acos(Vector3::Dot(facing, Vector3::UnitX));
if (angle < Math::ToRadians(30.0f))
{
	// Do something
	// ...
}
else if (angle < Math::ToRadians(45.0f))
{
	// Do something else
	// ...
}

The other advantage of using a variable is it makes it much easier to debug. In this example, you can place a breakpoint on the first if statement and are able to mouse over angle to see what value it has. Without the temporary, you have to manually step into each function call which is much more tedious.

Even in the case where you only are using angle once, saving it in a temporary makes the code more clear as you can quickly see that the intent of the code is to get an angle and then compare it.

Classes

Don’t make everything `public`

Code Review

Generally, only functions which need to be called by others should be public. Member variables should almost always be private or protected (but can have public getter/setter functions). Also, helper member functions that only need to be called inside the class should be private or protected. As for whether something should be private or protected, the decision should be based on whether you will need access in a subclass or not.

Avoid redundant access specifiers

clang-tidy

This is checked by clang-tidy’s readability-redundant-access-specifiers check.

If you have two consecutive sections of declarations in a class with the same access specifier, you only need to declare the access specifier once. For example, the second public: in this declaration is redundant:

class MyClass
{
public:
	void Function();

public:
	int mVar = 0;
};

Instead, you can write the above as:

class MyClass
{
public:
	void Function();

	int mVar = 0;
};

Most member functions should be implemented in the .cpp

Code Review

Generally, only short (1-2 line) member functions should be implemented within the class the declaration in the header. Longer functions should be implemented in the .cpp file.

For one-line member functions, if the declaration and implementation fits on one line, you do not need to put the brace on a new line. For example, the following is okay:

class MyClass
{
public:
	int GetZero() const { return 0; }
};

Explicitly initialize all non-class member variables

clang-tidy

This is checked by clang-tidy’s cppcoreguidelines-pro-type-member-init check.

Keep in mind that in C++, variables which are basic types or pointers are not guaranteed to be initialized to a sensible value.

For example, in this code mVar is uninitialized:

class MyClass
{
public:
	MyClass() {}

private:
	int mVar;
};

To initialize these basic type or pointer member variables, you have two options. You can initialize it at the declaration point, like this:

class MyClass
{
public:
	MyClass() {}

private:
	int mVar = 0;
};

Alternatively, you can initialize it in the constructor (ideally with the initializer list), like this:

class MyClass
{
public:
	MyClass()
	: mVar(0)
	{
	}

private:
	int mVar;
};

Generally, the constructor with initializer list will be implemented in the .cpp and not inline in the header as in the above example.

For non-basic class members, if you just want to default construct them, you don’t need to do anything as C++ will automatically default construct them. If you want to construct the class with a parameter you can use the initializer list or you can initialize the class at its declaration using the C++11 brace initialization syntax, like:

class MyClass
{
public:
	MyClass() { }

private:
	std::string mStr{"Hello!"};
};

If the class member you are constructing depends on a parameter passed to the constructor or requires some additional code to run, you should either use the initializer list or implement the initialization in the body of the constructor.

If a member variable is initialized both at declaration and in the initializer list, the initializer list takes precedence.

The order in which member variables are initialized is based on the order they are declared. The order they appear in the initializer list has no bearing on the order the variables get initialized.

In an initializer list, only initialize one variable per line

clang-format

This is automatically handled by clang-format, but generally it makes it more readable if each initialization is on a separate line.

Member functions which don’t modify member data should be `const`

clang-tidy

This is checked by clang-tidy’s readability-make-member-function-const check.

If a member function does not modify any member data, it should be marked as const to denote to callers that calling the function doesn’t modify the object. This is especially relevant in the case that someone is holding a const reference to an object and wants to still be able to call that function.

For example, in this code the GetZero function clearly does not modify member data:

class MyClass
{
public:
	int GetZero() { return 0; }
};

Thus, GetZero should be marked as const like:

class MyClass
{
public:
	int GetZero() const { return 0; }
};

Use `override` when overriding a virtual function

clang-tidy

This is checked by clang-tidy’s modernize-use-override check.

The override keyword prevents you from making a mistake where you intend to override a function from a base class, but accidentally do not. For example, in this case the Ball class doesn’t actually override Update but instead makes an entirely different Update because the parameters do not match:

class Actor
{
	virtual void Update(float deltaTime);
};

class Ball : public Actor
{
	virtual void Update();
};

If the Ball::Update function were declared with override it would fail to compile. Furthermore, the use of virtual on a function that’s already virtual in the parent class is redundant, so you should remove that.

The corrected version of above code is:

class Actor
{
	virtual void Update(float deltaTime);
};

class Ball : public Actor
{
	void Update(float deltaTime) override;
};

Writing Better/Modern C++

Delete blocks of commented code

Code Review

Commenting out code during testing makes a lot of sense, but once you finalize what code you want, you should delete any code that is commented out.

Don’t do `using namespace std;`

Code Review

This is an easy way to see that someone is a newbie C++ programmer. Namespaces exist to prevent your names from clashing with names in built-in C++ libraries. Especially because many game engines barely even use STL classes in their codebase, you don’t want to just take the entire namespace.

Generally, you should directly qualify any names in like std::string. However, if you really want to you can use specific members of std only in .cpp files (NOT in header files), for example if you say:

using std::string;

It makes it so that whenever you say string it knows you want the one in std.

Don’t use `this->` in front of class members

Code Review

Using this-> makes your code needlessly verbose. Since we already flag member variables with m in front of them, it is not necessary to use this-> to distinguish local variables from member variables. It also is needlessly verbose to use this-> to call member functions given that almost all functions in the program are likely to be member functions.

Use `nullptr` instead of `NULL` or `0`

Code Review

The nullptr keyword was added in C++11 as it is strongly-typed as a pointer (whereas NULL or 0 are first an integer). Use nullptr.

Use C++ style casts

clang-tidy

This is checked by clang-tidy’s google-readability-casting check.

For example, when casting from an integer to a float, don’t do:

float toFloat = (float)myInt;

Instead, you should use static_cast like:

float toFloat = static_cast<float>(myInt);

In most cases you should use static_cast. However, you should use dynamic_cast when you need to speculatively test whether some pointer is a pointer to a subclass. (That being said, if you know for sure something has to be of a certain type, you can still use static_cast). For the code you write in this course, you will not need to use reinterpret_cast or const_cast.

Prefer a type change to an excessive amount of casts

Code Review

For example, suppose you have a variable declared as an int but in almost every case, you need to use it as a float. Rather than doing a static_cast every time you need to use the variable as a float, it would be better to declare it as a float and then only cast to an int when absolutely needed.

Avoid unnecessary dynamic allocations

Code Review

Unlike in Java, you do not need to dynamically allocate every class instance with new. You should only dynamically allocate an object if it needs to exist beyond the scope of the function in which it is constructed.

Don’t check if a pointer is null before deleting

clang-tidy

This is checked by clang-tidy’s readability-delete-null-pointer check.

Deleting a null pointer is completely safe, so a null check is unnecessary.

Return STL container member variables by reference

Code Review

In the following code, the GetBlocks function below is woefully inefficient because it returns a copy of the mBlocks vector. Additionally, it means that you cannot actually modify the mBlocks vector through GetBlocks.

class Game
{
public:
	std::vector<class Block*> GetBlocks() { return mBlocks; }

private:
	std::vector<class Block*> mBlocks;
};

If it’s likely a caller needs to modify the mBlocks vector, you should change it so GetBlocks returns the vector by reference:

class Game
{
public:
	std::vector<class Block*>& GetBlocks() { return mBlocks; }

private:
	std::vector<class Block*> mBlocks;
};

However, if no caller will ever need to modify the vector (and instead just needs access), you can return a const reference and mark the function as const:

class Game
{
public:
	const std::vector<class Block*>& GetBlocks() const { return mBlocks; }

private:
	std::vector<class Block*> mBlocks;
};

Save STL containers returned-by-reference in a reference

Code Review

Suppose you have this correct version of GetBlocks:

class Game
{
public:
	std::vector<class Block*>& GetBlocks() { return mBlocks; }

private:
	std::vector<class Block*> mBlocks;
};

If some code later does something like:

std::vector<Block*> blocks = GetGame()->GetBlocks();

This is still creating a copy of the mBlocks vector because the local blocks variable is a value and not a reference.

To fix this, you need to instead declare blocks as a reference:

std::vector<Block*>& blocks = GetGame()->GetBlocks();

The same gotcha applies to auto. This version creates a copy:

auto blocks = GetGame()->GetBlocks();

This version avoids the copy:

auto& blocks = GetGame()->GetBlocks();

Pass STL containers by (`const`) reference

Code Review

Passing a container by value creates an unnecessary copy which can be very inefficient. In most cases, you should instead pass by const reference. You would remove the const in a case where the function actually needs to modify the container.

Instead of:

void MyFunction(std::vector<int> v)
{
	// Do something
	// ...
}

Use:

void MyFunction(const std::vector<int>& v)
{
	// Do something
	// ...
}

Prefer `enum class` or an `enum` declared inside a class to a global `enum`

Code Review

The problem with a global enum is it adds the enum members to the global namespace. For example, suppose you have a MouseState like:

enum MouseState
{
	Idle,
	FindCheese,
	Eat
};

If you later add a global enum for CatState and it, too, has an Idle state this causes a clash as Idle already exists in the global namespace.

One way to resolve this is by using an enum class like:

enum class MouseState
{
	Idle,
	FindCheese,
	Eat
};

With the use of enum class any references to members of the enum must be qualified with the name of the enum. For example, you would reference the idle state with MouseState::Idle. This means that if CatState is also declared as an enum class, the CatState::Idle name will not clash.

The second option is to declare the enum as a member of the Mouse class, for example:

class Mouse : public Actor
{
	enum State
	{
		Idle,
		FindCheese,
		Eat
	};
};

This places the enum member names within the scope of the Mouse class, so to access the idle state you would use Mouse::Idle to access it. This does mean that if Mouse has a function also named Idle it will still clash.

When you use which of these two options is up to you. Arguably enum class is slightly safer as it is strongly-typed to the enum and there can never be a name conflict.

Find a good balance of commenting

Code Review

It sometimes is difficult to find a good balance of comments. On the one hand, if you have a complex operation that may not be obvious on first glance, adding comments can be very helpful for readers of the code to later understand what’s happening.

On the other hand, being overly zealous with comments may actually make the code less readable, for example these comments are unnecessary:

// Initialize x to 0
int x = 0;
// Initialize y to 0
int y = 0;
// z is x plus y
int z = x + y;

Although this is an extreme example, if you find that you are writing lots of comments, you probably are putting too many.

Furthermore, be sure to remove the following pet peeve comments:

A // TODO comment after you’ve done the thing that the “TODO” was saying you should do
Old code that is no longer being used

Finally, we do not have a requirement for a specific comment naming style required for function declarations, and in most cases the expectation is that there does not need to be a comment preceding every function declaration.

Anti-patterns

An anti-pattern is a ubiquitous coding pattern that is counter-productive and leads to poorly maintainable code. Here are some common anti-patterns that you should avoid in this class.

Avoid copy-paste coding

Code Review

If you find that you are copy/pasting the same code into a different place, this almost always indicates there is a better way.

If the code you’re copy/pasting is into a different scope than the original one, this means that you should make a function that contains the code. Alternatively, if you’re copy/pasting a calculation into the same scope, it may mean you just need to make a variable to save the result of the computation to later reuse it.

Beyond the fact that it reduces clarify, copy-paste coding means that if you later discover there is an error in that code, you have to remember to fix it in all the places that same code was pasted.

Avoid spaghetti code

Code Review

This is a very subjective item, but in general spaghetti code is code where the control flow is excessively convoluted. For example, lots of conditionals (perhaps with nesting) or long loops or lots of use of continue/break. Basically, a level of complexity where a “reasonable developer” would look at the code and decide that the control flow is just too difficult to easily follow.

To fix spaghetti code, it may involve a combination of looking to see if you can simplify the control flow and/or split things up into functions to reduce the cognitive overhead of looking at the code.

ITP 380-Specific

These rules apply specifically to the ITP 380 codebase and/or libraries the codebase uses. These items all are only flagged in manual code review.

Use `float` not `double`

Code Review

The ITP 380 codebase, like most video game engines, uses float for all floating-point variables, and not double. Although doubles have increased precision and can support a larger range of value, they take up double the memory. Games typically do not need the extra precision and range of a double, so using one is just a waste of memory.

Use provided math library functions

Code Review

The provided math library performs most if not all the math operations you should need in your code. For example, this code that manually normalizes a vector is unnecessary:

Vector3 v(1.0f, 1.0f, 1.0f);
float length = sqrtf(v.x * v.x + v.y * v.y + v.z * v.z);
v.x /= length;
v.y /= length;
v.z /= length;

Instead, you can use the provided Normalize member function:

Vector3 v(1.0f, 1.0f, 1.0f);
v.Normalize();

Manually calculating these operations is error prone, and additionally makes the code unnecessarily verbose.

As an addendum, you should prefer the standalone provided math library functions over the ones STL. For example, instead of using cos you should use Math::Cos.

You can also use Math::ToRadians and Math::ToDegrees to convert angles and you can use some predefined constants such as Math::Pi and Math::PiOver2.

Use `SDL_Log` instead of `printf` or `cout`

Code Review

The SDL_Log function is automatically compiled out in a release builds whereas printf and cout are not. Furthermore, SDL_Log is far more efficient than cout is.

Don’t add separate component pointers to `Actor`

Code Review

Even though many actors do have a CollisionComponent, you should not add a CollisionComponent pointer as a member variable of the Actor base class and create the component in the constructor. This defeats the purpose of the component model, because the point is that not every actor needs a CollisionComponent and forcing that in the best case is inefficient and in the worst case will cause bugs.

Use `GetComponent` sparingly

Code Review

Calling GetComponent has some overhead cost associated with it. As a good rule of thumb, you should avoid calling GetComponent for the same component multiple times per frame.

For example, suppose the following code is in Player::Update. It calls GetComponent on the player’s CollisionComponent once per every iteration, when the component is not changing:

for (auto block : mGame->GetBlocks())
{
	if (GetComponent<CollisionComponent>()->Intersects(block->GetComponent<CollisionComponent>()))
	{
		// Do something
		// ...
	}
}

Minimally, the above code should be changed to something like this, which calls GetComponent for the player’s collision only once before the loop:

CollisionComponent* playerColl = GetComponent<CollisionComponent>();
for (auto block : mGame->GetBlocks())
{
	if (playerColl->Intersects(block->GetComponent<CollisionComponent>()))
	{
		// Do something
		// ...
	}
}

An improvement over this is to declare a member variable, like mPlayerColl and save a pointer to that once when the CollisionComponent is created, and then you will not have to call GetComponent for the player’s collision at all anymore:

for (auto block : mGame->GetBlocks())
{
	if (mPlayerColl->Intersects(block->GetComponent<CollisionComponent>()))
	{
		// Do something
		// ...
	}
}

Do not call `GetMinOverlap` multiple times for the same pair

Code Review

GetMinOverlap is considered a repeat computation as per the rule about temporaries. However, there is a further problem with this because every call to GetMinOverlap modifies the offset variable passed in. This is a common enough poor practice that it deserves its own rule.

Here is an example of what you should NOT do:

for (auto block : GetGame()->GetBlocks())
{
	Vector2 offset;
	if (playerColl->GetMinOverlap(block->GetCollision(), offset) == CollSide::Top)
	{
		// Do something for top
		// ...
	}
	else if (playerColl->GetMinOverlap(block->GetCollision(), offset) == CollSide::Bottom)
	{
		// Do something for bottom
		// ...
	}
	else if (playerColl->GetMinOverlap(block->GetCollision(), offset) == CollSide::Left ||
					 playerColl->GetMinOverlap(block->GetCollision(), offset) == CollSide::Right)
	{
		// Do something for side
		// ...
	}
}

Instead, you should write this code as:

for (auto block : GetGame()->GetBlocks())
{
	Vector2 offset;
	CollSide side = playerColl->GetMinOverlap(block->GetCollision(), offset);
	if (side == CollSide::Top)
	{
		// Do something for top
		// ...
	}
	else if (side == CollSide::Bottom)
	{
		// Do something for bottom
		// ...
	}
	else if (side == CollSide::Left || side == CollSide::Right)
	{
		// Do something for side
		// ...
	}
}

This first avoids the waste of calling GetMinOverlap four times for every block that doesn’t intersect, and it also makes the code much easier to debug. Note the correct approach also allows you to use a switch instead of the else if chain, if it makes sense to (in this example, either way is acceptable).

Style Guide

Formatting

Line length

Use tabs

Braces

Indentation

Spacing and parenthesis

Blank lines

Floating-point literals

Naming

General naming rules

File names

Class naming

Function naming

Local variable naming

Constant naming

Booleans

Constants

Avoid magic numbers

Where to declare

Use const, not #define

Header Files

Implementations in headers

Use #pragma once instead of include guards

Use forward declarations

Conditionals

No empty blocks

No if and statement on same line

Single line blocks and indentation

Simplify Boolean expressions

Test STL container emptiness with empty()

Avoid code duplication between conditions

Use || instead of duplicated blocks

Prefer && instead of simple nested conditions

An else immediately followed by if should be an else if

Avoid using std::string for conditions

Prefer switches to chained else ifs

Make switch case fall-through clear

Loops

No loop keyword and statement on same line

Single line blocks and indentation

Regular for loops over a vector should use size_t

Use range-based for loops instead of iterators

Try to avoid using continue and break

Variables

Avoid using global variables (except for constants)

Only one declaration per line

Explicitly initialize all non-class variables

Use auto sparingly

Use temporary variables instead of repeat computations

Classes

Don’t make everything public

Avoid redundant access specifiers

Most member functions should be implemented in the .cpp

Explicitly initialize all non-class member variables

In an initializer list, only initialize one variable per line

Member functions which don’t modify member data should be const

Use override when overriding a virtual function

Writing Better/Modern C++

Delete blocks of commented code

Don’t do using namespace std;

Don’t use this-> in front of class members

Use nullptr instead of NULL or 0

Use C++ style casts

Prefer a type change to an excessive amount of casts

Avoid unnecessary dynamic allocations

Don’t check if a pointer is null before deleting

Return STL container member variables by reference

Save STL containers returned-by-reference in a reference

Pass STL containers by (const) reference

Prefer enum class or an enum declared inside a class to a global enum

Find a good balance of commenting

Anti-patterns

Avoid copy-paste coding

Avoid spaghetti code

ITP 380-Specific

Use float not double

Use provided math library functions

Use SDL_Log instead of printf or cout

Don’t add separate component pointers to Actor

Use `#pragma once` instead of include guards

Test STL container emptiness with `empty()`

Use `||` instead of duplicated blocks

Prefer `&&` instead of simple nested conditions

An `else` immediately followed by `if` should be an `else if`

Avoid using `std::string` for conditions

Prefer switches to chained `else if`s

Regular for loops over a vector should use `size_t`

Use range-based `for` loops instead of iterators

Try to avoid using `continue` and `break`

Use `auto` sparingly

Don’t make everything `public`

Member functions which don’t modify member data should be `const`

Use `override` when overriding a virtual function

Don’t do `using namespace std;`

Don’t use `this->` in front of class members

Use `nullptr` instead of `NULL` or `0`

Pass STL containers by (`const`) reference

Prefer `enum class` or an `enum` declared inside a class to a global `enum`

Use `float` not `double`

Use `SDL_Log` instead of `printf` or `cout`

Don’t add separate component pointers to `Actor`

Use `GetComponent` sparingly

Do not call `GetMinOverlap` multiple times for the same pair