SFML snake game in C++

Question

I made a snake game in sfml and i'm kind of proud of the code's structure, but proud doesn't mean it's good, so i'm putting it here so that if there is something that could be bettered, you would know.

main.cpp

#ifndef UNICODE
#define UNICODE
#endif
#include "app.h"
int main() {
 app game(800, 600, L"Test");
 game.start();
 game.end();
}

app.h

#pragma once
#include <SFML/Graphics.hpp>
#include "Snake.h"
#include "Board.h"
class app {
public:
 app(int windowWidth, int windowHeight, const wchar_t* name);
 ~app() = default;
 // Runs the app
 void start();
 void end();
private:
 // MEMBER VARIABLES
 const int winWidth, winHeight;
 int score;
 bool play;
 sf::RenderWindow window;
 Snake snake;
 Board board;
 // MEMBER FUNCTIONS
 // Draws the objects
 void drawWindow();
 // Handles events
 void handleEvents();
 // Updates the window
 void updateWindow();
};

app.cpp

#include "app.h"
#include <iostream>
#include <thread> 
#include <chrono>
app::app(int windowWidth, int windowHeight, const wchar_t* name)
 : winWidth{ windowWidth }, winHeight{ windowHeight }, score{ 0 },
 play{ false } {
 while (true) {
 int choice;
 std::wcout << L"Choose: " << std::endl;
 std::wcout << L"1: Play " << std::endl;
 std::wcout << L"2: Quit " << std::endl;
 std::cin >> choice;
 if (choice == 1) {
 play = true;
 break;
 }
 else break;
 }
 // Clears screen
 for (size_t i = 0; i < 10; ++i)
 std::wcout << L"\n\n\n\n\n\n\n\n\n\n\n\n" << std::endl;
 if (play) {
 window.create(sf::VideoMode(winWidth, winHeight), name);
 window.setFramerateLimit(5);
 }
}
// Handles any game event
void app::handleEvents() {
 sf::Event event;
 while (window.pollEvent(event)) {
 switch (event.type) {
 case sf::Event::Closed:
 window.close();
 break;
 case sf::Event::TextEntered:
 snake.changeDirection(static_cast<char>(event.text.unicode));
 }
 }
}
// Draws all game objects
void app::drawWindow() {
 for (size_t i = 0, h = board.height(); i < h; ++i) {
 for (size_t j = 0, w = board.width(); j < w; ++j) {
 // Draws walls
 if (board[i * w + j] == 2) {
 sf::RectangleShape rect;
 rect.setSize({ static_cast<float>(board.divisor()), static_cast<float>(board.divisor()) });
 rect.setPosition({ static_cast<float>(board.divisor() * j), static_cast<float>(board.divisor() * i)});
 window.draw(rect);
 }
 // Draws snake
 else if (board[i * w + j] == 3) {
 sf::RectangleShape rect;
 rect.setFillColor(sf::Color::Green);
 rect.setSize({ static_cast<float>(board.divisor()), static_cast<float>(board.divisor()) });
 rect.setPosition({ static_cast<float>(board.divisor() * j), static_cast<float>(board.divisor() * i) });
 window.draw(rect);
 }
 // Draws food
 else if (board[i * w + j] == 4) {
 sf::RectangleShape rect;
 rect.setFillColor(sf::Color::Red);
 rect.setSize({ static_cast<float>(board.divisor()), static_cast<float>(board.divisor()) });
 rect.setPosition({ static_cast<float>(board.divisor() * j), static_cast<float>(board.divisor() * i) });
 window.draw(rect);
 }
 }
 }
}
// Updates the render window
void app::updateWindow() {
 window.clear(sf::Color::Black);
 drawWindow();
 window.display();
}
// Starts the app
void app::start() {
 while (window.isOpen()) {
 handleEvents();
 snake.move();
 board.update(window, snake, &score);
 updateWindow();
 }
}
void app::end() {
 if (play) {
 std::wcout << L"You lose!" << std::endl;
 std::wcout << L"Score: " << score << std::endl;
 std::this_thread::sleep_for((std::chrono::milliseconds)3000);
 }
}

Snake.h

#pragma once
#include <SFML/Graphics.hpp>
#include <vector>
class Snake {
public:
 Snake();
 ~Snake() = default;
 // Changes the dir value based on the input
 void changeDirection(char input);
 // Adds a piece to the snake
 void add();
 // Returns the size of snakeContainer
 size_t getSnakeSize();
 // Moves the snake
 void move();
private:
 // MEMBER VARIABLES 
 struct Snake_segment
 {
 int xPos, yPos, prevxPos, prevyPos;
 };
 const enum direction {
 UP = 0,
 RIGHT,
 DOWN,
 LEFT
 };
 std::vector<Snake_segment> snakeContainer;
 direction dir;
 // MEMBER FUNCTIONS
 // Makes the segments follow the head
 void follow();
 // Moves the snake's head
 void moveHead();
public:
 // Operator overloading (i wasn't able to declare it in .cpp file)
 Snake_segment operator[](int i) { return snakeContainer[i]; }
};

Snake.cpp

#include "Snake.h"
// Initializes a two-piece snake
Snake::Snake()
 : dir { RIGHT } {
 Snake_segment head { 10, 7, 9, 7 };
 snakeContainer.push_back(head);
 --head.xPos;
 snakeContainer.push_back(head);
}
void Snake::add() {
 Snake_segment newSegment;
 newSegment.xPos = snakeContainer[snakeContainer.size() - 1].prevxPos;
 newSegment.yPos = snakeContainer[snakeContainer.size() - 1].prevyPos;
 snakeContainer.push_back(newSegment);
}
size_t Snake::getSnakeSize() {
 return snakeContainer.size();
}
// Changes the direction based on input
void Snake::changeDirection(char input) {
 switch (input) {
 case 'w':
 if (dir != DOWN) dir = UP;
 break;
 case 'd':
 if (dir != LEFT) dir = RIGHT;
 break;
 case 's':
 if (dir != UP) dir = DOWN;
 break;
 case 'a':
 if (dir != RIGHT) dir = LEFT;
 }
}
// All the pieces follow the head
void Snake::follow() {
 for (size_t i = 1, n = snakeContainer.size(); i < n; ++i) {
 snakeContainer[i].prevxPos = snakeContainer[i].xPos;
 snakeContainer[i].prevyPos = snakeContainer[i].yPos;
 snakeContainer[i].xPos = snakeContainer[i - 1].prevxPos;
 snakeContainer[i].yPos = snakeContainer[i - 1].prevyPos;
 }
}
// Moves the snake's head
void Snake::moveHead() {
 snakeContainer[0].prevxPos = snakeContainer[0].xPos;
 snakeContainer[0].prevyPos = snakeContainer[0].yPos;
 switch (dir) {
 case UP:
 --snakeContainer[0].yPos;
 break;
 case RIGHT:
 ++snakeContainer[0].xPos;
 break;
 case DOWN:
 ++snakeContainer[0].yPos;
 break;
 case LEFT:
 --snakeContainer[0].xPos;
 }
}
// Moves the snake
void Snake::move() {
 moveHead();
 follow();
}

Board.h

#pragma once
#include <SFML/Graphics.hpp>
#include "Snake.h"
class Board {
public:
 Board();
 ~Board() = default;
 void update(sf::RenderWindow& win, Snake& snek, int* scor);
 int width() const;
 int height() const;
 int divisor() const;
 char operator[](int i) const;
private:
 // MEMBER VARIABLES
 std::string map;
 const size_t mapWidth = 20;
 const size_t mapHeight = 15;
 // Is used to divide the screen in a grid
 const int common_divisor = 40;
 // MEMBER FUNCTIONS
 // Checks if snek has collided with something
 void genFood();
 void checkCollisions(sf::RenderWindow& win, Snake& snek, int* scor);
};

Board.cpp

#include "Board.h"
#include <random>
Board::Board() {
 // Creates a 20x15 grid
 map = {
 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2,
 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2
 };
 /*
 REMINDER:
 1 = FREE SPACE
 2 = WALL
 3 = SNAKE
 4 = FOOD
 */
 genFood();
}
void Board::genFood() {
 int fx, fy;
 do {
 std::random_device gen;
 std::uniform_int_distribution<int> disX(1, mapWidth - 1);
 std::uniform_int_distribution<int> disY(1, mapHeight - 1);
 fx = disX(gen);
 fy = disY(gen);
 } while (map[fy * mapWidth + fx] != 1);
 map[fy * mapWidth + fx] = 4;
}
void Board::update(sf::RenderWindow& win, Snake& snek, int* scor) {
 checkCollisions(win, snek, scor);
 // Iterates through the whole map
 for (size_t i = 0; i < mapHeight; ++i) {
 for (size_t j = 0; j < mapWidth; ++j) {
 // Makes walls
 if (i == 0 || i == mapHeight - 1) map[i * mapWidth + j] = 2;
 else if (j == 0 || j == mapWidth - 1) map[i * mapWidth + j] = 2;
 // Sets free space
 else if (map[i * mapWidth + j] != 4) map[i * mapWidth + j] = 1;
 // Sets snek
 for (size_t k = 0, n = snek.getSnakeSize(); k < n; ++k) {
 if (snek[k].yPos == i && snek[k].xPos == j)
 map[i * mapWidth + j] = 3;
 }
 }
 }
}
void Board::checkCollisions(sf::RenderWindow& win, Snake& snek, int* scor) {
 for (size_t i = 0; i < mapHeight; ++i) {
 for (size_t j = 0; j < mapWidth; ++j) {
 // Checks snek and wall collisions
 if (map[snek[0].yPos * mapWidth + snek[0].xPos] == 2 ||
 map[snek[0].yPos * mapWidth + snek[0].xPos] == 3 ) win.close();
 // Checks snek and food collisions
 else if (map[snek[0].yPos * mapWidth + snek[0].xPos] == 4) {
 map[snek[0].yPos * mapWidth + snek[0].xPos] = 1;
 snek.add(); 
 *scor += 100;
 genFood();
 }
 }
 }
}
int Board::width() const { return mapWidth; }
int Board::height() const { return mapHeight; }
int Board::divisor() const { return common_divisor; }
char Board::operator[](int i) const { return map[i]; }

Answer 1

Here are some things that may help you improve your code.

Don't declare enum const

In snake.h, the direction enum is declared as const but this is an error, since only functions and objects can be declared const.

Use const where practical

The Snake::getSnakeSize() doesn't alter the underlying Snake and so it should be declared const. Additionally, I'd name it size() to be consistent with standard library functions.

Simplify your code

The current Snake::add() code is this:

void Snake::add() {
 Snake_segment newSegment;
 newSegment.xPos = snakeContainer[snakeContainer.size() - 1].prevxPos;
 newSegment.yPos = snakeContainer[snakeContainer.size() - 1].prevyPos;
 snakeContainer.push_back(newSegment);
}

However, it could be simplified into a single line:

void Snake::add() {
 snakeContainer.push_back({
 snakeContainer.back().prevxPos, 
 snakeContainer.back().prevyPos, 
 snakeContainer.back().prevxPos, 
 snakeContainer.back().prevyPos, 
 });
}

Similarly, the follow code could be simplified by using iterators.

void Snake::follow() {
 auto it = snakeContainer.begin();
 for (auto prev = it++; it != snakeContainer.end(); ++it, ++prev) {
 it->prevxPos = it->xPos;
 it->prevyPos = it->yPos;
 it->xPos = prev->prevxPos;
 it->yPos = prev->prevyPos;
 }
}

In both of these case, futher simplification could be obtained by introducing a struct Coord { unsigned x, y; };

void Snake::follow() {
 auto it = snakeContainer.begin();
 for (auto prev = it++; it != snakeContainer.end(); ++it, ++prev) {
 it->prev = it->curr;
 it->curr = prev->prev;
 }
}

Use static constexpr for class constants

The current code has internal const variables for the width and height and then wrapper accessors, but this is much simplified by simply using public static constexpr variables and no wrapper. That's assuming you have a C++11 compiler. If not, the next best thing would be plain const and no wrapper.

Reconsider the class interfaces

Mostly the classes make sense to me, but it seems that the score should actually be maintained by the Board class and then returned to a caller on request via a const method. Also, it seems that divisor should be calculated and stored by the app class as a float. This would remove a lot of ugly static_casts as well. Also, it may make sense for the Board to own the Snake.

Add helper functions for clarity

I would advise converting from a comment to an enum or an enum class and then using that.

enum Tile { Open = 1, Wall, Body, Food };

Next, I'd suggest using helper functions to make the code easier to read and understand. For example:

bool Board::isEmpty(Coord coord) const {
 return at(coord) == Open;
}
bool Board::place(Coord coord, int item) {
 if (item != Open && !isEmpty(coord)) {
 return false;
 }
 map[coord.y * width + coord.x] = item;
 return true;
}
int Board::at(Coord coord) const {
 return map[coord.y * width + coord.x];
}

Here's the corresponding update function.

void Board::update(sf::RenderWindow& win) {
 auto newHead{snake.moveHead()};
 place(snake.follow(), Open);
 switch (at(snake.headLocation())) {
 case Wall: 
 case Body: 
 win.close();
 break;
 case Food: 
 place(snake.headLocation(), Open);
 place(snake.add(), Body);
 m_score += 100;
 genFood();
 }
 place(newHead, Body);
}

With this, note that there is no longer any need to loop through all coordinates and no need for a separate collision detection routine. Also, move is eliminated in favor of the two distinct calls that were in it. In this rewrite, moveHead() returns the location of the new head, and follow() returns the old location of the last segment. Since those are the only two nodes of the snake that change from one iteration to the next, those are the only two cells that need updating.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl. Also, you can make things a little neater. Instead of this:

 std::wcout << L"You lose!" << std::endl;
 std::wcout << L"Score: " << score << std::endl;
I would recommend writing it like this: 
 std::wcout << L"You lose!\nScore: " << score << '\n';

Don't overuse std::random_device

For some implementations, std::random_device is actually driven by a hardware-based generator and the quality of the generated random numbers may actually drop precipitously if too many random numbers are drawn too quickly. For that reason, it's better not to overuse std::random_device. Instead, seed a pseudorandom generator (PRG) once from std::random_device and then use the PRG. Here's a rewrite of the genFood() routine that does just that:

void Board::genFood() {
 static std::random_device rd;
 static std::mt19937 gen(rd());
 static std::uniform_int_distribution<unsigned> disX(1, width - 2);
 static std::uniform_int_distribution<unsigned> disY(1, height - 2);
 while (!place({disX(gen), disY(gen)}, Food))
 { /* keep attempting until it works */ }
}

Think of the user

How often does it happen that the user starts a game, only to immediately ask to quit? It seems unlikely to me, so I would eliminate the Play/Quit prompt entirely. Future enhancements that might be nice would be to display the score and length of snake as the game is being played.

• Nadpher: Thanks. I never used constexpr, since i really don't understand how it works. anyways i prompt the user play/quit at the start in case the run of the program was a mistake/ something the user didn't want to do, but now that i think about it yeah, you're right, it's pretty useless.
• Nadpher: Anyways, i'd like to know why i should make the variables public, without getters and setters. I've always known public variables are a bad practice. Just curious
• Edward: Public variables are only bad practice when external access to them is not required or when unrestricted external write access could allow an invariant violation. Because external read-only access is required and because const enforces read-only access, making these public is the best way to write it.

Answer 2

Some observations:

• Comments:

  // Draws the objects
  void drawWindow();
  // Handles events
  void handleEvents();
  // Updates the window
  void updateWindow();
  

  These comments don't tell you anything that the code doesn't already tell you, so they should be removed. Ideally the code is well named, like in that example, so you don't feel the need to even write a comment.

  Annother advice with that is. If you feel the need, in a long function, to comment several parts because they do something different it's probably a good idea to extract the parts into its own functions. Erase the Comments and makes the code self documentary.

• Namespaces:

  You should always put your functions and classes into namespaces to avoid name clashes.

• unicode

  #ifndef UNICODE
  #define UNICODE
  #endif
  

  is this still needed?

• std::endl:

  if you only want a newline you should replace std::endl with '\n'.

  std::endl also does a expensive flush operation which is rarely desired.

• Nadpher: Really i defined unicode since i've seen on multiple occasions that unicode is now the standard and writing unicode literals is good practice, on Windows docs too. That's why i always define unicode
• Snowbody: Pretty sure all compilers today define it for you, you don't need to define it yourself.

Answer 3

while (true) {

Some advise using for (;;) instead, but this loop really isn't necessary. But you don't even have a loop here; you'll exit the loop immediately no matter what is entered. Also this throws an exception if you enter a non-int.

Clearing the terminal should be done in a separate routine in case you want to switch to a terminal library like curses. (Also terminal is not guaranteed to be 24x80)

Try not to loop over the whole grid. See if you can avoid clearing and redrawing everything. Just make the necessary changes. When drawing the walls, think of a different encoding so you don't have to loop over the whole grid.

"divisor" is a bad name for the size of a grid cell.

snake color should be a parameter rather than a literal. Also the snake should draw itself, and drawWindow should call it.

In SnakeContainer types, use a bit more encapsulation, include xpos and ypos together in a struct pos to cut down on duplicated code..

Shouldn't the snake be a member of the board, not a parameter?