7
\$\begingroup\$

This is the improved code of a question I asked some days ago.

main.cpp

#include "app.h"

int main() {

    Game::app game(800, 600, L"Test");
    game.start();
    game.end();
}

app.h

#pragma once

#include <SFML/Graphics.hpp>

#include "Snake.h"
#include "Board.h"

namespace Game {
    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;
        const float common_divisor;
        sf::RenderWindow window;
        Board board;
        sf::Font calibri;

        // MEMBER FUNCTIONS

        void drawWindow();
        void handleEvents();
        void updateWindow();
    };
}

app.cpp

#include "app.h"

#include <iostream>
#include <thread>
#include <chrono>

Game::app::app(int windowWidth, int windowHeight, const wchar_t* name)
    : winWidth{ windowWidth }, winHeight{ windowHeight }, common_divisor{ 40.0f } { 

    if (!calibri.loadFromFile("res/fonts/arial.ttf")) {
        std::wcout << L"[ERROR]: Couldn't load font\n";
    }

    window.create(sf::VideoMode(winWidth, winHeight), name);
    window.setFramerateLimit(5);
}

// Handles any game event
void Game::app::handleEvents() {

    sf::Event event;

    while (window.pollEvent(event)) {
        switch (event.type) {

        case sf::Event::Closed:
            window.close();
            break;

        case sf::Event::TextEntered:
            board.changeDirection(static_cast<char>(event.text.unicode));
        }
    }
}

// Draws all game objects
void Game::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({ common_divisor, common_divisor });

                rect.setPosition({ common_divisor * j, common_divisor * i});
                window.draw(rect);
            }

            // Draws snake
            else if (board[i * w + j] == 3) {

                sf::RectangleShape rect;
                rect.setFillColor(sf::Color::Green);
                rect.setSize({ common_divisor, common_divisor });

                rect.setPosition({ common_divisor * j, common_divisor * i });
                window.draw(rect);
            }

            // Draws food
            else if (board[i * w + j] == 4) {

                sf::RectangleShape rect;
                rect.setFillColor(sf::Color::Red);
                rect.setSize({ common_divisor, common_divisor });

                rect.setPosition({ common_divisor * j, common_divisor * i });
                window.draw(rect);
            }

        }
    }

    // Draws the game score
    sf::Text text;
    text.setFont(calibri);
    text.setPosition({ 0.0f, 0.0f });
    text.setString("Score: " + std::to_string(board.score()));
    text.setFillColor(sf::Color::Black);
    window.draw(text);
}

// Updates the render window
void Game::app::updateWindow() {
    window.clear(sf::Color::Black);
    drawWindow();
    window.display();
}

// Starts the app
void Game::app::start() {

    while (window.isOpen()) {
        handleEvents();
        board.update(window);
        updateWindow();
    }
}

void Game::app::end() {

    std::wcout << L"Game over!\nScore: " << board.score() << L'\n';
    std::this_thread::sleep_for((std::chrono::milliseconds)3000);   
}

Snake.h

#pragma once

#include <SFML/Graphics.hpp>
#include <vector>

#include "Coord.h"

class Snake {
public:

    Snake();
    ~Snake() = default;

    // Changes the dir value based on the input
    void changeDirection(char input);

    // Adds a piece to the snake and returns its location
    Coord add();
    size_t size();  

    /* Moves all pieces and returns
    the previous position of last piece */
    Coord follow();
    Coord moveHead(); // Moves and returns position of new head
    Coord headLocation() const;

private:    

    // MEMBER VARIABLES 
    struct Snake_segment
    {
        Coord current, previous;
    };

    enum direction {
        UP = 0,
        RIGHT,
        DOWN,
        LEFT
    };

    std::vector<Snake_segment> snakeContainer;
    direction dir;

public:

    Snake_segment operator[](int i) const;  
};

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.current.x;
    snakeContainer.push_back(head);
}

Coord Snake::add() {
    snakeContainer.push_back({
        snakeContainer.back().previous,
        snakeContainer.back().previous
    });

    return snakeContainer.back().current;
}

size_t Snake::size() {
    return snakeContainer.size();
}

// Changes the direction based on input (BUGGED)
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
Coord Snake::follow() {

    auto it = snakeContainer.begin();
    for (auto prev = it++; it != snakeContainer.end(); ++it, ++prev) {
        it->previous = it->current;
        it->current = prev->previous;
    }

    return snakeContainer.back().previous;
}


Coord Snake::moveHead() {

    snakeContainer[0].previous = snakeContainer[0].current;

    switch (dir) {
    case UP:
        --snakeContainer[0].current.y;
        break;

    case RIGHT:
        ++snakeContainer[0].current.x;
        break;

    case DOWN:
        ++snakeContainer[0].current.y;
        break;

    case LEFT:
        --snakeContainer[0].current.x;

    }

    return snakeContainer.front().current;
}

Snake::Snake_segment Snake::operator[](int i) const { return snakeContainer[i];               }
Coord Snake::headLocation()                   const { return snakeContainer.front().current;  }

Board.h

#pragma once

#include "Snake.h"

class Board {
public:

    Board();
    ~Board() = default;

    void update(sf::RenderWindow& win);
    void changeDirection(char input);
    char operator[](int i) const;
    int score() const;

    static constexpr int width = 20;
    static constexpr int height = 15;       

private:

    enum Tile {
        OPEN = 1,
        WALL,
        SNAKE,
        FOOD
    };

    // MEMBER VARIABLES
    Snake snake;
    std::string map;
    int m_score;

    // MEMBER FUNCTIONS

    void genFood();
    bool place(Coord coord, int item); // Sets a cell a certain value
    bool isEmpty(Coord coord) const;
    int at(Coord coord)       const;

};

Board.cpp

#include "Board.h"

#include <random>

Board::Board()
    : m_score{ 0 } {

    // 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
    };

    genFood();
}

int Board::at(Coord coord) const {
    return map[coord.y * width + coord.x];
}

bool Board::isEmpty(Coord coord) const {
    return at(coord) == OPEN;   
}

// Sets a cell a certain value
bool Board::place(Coord coord, int item) {
    if (item != OPEN && !isEmpty(coord))
        return false;

    map[coord.y * width + coord.x] = item;
    return true;
}

void Board::genFood() {

    int fx, fy;

    do {
        std::random_device gen;
        std::uniform_int_distribution<int> disX(0, width - 1);
        std::uniform_int_distribution<int> disY(0, height - 1);

        fx = disX(gen);
        fy = disY(gen);
    } while (map[fy * Board::width + fx] != OPEN);

    map[fy * width + fx] = FOOD;
}

void Board::update(sf::RenderWindow& win) {
    auto newHead{ snake.moveHead() };
    place(snake.follow(), OPEN);

    switch (at(snake.headLocation())) {
    case WALL:
    case SNAKE:
        win.close();
        break;

    case FOOD:
        place(snake.headLocation(), OPEN);
        place(snake.add(), SNAKE);
        m_score += 100;
        genFood();
    }

    place(newHead, SNAKE);
}

void Board::changeDirection(char input) {
    snake.changeDirection(input);
}

char Board::operator[](int i) const { return map[i];  }
int Board::score()            const { return m_score; }

Coord.h

#pragma once

struct Coord {
    unsigned int x, y;
};
\$\endgroup\$
  • \$\begingroup\$ Immediately after this update i replaced the random device with a pseudo random generator. \$\endgroup\$ – Nadpher May 16 at 16:32
8
\$\begingroup\$

Your program is definitely improved over the last version. Good job! Here are some ideas for you about further improvements.

Make the object interface easy for the user

The app object has two public functions, start and end that are apparently intended to be called in that order. To me, it would make more sense to eliminate end and simply move the contents of end to the end of start outside the while loop. That way the user only need to make a single call. Another idea would be to have a freestanding function that does what main currently is doing. It might look like this:

void snakes(int width, int height, const wchar_t *label) {
    Game::app game(width, height, label);
    game.start();
}

Try to make the application portable

The application currently tries to load the font from "res/fonts/arial.ttf" but no such file is on my machine so that load attempt fails. The error message is good, but could be better if it were to tell the user the actual path name the program is trying to use. Even better would be to allow the user to select a font or at least make it configurable per platform. This also leads us to the next suggestion.

Reduce or name and isolate constants

To run this code on my Linux machine, I created a new variable:

static const auto fontfile{"/usr/share/fonts/gnu-free/FreeSans.ttf"};

Then I used fontfile to load the file instead of having a hardcoded string embedded within the constructor. This way it's much easier to find and, if needed, change in the future. Similarly, instead of passing a constant to construct the common_divisor, one could instead compute it like this:

common_divisor{static_cast<float>(windowWidth)/Board::width}

Using that method, the code will continue to work even with different size windows as long as they have a 4:3 aspect ratio. This brings us to the next suggestion.

Avoid hardcoding large, regular data structures

There's nothing particularly wrong with having the default map hardcoded as in the current code, but it would be very simple to make it much more flexible and interesting. One way to do that is to construct the entire Board on the fly. Here's one way to do that:

Board::Board() : 
    map(static_cast<size_t>(width*height), static_cast<char>(OPEN))
{
    // set top and bottom walls
    for (unsigned i=0; i < width; ++i) {
        place({i, 0}, WALL);
        place({i, height-1}, WALL);
    }
    // set left and right walls
    for (unsigned j=1; j < height-1; ++j) {
        place({0, j}, WALL);
        place({width-1, j}, WALL);
    }

    // create two-segment snake
    place(snake.headLocation(), SNAKE);
    place(snake.add(), SNAKE);

    // add a bit of food
    genFood();
}

Now it is able to accept an arbitrary size window. Another subtle point here is that when a variable has a default that is always assigned when the object is constructed, assign it inline with the declaration instead. In this case the relevant declaration is:

int m_score = 0;

Also, if you wanted to always have a 4:3 aspect ratio, you could define height in terms of width like this:

static constexpr int height = width * 3 / 4;

Consider using finer-grained helper functions

At the moment, the code includes a function called genFood() which find a random empty square and then puts food there. Since there's already a function to put an object at an arbitrary location, I'd suggest the only thing missing is a function to find a random empty square. I'd write it like this:

Coord Board::randomEmpty() const {
    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);
    Coord coord{disX(gen),disY(gen)};

    while (!isEmpty(coord)) {
        coord = {disX(gen),disY(gen)};
    }
    return coord;
}

Then where the code currentl uses genFood, one would write this:

place(randomEmpty(), Food);

I'd suggest that using randomEmpty() to initialize the snake's location might also be good as long as the direction was chosen such as to not cause the player to immediately crash into a wall! Also note here that the maximums are width - 2 and height - 2 and not width - 1 and height - 1 which are the locations of walls and thus not actually candidate locations for the food.

Use helper functions to simplify code

Now that there are some helper functions in the code, such as Board::at(), I'd suggest that using them would make the code simpler and easier to read and understand. Here's a way to rewrite the app::drawWindow() function:

void Game::app::drawWindow() {
    for (unsigned i = 0, h = board.height; i < h; ++i) {
        for (unsigned j = 0, w = board.width; j < w; ++j) {
            Coord here{j, i};
            sf::RectangleShape rect;
            rect.setSize({ common_divisor, common_divisor });
            rect.setPosition({ common_divisor * j, common_divisor * i });
            switch(board.at(here)) {
                case Board::WALL:
                    window.draw(rect);
                    break;
                case Board::SNAKE:
                    rect.setFillColor(sf::Color::Green);
                    window.draw(rect);
                    break;
                case Board::FOOD: 
                    rect.setFillColor(sf::Color::Red);
                    window.draw(rect);
            }
        }
    }
    // Draws the game score
    sf::Text text;
    text.setFont(calibri);
    text.setCharacterSize(common_divisor);
    text.setPosition({ 0.0f, 0.0f });
    text.setString("Score: " + std::to_string(board.score()));
    text.setFillColor(sf::Color::Black);
    window.draw(text);
}

This requires that both Board::at() and the enum are made public instead of private but it makes the code much easier to read and understand. It also eliminates the need for the operator[]. This version also scales the score string so that it is always the same size as the wall.

Consider more fully using SFML

SFML includes a number of virtual base objects that make things simpler if you use them. For instance, you could derive app from sf::Drawable and change from drawWindow() to this instead:

void draw(sf::RenderTarget& target, sf::RenderStates states) const override;

Then within updateWindow() it would look like this:

void Game::app::updateWindow() {
    window.clear(sf::Color::Black);
    window.draw(*this);
    window.display();
}

Make sure you have all required #includes

The code uses std::string but doesn't #include <string>. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation and therefore belong in the .cpp file.

Don't use unnecessary #includes

This is a complementary suggestion to the one above. The code has #include "Snake.h" in app.h but nothing from that include file is actually needed in that code. For that reason, that #include should be eliminated. Also Snake.h includes SFML/Graphics.hpp but also makes no use of it.

Don't store variables that aren't needed

The winWidth and winHeight variables are not really needed within the class. Instead, use the passed values within the app constructor and don't bother saving them.

Reconsider the class interface

The Board class knows almost nothing about SFML and that's pretty good design because it means that only the app class needs to deal with SFML. But it's not quite perfect. The update function is passed an sf::RenderWindow. I'd suggest that a better way to do this is to eliminate the parameter and instead pass a bool back that is true if the user has crashed.

Eliminate redundant data

Each Snake_segment contains both the current and previous coordinates. However, only the current position and direction are really needed. The head of the snake needs the direction, but all subsequent nodes only need to update their current position to the previous segment's current position. The only slightly tricky part is to keep track of where to add a tail piece, but I'm sure that you will be able to see how to do this with a bit of thought.

Clean up as the program ends

Almost everything is automatically cleaned up at the end of the program except that there may be extra keystrokes in the input buffer. It would be nice to empty those out before the program leaves so they don't show up on the command line after the game is over.

Tighten up the interface

The Board::place routine takes a Coord and an int item as parameters. It would be a wee bit more correct and also aid the reader of the code if it took a Tile item as the second parameter instead.

\$\endgroup\$

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.