An implementation of Conway's Game of Life using C++ and SFML

Question

I recently started learning C++ again, and I wrote a simple version of Conway's Game of Life. This version doesn't use 2 buffers to transfer changes over to the main view. Instead, it uses a stack of changes that are applied then drawn.

I'd like some feedback on how to improve my code.

Controls are:
Space - Start/Stop the simulation
E - Clear the board when simulation is stopped
R - Randomize the board when simulation is stopped
Left Click - Toggle cells when simulation is stopped

What it should look like:

The Code:
main.cpp

#include "gol.hpp"
#include <SFML/Graphics.hpp>

int main() {
    const int boardWidth = 80;
    const int boardHeight = 60;
    const int cellWidth = 10;
    const int cellHeight = 10;
    bool doUpdate = false;

    sf::RenderWindow window(sf::VideoMode(boardWidth*cellWidth, boardHeight*cellHeight), "Game Of Life");
    window.setFramerateLimit(30);

    GameOfLife game(boardWidth,boardHeight,cellWidth,cellHeight);

    while (window.isOpen()) {
        sf::Event event;
        while (window.pollEvent(event)) {
            switch (event.type) {
                case sf::Event::Closed:
                    window.close();
                    break;
                case sf::Event::KeyReleased:
                    if (event.key.code == sf::Keyboard::Space) {
                        doUpdate = !doUpdate;
                    }
                    else if (event.key.code == sf::Keyboard::E) {
                        game.m_clearCells();
                    }
                    else if (event.key.code == sf::Keyboard::R) {
                        game.m_randomizeCells();
                    }
                    break;
                case sf::Event::MouseButtonPressed:
                    if (!doUpdate && event.mouseButton.button == sf::Mouse::Left) {
                        int x = (double)event.mouseButton.x / cellWidth;
                        int y = (double)event.mouseButton.y / cellHeight;
                        if (x >= 0 && x <= boardWidth && y >= 0 && y <= boardHeight) {
                            game.m_flipCell(x, y);
                        }
                    }
                    break;
                default:
                    break;
            }
        }
        window.clear();
        if (doUpdate) {
            game.m_updateCells();
            game.m_drawCellChanges(window);
        }
        else {
            game.m_drawCells(window);
        }
        window.display();
    }
    return 0;
}

gol.hpp

#ifndef GOL_HPP_
#define GOL_HPP_

#include <stack>
#include <utility>
#include <SFML/Graphics.hpp>

typedef struct {
    int live;
    sf::RectangleShape shape;
} Cell;

typedef std::pair<int, int> CellChange;
typedef std::stack<CellChange> CellChanges;

class GameOfLife {
    Cell* cells;
    CellChanges changes;
    int width, height, size;

    int n_getIndex(int x, int y);
    int n_countNeighbors(int x, int y);
    int n_wrapValue(int v, int max);

public:
    GameOfLife(int width_, int height_, int cellWidth, int cellHeight);
    ~GameOfLife();

    void m_drawCells(sf::RenderWindow& window);
    void m_drawCellChanges(sf::RenderWindow& window);
    void m_updateCells();
    void m_flipCell(int x, int y);
    void m_randomizeCells();
    void m_clearCells();
};

#endif

gol.cpp

#include <cstdlib>
#include <ctime>
#include "gol.hpp"

int GameOfLife::n_wrapValue(int v, int max) {
    if (v == -1) return max - 1;
    return v % max;
}

int GameOfLife::n_getIndex(int x, int y) {
    return y * width + x;
}

int GameOfLife::n_countNeighbors(int x, int y) {
    int neighbors = 0;
    // Don't count the current cell
    neighbors -= cells[n_getIndex(x, y)].live;
    for (int oy = -1; oy < 2; oy++) {
        for (int ox = -1; ox < 2; ox++) {
            // Wrap the coordinates around the board
            int cx = n_wrapValue(x + ox, width);
            int cy = n_wrapValue(y + oy, height);
            neighbors += cells[n_getIndex(cx, cy)].live;
        }
    }
    return neighbors;
}

void GameOfLife::m_flipCell(int x, int y) {
    cells[n_getIndex(x, y)].live = !cells[n_getIndex(x, y)].live;
}

void GameOfLife::m_randomizeCells() {
    // Randomize the contents of the cells
    // Generate a random number(1-10), if the number is even, the cell is alive, else the cell is dead.
    srand(time(nullptr));
    for (int i = 0; i < size; i++) {
        cells[i].live = (rand() % 10) % 2 == 0;
    }
}

void GameOfLife::m_clearCells() {
    for (int y = 0; y < height; y++) {
        for (int x = 0; x < width; x++) {
            int i = n_getIndex(x, y);
            cells[i].live = false;
        }
    }
}

GameOfLife::GameOfLife(int width_, int height_, int cellWidth, int cellHeight) {
    size = width_ * height_;
    width = width_;
    height = height_;

    cells = new Cell[size];
    
    // Set the positions of the cells in the grid to screen position.
    // Set the parameters of the cells for drawing
    m_randomizeCells();
    for (int y = 0; y < height; y++) {
        for (int x = 0; x < width; x++) {
            int i = n_getIndex(x, y);
            cells[i].shape.setPosition(sf::Vector2f(x*cellWidth, y*cellHeight));
            cells[i].shape.setSize(sf::Vector2f(cellWidth, cellHeight));
            cells[i].shape.setFillColor(cells[i].live ? sf::Color::White : sf::Color::Black);
        }
    }
}

GameOfLife::~GameOfLife() {
    delete[] cells;
}

void GameOfLife::m_drawCells(sf::RenderWindow& window) {
    for (int i = 0; i < size; i++) {
        // Correct the color of the cell
        cells[i].shape.setFillColor(cells[i].live ? sf::Color::White : sf::Color::Black);
        window.draw(cells[i].shape);
    }
}

void GameOfLife::m_drawCellChanges(sf::RenderWindow& window) {
    // m_drawCells must be called at least once before this so we can properly see the board
    // Apply changes to the cells and draw those changes
    while (!changes.empty()) {
        CellChange change = changes.top();
        changes.pop();
        Cell& cell = cells[change.first];
        cell.live = change.second;
        cell.shape.setFillColor(cell.live ? sf::Color::White : sf::Color::Black);
        window.draw(cell.shape);
    }
}

void GameOfLife::m_updateCells() {
    for (int y = 0; y < height; y++) {
        for (int x = 0; x < width; x++) {
            int i = n_getIndex(x, y);
            int n = n_countNeighbors(x, y);
            int isLive = cells[i].live;
            int state = isLive;
            
            if (isLive && (n < 2 || n > 3)) state = false;
            else if (n == 3) state = true;
            
            // Count the cells that changed and cells that stay alive. 
            // The ones that stayed alive are drawn so we can see the patterns that never die
            if (state != isLive || (state && isLive)) {
                changes.push(std::make_pair(i, state));
            }
        }
    }
}

Answer 1

General Observations

The code is generally pretty good, the design of the game can be improved.

There are 2 ways to improve the design of the game, first separate the display of the game from the logic of the game. This has already been done to some extent, but it can be improved using either the Model View Viewmodel (MVVM) design pattern or the Model View Controller (MVC) design pattern. There should be a class that is independent of the actual implementation of the display that hides the details of the display. This allows you to use different display software to display the game with the logic of the game remaining intact. You could have a class called MyInoutOutoutClass that handles the actual input and output and communicate with it either to a controller or through the view model modelview relationship.

This would mean that the include for SFML does not need to be included in main().

The other design improvement would be to move the while loop in main() into a function in the GameOfLife class in a function possibly called run() or rungame(). In the main() function itself, just init the display system and the game of life.

It is interesting to note that there is no use of std::size_t as an index or or size value.

Order of Private Versus Public

It is generally considered a best practice to put the public interfaces first, then the protected interfaces and the the private data and interfaces. This is true in both the header file and the source file. The reason for this is that when you are working with others on a team the consumers (users) of the public interfaces can quickly find the public interfaces to put it in their code. This allows rapid parallel development in a software team environment.

Keep Private Data Private

There is no reason that Cell typedef, the CellChange typedef and the CellChanges typedef are declared in public space, these are all private to the game of life. There are 2 ways to deal with this, the typedefs can be within the GameOfLife class, or there could be forward declarations in the header file and the actual definitions of the struct could be in the gol.cpp file.

class GameOfLife {
public:
    GameOfLife(int width_, int height_, int cellWidth, int cellHeight);
    ~GameOfLife();

    void m_drawCells(sf::RenderWindow& window);
    void m_drawCellChanges(sf::RenderWindow& window);
    void m_updateCells();
    void m_flipCell(int x, int y);
    void m_randomizeCells();
    void m_clearCells();

private:
    typedef struct {
        bool live;
        sf::RectangleShape shape;
    } Cell;

    typedef std::pair<int, int> CellChange;
    typedef std::stack<CellChange> CellChanges;

    Cell* cells;
    CellChanges changes;
    int width, height, size;

    int n_getIndex(int x, int y);
    int n_countNeighbors(int x, int y);
    int n_wrapValue(int v, int max);

};

Choice of int Versus bool

It isn't clear why the live field in the Cell struct is an int rather than a bool, the code clearly treats it as a boolean value and assignments to it are true and false. I changed it in my version and it removes a couple of warning messages.

Memory Allocation of Cells Versus Using an STL Container Class

It isn't clear why the code allocates a C style array of cells rather than using std::vector to contain a variable array of cells. Using STL containers lets C++ do the work rather than your code, and iterators can sometimes improve the performance. This might also reduce the need for a destructor (a default one could be used instead).

Random Number Generators

In the current implementation the function srand() should be called in main() and rand() should continue to be called in m_randomizeCells(). The srand() function should only be called once per execution of the program. As pointed out by J_H generating a number between 1 and 10 is an interesting limitation, you could generat and number between 1 and size.

In general in C++ there are better random number generators than the rand() function. The srand() and rand() functions are part of the standard C library and there have been improvements since they were written. The following example is from a discussion of C++ random number generators on stack overflow.

#include <random>
#include <iostream>

int main()
{
    std::random_device dev;
    std::mt19937 rng(dev());
    std::uniform_int_distribution<std::mt19937::result_type> dist6(1,6); // distribution in range [1, 6]

    std::cout << dist6(rng) << std::endl;
}