7
\$\begingroup\$

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];         }
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Nadpher May 16 at 13:08
  • \$\begingroup\$ 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 \$\endgroup\$ – Nadpher May 16 at 14:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Edward May 16 at 14:47
2
\$\begingroup\$

Some observations:

  • Comments:

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

    These comments don t tell you anything new the code doesnt already tell you so they should be removed. Ideally the code is well named like in that example so you dont feel the need to even write one.

    Annother advice with that is. If you feel the need in a long function to comment several parts because they do sth different its probaly a good idea to extract the parts into its own functions. Erases the Comments and makes the code self documentary.

  • Namespaces: You should always put youre 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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Nadpher May 14 at 18:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.