Conway's game of life in C++ using SDL2

Question

I finally finished my Conway's game of life in c++ project and i'd love to find out how to improve the code.

main.cpp

#include "Engine/Engine.h"
int main()
{
 Conway::Engine Engine(1280, 1024);
 Engine.Run();
 return 0;
}

Engine.h

#pragma once
#include <SDL2/SDL.h>
#include <memory>
#include "Board.h"
// Namespace Conway to
// always enclose classes in a namespace.
namespace Conway
{
 // The engine class handles
 // everything about the program:
 // It handles the window, the renderer,
 // Drawing, Events, Game logic,
 // and holds representations of the game's
 // parts (Cell, Grid)
 class Engine
 {
 public:
 Engine(int ScreenWidth, int ScreenHeight);
 ~Engine(); 
 void Run();
 private:
 void HandleEvents();
 void Draw();
 void DrawLines();
 const int m_ScreenWidth;
 const int m_ScreenHeight;
 bool m_Update = false;
 bool m_Running = true;
 std::unique_ptr<Board> m_Board;
 SDL_Window* m_Window;
 SDL_Renderer* m_Renderer;
 };
}

Engine.cpp

#include "Engine.h"
Conway::Engine::Engine(int ScreenWidth, int ScreenHeight)
 : m_ScreenWidth{ScreenWidth}, m_ScreenHeight{ScreenHeight}
{
 SDL_assert(SDL_Init(SDL_INIT_VIDEO) >= 0);
 m_Window = SDL_CreateWindow(
 "Conway's game of life",
 SDL_WINDOWPOS_UNDEFINED,
 SDL_WINDOWPOS_UNDEFINED,
 m_ScreenWidth,
 m_ScreenHeight,
 SDL_WINDOW_SHOWN 
 );
 SDL_assert(m_Window != NULL);
 if (!m_Board)
 {
 Coord<int, int> ScreenSize = {m_ScreenWidth, m_ScreenHeight};
 m_Board = std::make_unique<Board>(ScreenSize);
 }
 SDL_assert(m_Board != nullptr);
 m_Renderer = SDL_CreateRenderer(m_Window, -1, SDL_RENDERER_ACCELERATED|SDL_RENDERER_PRESENTVSYNC);
 SDL_assert(m_Renderer != NULL);
 // Show lines
 SDL_RenderClear(m_Renderer);
 DrawLines();
}
Conway::Engine::~Engine()
{
 SDL_DestroyWindow(m_Window);
 SDL_DestroyRenderer(m_Renderer);
 m_Window = NULL;
 m_Renderer = NULL;
 SDL_Quit();
}
void Conway::Engine::HandleEvents()
{
 SDL_Event Event;
 while(SDL_PollEvent(&Event))
 {
 switch(Event.type)
 {
 case SDL_QUIT:
 m_Running = false;
 break;
 // Toggles the updating with a keypress
 case SDL_KEYDOWN:
 if (Event.key.keysym.sym == SDLK_SPACE)
 {
 m_Update = m_Update ? false : true;
 DrawLines();
 }
 else if (Event.key.keysym.sym == SDLK_c)
 {
 m_Board->Clear();
 }
 break;
 case SDL_MOUSEBUTTONDOWN:
 if (!m_Update)
 {
 if (Event.button.button == SDL_BUTTON_LEFT)
 {
 m_Board->ToggleClickedCell({Event.button.x, Event.button.y});
 }
 }
 break;
 }
 }
}
// Draws the grid
void Conway::Engine::Draw()
{
 SDL_RenderClear(m_Renderer);
 for (int i = 0; i < Board::GRID_HEIGHT; ++i)
 {
 for (int j = 0; j < Board::GRID_WIDTH; ++j)
 {
 if (m_Board->ReadCell(j + Board::GRID_WIDTH * i) == Board::Cell::Alive)
 {
 SDL_SetRenderDrawColor(m_Renderer, 255, 255, 255, 255);
 }
 else 
 {
 SDL_SetRenderDrawColor(m_Renderer, 0, 0, 0, 255);
 }
 SDL_Rect rect;
 rect.x = m_Board->GetCellSize().first * j;
 rect.y = m_Board->GetCellSize().second * i;
 rect.w = m_Board->GetCellSize().first;
 rect.h = m_Board->GetCellSize().second;
 SDL_RenderFillRect(m_Renderer, &rect);
 }
 }
 if (!m_Update)
 {
 DrawLines();
 }
 SDL_RenderPresent(m_Renderer);
}
// This function draws
// the lines delimiting each cell.
// The first loop draws the horizontal
// lines, the second one the vertical lines.
void Conway::Engine::DrawLines()
{
 SDL_SetRenderDrawColor(m_Renderer, 255, 255, 255, 255);
 for (int i = 0; i < Board::GRID_HEIGHT; ++i)
 {
 if (i != 0)
 {
 SDL_RenderDrawLine(
 m_Renderer,
 0,
 m_Board->GetCellSize().second * i,
 m_Board->GetCellSize().first * Board::GRID_WIDTH,
 m_Board->GetCellSize().second * i
 );
 }
 }
 for (int i = 0; i < Board::GRID_WIDTH; ++i)
 {
 if (i != 0)
 {
 SDL_RenderDrawLine(
 m_Renderer,
 m_Board->GetCellSize().first * i,
 0,
 m_Board->GetCellSize().first * i,
 m_Board->GetCellSize().second * Board::GRID_HEIGHT
 );
 }
 }
 SDL_RenderPresent(m_Renderer);
 SDL_SetRenderDrawColor(m_Renderer, 0, 0, 0, 255);
} 
// Main game loop
void Conway::Engine::Run()
{
 while (m_Running)
 {
 HandleEvents();
 if (m_Update)
 {
 m_Board->Update();
 }
 Draw();
 SDL_Delay(100);
 }
}

Board.h

#pragma once
#include <vector>
#include "Coord.h"
namespace Conway
{
 class Board
 {
 public:
 Board(Coord<int, int> ScreenSize);
 static constexpr int GRID_WIDTH = 80;
 static constexpr int GRID_HEIGHT = 60;
 Coord<int, int> GetCellSize() { return m_CellSize; }
 void ToggleClickedCell(Coord<int, int> MouseCoords);
 void Update();
 void Clear();
 enum class Cell
 {
 Dead,
 Alive
 };
 private:
 int CountAliveNeighbors(Coord<int, int> GridCell);
 std::vector<Cell> m_Grid;
 const Coord<int, int> m_CellSize;
 public:
 Cell ReadCell(int Index) { return m_Grid[Index]; }
 };
}

Board.cpp

#include "Board.h"
#include <cmath>
Conway::Board::Board(Coord<int, int> ScreenSize)
 : m_CellSize{ScreenSize.first / GRID_WIDTH, ScreenSize.second / GRID_HEIGHT}
{
 int GridSize = GRID_WIDTH * GRID_HEIGHT;
 std::vector<Cell> temp(GridSize, Cell::Dead);
 m_Grid = temp;
}
void Conway::Board::Clear()
{
 std::fill(m_Grid.begin(), m_Grid.end(), Cell::Dead);
}
int Conway::Board::CountAliveNeighbors(Coord<int, int> GridCell)
{
 int count = 0;
 for (int i = -1; i < 2; ++i)
 {
 for (int j = -1; j <2; ++j)
 {
 int absoluteX = GridCell.first + i;
 int absoluteY = GridCell.second + j;
 if (absoluteX == -1 || absoluteX == GRID_WIDTH ||
 absoluteY == -1 || absoluteY == GRID_HEIGHT ||
 (i == 0 && j == 0))
 {
 continue;
 }
 if (m_Grid[absoluteX + GRID_WIDTH * absoluteY] == Cell::Alive)
 {
 ++count;
 }
 }
 }
 return count;
}
// Inverses the cell that was clicked on
void Conway::Board::ToggleClickedCell(Coord<int, int> Coords)
{
 int ClickedCell = (floor(Coords.first / m_CellSize.first)) + GRID_WIDTH * (floor(Coords.second / m_CellSize.second));
 m_Grid[ClickedCell] = m_Grid[ClickedCell] == Cell::Dead ? Cell::Alive : Cell::Dead;
}
void Conway::Board::Update()
{
 std::vector<Cell> temp(m_Grid);
 for (int i = 0; i < Board::GRID_HEIGHT; ++i)
 {
 for (int j = 0; j < GRID_WIDTH; ++j)
 {
 if (m_Grid[j + GRID_WIDTH * i] == Cell::Alive)
 {
 if (CountAliveNeighbors({j, i}) < 2 || CountAliveNeighbors({j, i}) > 3)
 {
 temp[j + GRID_WIDTH * i] = Cell::Dead;
 }
 }
 else
 {
 if (CountAliveNeighbors({j, i}) == 3)
 {
 temp[j + GRID_WIDTH * i] = Cell::Alive;
 }
 }
 }
 }
 m_Grid = temp;
}

Coord.h

#pragma once
template <typename T1, typename T2>
struct Coord
{
 T1 first;
 T2 second;
};

Answer 1

The code looks well organized, and has a clear coding style. Good separation of responsibility between classes, almost no raw pointers (except for those coming from the C API of SDL of course), and no global variables. Nice! But there are still some areas of improvement:

Only use SDL_assert() to check for programming errors

Assertions are a tool to help find bugs in your program. However, in release builds, these assertions are typically compiled out. Thus, they should not be used to check for errors that can reasonably happen. For example:

SDL_assert(m_Window != NULL);

It is very possible that, without any bugs in your program, an SDL window could not be created, for example because of an out of memory condition, or the program being run without a display server running. So instead, you have to use a regular if-statement to check for this condition, and then handle the error appropriately. You could use exceptions for that, like so:

#include <stdexcept>
...
if (!m_Window)
{
 throw std::runtime_error("Failed to create window");
}

Use nullptr instead of NULL

NULL should be used in C code, in C++ you should use nullptr. However, you can also avoid writing it entirely in most cases. For example, instead of if (foo != nullptr), you can just write if (foo). Also, instead of Foo *foo = nullptr you can write Foo *foo = {}.

Whether you want to use nullptr explicitly or use the shorter notations is up to the code style you are using.

Avoid unnecessary indirection

One of things you do in the constructor of Engine is to allocate a new instance of Board and store the pointer in m_Board. But why allocate this way, when you can just store a Board directly in Engine, like so:

class Engine {
 ...
 private:
 Board m_Board;
};

The constructor should then ensure it initializes it like so:

Conway::Engine::Engine(int ScreenWidth, int ScreenHeight)
 : m_ScreenWidth{ScreenWidth}, m_ScreenHeight{ScreenHeight}
 , m_Board({ScreenWidth, ScreenHeight})
{
 ...

Don't draw in the constructor of Engine

It should not be necessary to call Draw() from the constructor, instead this is done in Run(). In general, avoid having functions do more than necessary.

Don't reset member variables in the destructor

There is no pointing in setting m_Window and m_Renderer to NULL in the destructor of Engine, since those variables will be gone as soon as the function exits.

Add a default statement to the switch in HandleEvents

Be explicit and tell the compiler what behaviour you want if Event.type doesn't match any of the case-statements. Otherwise, when enabling warnings, the compiler might warn about unhandled event types. It just has to be:

default:
 break;

Improve class Coord

Your class Coord is basically the same as std::pair. So, if you really wanted to have coordinate pairs where each coordinate can have its own type, you should just have written std::pair<int, int> instead of Coord<int, int>. However, in your code you always use ints for coordinates. So there really is no need for a template at all. Furthermore, you clearly want x and y-coordinates, so just make that explicit:

struct Coord
{
 int x;
 int y;
};

Be consistent in how you name things. In your code, you use i, first and somethingX as names for variables related to the x coordinate. Make sure it has x in the name everywhere. Also, do use your class Coord wherever you have a pair of coordinate. Here is how it would look:

int Conway::Board::CountAliveNeighbors(Coord GridCell)
{
 int count = 0;
 for (int dx = -1; dx <= 1; ++dx)
 {
 for (int dy = -1; dy <= 1; ++dy)
 {
 Coord absolute;
 absolute.x = GridCell.x + dx;
 absolute.y = GridCell.y + dy;
 ...

Don't use arbitrary delays

You are calling SDL_Delay(100), which limits your code to run at less than 10 frames per second. Maybe you want to have the evolution of the board go at a rate of 10 Hz, but it is in general better to decouple rendering from the timesteps of your simulation. You already set the SDL_RENDERER_PRESENTVSYNC flag, so you can drop the call to SDL_Delay() and have your code render at the same framerate as your monitor.

If you want to limit how often the board updates, then I suggest you use SDL_GetTicks() to keep track of time, and only call Update() when enough time has passed.

Pass coordinate pairs to ReadCell()

The fact that class Board stores cells as a one-dimensional std::vector should not have to be exposed to other classes. So it is better if ReadCell() takes x and y-coordinates in the form of a Coord, and converts them to an index itself, so in Engine::Draw() you can write:

if (m_Board.ReadCell({x, y}) == Board::Cell::Alive)

Rename ToggleClickedCell() to ToggleCell()

You have a very good separation of responsibility in your code: class Board implements the logics of the board, while class Engine handles user input and rendering. This makes it easy to change the Engine while keeping the functionality of the Board the same. For example, you could make a text-only version of your program by changing Engine such that it would not use SDL but render the board as ASCII art for example. In that case, you would not use a mouse but the keyboard to toggle cells, so it would be strange to have to call ToggleClickedCell() when no clicking is involved.

You should also just pass the grid x and y coordinates to ToggleCell(), not the mouse coordinates. Converting mouse coordinates to grid coordinates should be done by Engine.

Make member functions const where appropriate

Apart from variables, you can also make member functions const. You should do this when the member function doesn't change any of the member variables of its class. That allows the compiler to optimize the code better. You just have to add it right after the declaration in the header files, like so:

int CountAliveNeighbors(Coord GridCell) const;

Avoid repeatedly using a function to get the same value

In Board::Update() there are three calls to CountAliveNeighbors({j, i}). Apart from the code duplication, if the compiler cannot see that each call will produce exactly the same result, it will perform more function calls than necessary. While there are ways to make the compiler optimize this anyway (using function attributes like [[gnu::const]] or link-time optimization), you can easily improve the code yourself by calling the function once and storing the result in a variable:

auto aliveNeigbors = CountAliveNeighbors({x, y});
if (ReadCell({x, y}) == Cell::Alive)
{
 if (aliveNeighbors < 2 || aliveNeighbors > 3)
 {
 ...

Keep two vectors of cells in memory

In Board::Update(), you create a temporary std::vector<Cell>, write the new cell state to it, and at the end copy the temporary vector into m_Grid, and then you destroy the temporary. If this was something you would only do sporadically, that could be fine, but this is where your program spents a large part of its time, so you should try to optimize this. A simple way to do this is to keep two vectors for storage, and a variable to keep track of the "current" vector. For example, in Board.h:

class Board
{
 ...
 private:
 std::vector<Cell> m_Grids[2];
 int m_CurrentGrid = 0;
};

Then, in Update(), do something like:

auto &m_Grid = m_Grids[m_CurrentGrid]; // Get a reference to the current grid
auto &temp = m_Grids[m_CurrentGrid ^ 1]; // Get a reference to the temporary grid
for (...)
{
 ...
}
m_CurrentGrid ^= 1; // Swap the temporary and current grid

Of course, everywhere you used m_Grid before, you have to ensure you use the current grid. This makes it even more important to use a member function to get cell at a given coordinate, instead of reading a vector directly, even inside class Board itself, because then you only need one place where you put the logic which of the two vectors to read.

• S.S. Anne: I've had many an SDL application crash on me because of SDL_assert(m_Window != NULL);. Good advice.