C++ beginner wrote Conway's game of life using SFML

Question

I'm an intermediate Java programmer trying to learn the basics of C++.

Thus, I decided to write "Conway's game of life" as my second C++ program (I already wrote a Mandelbrot image generator). It was a fun exercise but I struggled a lot with structuring my program and dealing with pointers and references.

It's a bit of a mishmash because I wanted to try to learn how to use both regular pointers and smart pointers, and I also implemented the Grid class to practice how to use templates and lambdas.

Never used SFML, never written a game. Never written a game loop so I don't really understand what I have done and all the documentation out there about game loops was difficult for me to understand.

All in all, it was a fun experience and please give me tips on structure, improvements, what was good, what was bad, should I give up and take up another hobby?

I would really like to become a better C++ developer.

Git repository for source code

main.cpp

int main()
{
 Game game(2048, 1024, "Game Of Life");
 game.run();
}

Game.cpp

#include <SFML/System/Clock.hpp>
#include <SFML/Graphics.hpp>
#include <time.h>
#include <iostream>
#include "LifeState.hpp"
#include "Game.hpp"
const int Game::FPS = 25;
const int Game::SKIP_TICKS = 1000 / FPS;
Game::Game(const int width, const int height, const std::string title) {
 this->data->window.create(sf::VideoMode(width, height), title);
 this->data->assets.loadTexture("tile", "assets/tile.png");
 this->data->assets.loadTexture("tile2", "assets/tile2.png");
 this->lifeState.init(this->data);
};
void Game::run(){
 int nextGameTick = clock.getElapsedTime().asMilliseconds();
 struct timespec tim, tim2;
 tim.tv_sec = 0;
 tim.tv_nsec = 0;
 while (this->data->window.isOpen()) {
 updateGame();
 displayGame();
 nextGameTick += SKIP_TICKS;
 tim.tv_nsec = (nextGameTick - clock.getElapsedTime().asMilliseconds());
 if(tim.tv_nsec >= 0){
 nanosleep(&tim, &tim2);
 }
 }
}
void Game::updateGame(){
 this->lifeState.update();
 sf::Event event;
 while (this->data->window.pollEvent(event))
 {
 if (event.type == sf::Event::Closed) {
 this->data->window.close();
 }
 if(event.type == sf::Event::MouseButtonPressed) {
 auto mouse_pos = sf::Mouse::getPosition(this->data->window);
 sf::Vector2<float> translated_pos = this->data->window.mapPixelToCoords(mouse_pos);
 this->lifeState.toggle(translated_pos);
 std::cout << "mouse clicked at: " << event.mouseButton.x << " " << event.mouseButton.y << std::endl;
 }
 if(event.type == sf::Event::KeyReleased) {
 if (event.key.code == sf::Keyboard::Space) {
 if(lifeState.isGenerating) {
 this->lifeState.stop();
 } else {
 this->lifeState.start();
 }
 }
 }
 }
}
void Game::displayGame(){
 this->data->window.clear(sf::Color::Black);
 this->lifeState.draw();
 this->data->window.display();
}

AssetManager.cpp

#include "AssetManager.hpp"
void AssetManager::loadTexture(std::string name, std::string filename) {
 sf::Texture texture;
 if(texture.loadFromFile(filename)) {
 this->textures[name] = texture;
 }
}
sf::Texture *AssetManager::getTexture(std::string name) {
 return &this->textures.at(name);
}

LifeState.cpp

#include <memory>
#include <iostream>
#include "LifeState.hpp"
#include <SFML/System/Clock.hpp>
void LifeState::init(GameDataRef &data) {
 this->data = data;
 auto size = this->data->assets.getTexture("tile")->getSize();
 int width = this->data->window.getSize().x / size.x;
 int height = this->data->window.getSize().y / size.y;
 auto boolean = [](int x, int y, int height, int width) { return false; };
 this->currentState.fill(height, width, boolean);
 this->nextState.fill(height, width, boolean);
 int posX = 0;
 int posY = 0;
 sf::Texture* tile = this->data->assets.getTexture("tile");
 this->sprites.fill(height, width, [tile, posX, posY](int x, int y, int height, int width) mutable {
 sf::Sprite sprite;
 sprite.setTexture(*tile);
 sprite.setTextureRect(sf::IntRect(0, 0, tile->getSize().x, tile->getSize().y));
 sprite.setPosition(posX, posY);
 posX += tile->getSize().x;
 if(y == width-1) {
 posY += tile->getSize().y;
 posX = 0;
 }
 return sprite;
 });
 this->lastTime = 0;
};
void LifeState::toggle(sf::Vector2<float> translated_pos) {
 this->sprites.forEach([&translated_pos, this](sf::Sprite sprite, int x, int y){
 if(sprite.getGlobalBounds().contains(translated_pos)) {
 if(this->currentState.get(x, y)) {
 this->currentState.set(x, y, false);
 } else {
 this->currentState.set(x, y, true);
 }
 }
 });
}
void LifeState::draw() {
 sf::Texture* tile = this->data->assets.getTexture("tile");
 sf::Texture* tile2 = this->data->assets.getTexture("tile2");
 sprites.forEach([this, tile, tile2](sf::Sprite sprite, int x, int y){
 if(this->currentState.get(x, y)) {
 sprite.setTexture(*tile2);
 } else {
 sprite.setTexture(*tile);
 }
 this->data->window.draw(sprite);
 });
}
void LifeState::start() {
 std::cout << "Started simulation" << std::endl;
 isGenerating = true;
}
void LifeState::stop() {
 std::cout << "Stopped simulation" << std::endl;
 isGenerating = false;
}
void LifeState::update() {
 if(isGenerating) {
 double time_counter = 0;
 int thisTime = clock.getElapsedTime().asSeconds();
 time_counter += (double)(thisTime - this->lastTime);
 if(time_counter > 0)
 {
 currentState.forEach([this](bool value, int x, int y) {
 const int neighbours = this->getNeighbours(x, y);
 this->updateCell(x, y, neighbours);
 });
 this->currentState = nextState;
 }
 this->lastTime = thisTime;
 }
}
int LifeState::getNeighbours(const int i, const int j) {
 int neighbours = 0;
 if(i == this->currentState.sizeX()-1 && j == currentState.sizeY(i)-1) {
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(currentState.get(i-1, j-1)) {
 neighbours++;
 }
 if(currentState.get(i, j-1)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i == this->currentState.sizeX()-1 && j > 0) {
 if(this->currentState.get(i, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i == this->currentState.sizeX()-1 && j == 0) {
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i == 0 && j == 0) {
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i == 0 && j == this->currentState.sizeY(i)-1) {
 if(this->currentState.get(i, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i == 0 && j > 0) {
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j-1)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i > 0 && j == 0) {
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i > 0 && j == this->currentState.sizeY(i)-1) {
 if(this->currentState.get(i-1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 return neighbours;
 }
 if(i > 0 && j > 0) {
 if(this->currentState.get(i-1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j+1)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j)) {
 neighbours++;
 }
 if(this->currentState.get(i+1, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i, j-1)) {
 neighbours++;
 }
 if(this->currentState.get(i-1, j-1)) {
 neighbours++;
 }
 return neighbours;
 }
 return neighbours;
}
void LifeState::updateCell(const int height, const int width, const int neighbours) {
 const bool isActive = this->currentState.get(height, width);
 if(neighbours < 2 && isActive) {
 this->nextState.set(height, width, false);
 return;
 } else if((neighbours == 2 || neighbours == 3) && isActive) {
 this->nextState.set(height, width, true);
 return;
 } else if(neighbours > 3 && isActive) {
 this->nextState.set(height, width, false);
 return;
 } else if(isActive == false && neighbours == 3) {
 this->nextState.set(height, width, true);
 return;
 }
}

Grid.hpp

#pragma once
#include <vector>
#include <functional>
#include <iostream>
template<typename T>
class Grid {
private:
 std::vector<std::vector<T>> data;
public:
 void fill(int x, int y, const std::function<T(int, int, int, int)> &func);
 std::size_t sizeX();
 std::size_t sizeY(int x);
 void forEach(const std::function<void(T, int, int)> &func);
 T get(int x, int y);
 void set(int x, int y, T value);
};
template<typename T>
void Grid<T>::fill(int x, int y, const std::function<T(int, int, int, int)> &func) {
 data.reserve(x);
 for (int i = 0; i < x; i++) {
 std::vector<T> v;
 v.reserve(y);
 data.push_back(v);
 std::cout << y << std::endl;
 for (int j = 0; j < y; j++) {
 T value = func(i, j, data.size(), y);
 this->data.at(i).push_back(value);
 }
 }
}
template<typename T>
std::size_t Grid<T>::sizeX() {
 return data.size();
}
template<typename T>
std::size_t Grid<T>::sizeY(int x) {
 return data.at(x).size();
}
template<typename T>
void Grid<T>::forEach(const std::function<void(T, int, int)> &func) {
 for (int i = 0; i < data.size(); i++) {
 for (int j = 0; j < data.at(i).size(); j++) {
 func(data.at(i).at(j), i, j);
 }
 }
}
template<typename T>
T Grid<T>::get(int x, int y) {
 return data.at(x).at(y);
}
template<typename T>
void Grid<T>::set(int x, int y, T value) {
 data.at(x).at(y) = value;
}

I have omitted some of the header files, but you can find them all in the source code repository.

• Toerktumlare: oh wow didn't even notice that, i have changed it to include.
• L. F.: Take a look at The Definitive C++ Book Guide and List on Stack Overflow.

Answer 1

Welcome to C++! Java and C++ are quite different languages with different programming idioms and techniques, so hopefully this review will help you get better in C++.

main.cpp

The first thing I noticed is that you omitted the #include when posting your main.cpp code. I checked your GitHub repository and noticed that #include "Game.hpp" is present in the main.cpp file. Please post complete code in the future.

When I see the line

Game game(2048, 1024, "Game Of Life");

I wonder which of the two numbers is the width and which is the height (if they are supposed to be dimensions at all) and what the "Game Of Life" string means. You didn't post your Game.hpp file, but I go ahead and check it. There is a constructor:

Game(const int width, const int height, const std::string title);

which answers the questions. It would be better if a reader does not need to examine another file to understand the code. In this case, a simple comment is sufficient:

// creates a 2048 x 1024 game with title "Game Of Life"
Game game(2048, 1024, "Game Of Life");

This approach has drawbacks: the comment is invalidated when the code changes, and comments shouldn't be used to indicate how the code works. For more complicated cases, the named argument idiom is commonly used.

Game.cpp

You didn't actually post Game.hpp. I can check your code on GitHub, but this answer can only review Game.cpp.

You included <time.h>. C headers of the form <time.h> are deprecated in favor of <ctime>. In fact, you should be using <chrono> instead of <ctime> in C++.

const int Game::FPS = 25;
const int Game::SKIP_TICKS = 1000 / FPS;

These two lines are (probably) definitions of static const variables. They can be made constexpr and defined directly in-class. ALL_CAPS identifiers are usually reserved for macros, and constants shouldn't really use them. In C++, types are used to assign values meanings. Make a type alias fps_t for the type of fps:

using fps_t = int;

Also, skip_ticks seems to be a time duration instead of a pure integer, so use chrono:

// in the Game class declaration
static constexpr fps_t fps = 25;
static constexpr auto skip_ticks = 1000ms / fps; 

(Assumes using namespace std::chrono_literals.) 1000ms is unambiguously 1000 milliseconds instead of 1000 microseconds or 1000 Minecraft ticks.

this-> has limited use and is redundant in your case, so leave them out. This applies to all functions.

Your constructor declares all parameters as const. This is not necessary. Also, title should be passed by std::string_view.

The run function is where the C time facilities become a bit unhandy with timespec and pointers. Here's what the same code looks like when written using C++ chrono facilities: (just an idea, not tested because I am not familiar with SFML clocks)

void Game::run()
{
 std::chrono::milliseconds nextGameTick{clock.getElapsedTime().asMilliseconds()};
 while (data->window.isOpen()) {
 updateGame();
 displayGame();
 nextGameTick += skip_ticks; // the units are handled properly by the type system
 std::chrono::milliseconds current_time{clock.getElapsedTime().asMilliseconds()};
 std::this_thread::sleep_for(nextGameTick - current_time); // more intuitive than timespec and nanosleep
 }
}

This needs #include <chrono> and #include <thread> (for sleep_until).

The updateGame function uses a bunch of ifs. At least they should be made else if to avoid redundant checking. And the right tool here is switch:

switch (event.type) {
case sf::Event::Closed:
 // ...
 break;
case sf::Event::MouseButtonPressed:
 // ...
 break;
case sf::Event::KeyReleased:
 // ...
 break;
}

Your use of endl is justified because flushing is required here. This may deserve a comment.

AssetManager.cpp

Both functions should pass the strings by string_view instead of string by value. And it may be more convenient for getTexture to return a reference instead of a pointer.

LifeState.cpp

Several of your lambdas can be simplified with default captures. For example, [=] instead of [tile, posX, posY], and [&] instead of [&translated_pos, this]. The lambda in draw can also be [&] if you make getTexture return a reference as recommended above, or make tile and tile2 references.

The process of assigning the game data should probably be in the constructor of the LifeState class instead of the init function. Don't name the parameter identically to the data member. boolean is not a useful name for the lambda. always_false is better.

The update function can similarly be rewritten using chrono.

In the getNeighbours function, use prefix ++ instead of postfix ++. Replace all neighbours++ with ++neighbours. And the logic should probably be simplified or organized somehow ...

Grid.hpp

Generic templates are not easy to get right. You definitely shouldn't be implementing Grid yourself in this project. Use an existing library instead.

A vector<vector<T>> doesn't feel good. A library should use contiguous memory internally.

You are using .at all over the place. Use [] when you have control over the index to reduce performance penalty.

fill should be a constructor, and should accept any callable object. std::function shouldn't be used here at all. And in this case, the use of std::endl is inherently wrong — you don't need to flush every single time and '\n' is the way to go. int also doesn't feel right here. And your implementation is needlessly complex. Here's a basic proofread (not tested):

template <typename T, typename F>
Grid<T>::Grid(size_type x, size_type y, F f)
 :data(x)
{
 for (size_type i = 0; i < x; ++i) {
 data[i].reserve(y);
 std::cout << y << '\n';
 for (size_type j = 0; j < y; ++j) {
 data[i].push_back(f(i, j, i + 1, y)); // the arguments are guessed
 }
 }
}

where size_type is declared beforehand with

using size_type = typename decltype(data)::size_type;

I don't see how sizeY should be taking an argument. The grid is supposed to be rectangular, right?

The get and set functions also feel a bit anti-C++-ish. get should be made const. I'd expect this in C++:

template <typename T>
T& Grid<T>::operator()(size_type x, size_type y)
{
 return data.at(x).at(y);
}
template <typename T>
const T& Grid<T>::operator()(size_type x, size_type y) const
{
 return data.at(x).at(y);
}

This way, we can do grid(x, y) to get and grid(x, y) = value to set.

Again, there are too many ways in which this feels unidiomatic and please use an existing library instead of rolling out your own.

• Toerktumlare: this is excellent, thank you for all your comments!
• Konrad Rudolph: I strongly disagree that a comment improves the constructor call. In fact, it makes it worse because it gives the reader false confidence. In reality, the comment could be completely wrong (e.g. width & height switched) and the reader would be none the wiser. This comment is a typical, extreme anti-pattern: It’s the cause of numerous hard to find bugs. Unfortunately there are no good and simple solutions to this issue. The best is to use strong typing with a named-argument idiom, and write auto game = Game{width{2048}, height{1024}, title{"foobar"}};.
• L. F.: @KonradRudolph I also prefer the named argument approach. In this case, the constructors are not gonna be reused anyway, and since OP is relatively new to C++, that may be too much for him I guess. I'll update my answer to incorporate this information.
• Toerktumlare: i looked into the "named-argument idiom" and that is what we call the "builder-pattern" in Java! nice to see that this pattern is also used in C++
• Konrad Rudolph: @ThomasAndolf Just to be clear, there are multiple completely different implementations of that idiom, and the one I was referring to isn’t the builder pattern (I’d call that builder pattern in C++!) and I strongly recommend against using it. I’d call that an anti-pattern in C++ because it makes it impossible to have const objects. In C++, an object should be be fully initialised once the constructor has finished running. The same is true in Java — builders should always generate a different type, such as StringBuilder, which is used to build an immutable String. This usage is OK.