HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Snake game in C++ (using SFML)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamesfmlusingsnake

Problem

I haven't done any programming in a while, and I'm getting back into it. I made this Snake clone to refresh me a bit, and I would like to get some feedback in the "elegance" and "well-done"-ness of the code.

main.cpp

#include "Game.h"

int main()
{
    Game::run();
    return 0;
}


Game.h

```
#ifndef __GAME_H__
#define __GAME_H__

#include

#include
#include
#include

enum class Direction {UP, RIGHT, DOWN, LEFT};
enum class MoveEvent {EAT, COLLIDE};

class Game
{
public:

static void run();

private:

static sf::RenderWindow * window;
static int score;
static int level;
static bool alive;

static std::pair foodLocation;

// Random Number Generation for X
static const int rangex_from;
static const int rangex_to;
static std::random_device rand_devX;
static std::mt19937 generatorX;
static std::uniform_int_distribution distrX;

static const int rangey_from;
static const int rangey_to;
static std::random_device rand_devY;
static std::mt19937 generatorY;
static std::uniform_int_distribution distrY;

static std::vector> snake_body;
static Direction snake_direction;
static Direction new_direction;

static std::pair getNewFoodLocation();
static void moveSnake();
static void draw();
static bool collides(std::pair, std::vector>);

static sf::Texture tile;
static sf::Texture body;
static sf::Texture food;

static sf::Sprite tile_spr;
static sf::Sprite body_sp

Solution

Basic structure

With all its members static, your Game class looks a whole lot like a namespace. I'd either make it a namespace, or else make everything that's specific to a particular game (e.g., the current score) non-static so that each game object really represents an actual game. The latter is probably preferable, but either is an improvement on the current situation.

Worse, your Game class looks a lot like what's often called a "God class". A single class that tries to be all, know all, and do all. By lumping everything together into a single class, you've lost most of the benefits of using classes in the first place, and just written basic block-structured code about like you would in something like C, but with Game:: tacked onto the beginnings of many (most?) of the names.

I'd rather see (for example) one class for the snake, another for a food object, and (possibly) another for the game board (or possibly just for a tile).

Random number generation

I don't see any use in having separate random number generators for your X and Y coordinates. Quite the contrary, you're almost certainly better off using a single generator for both. Likewise, there's no point in creating separate instances of random_device to initialize each.

Wrapping numbers

A fair amount of the logic in your game code is devoted to the fairly simple job of ensuring that a number always stays within a particular range (and if you try to decrement below the beginning or increment above the end, wrapping around to the other end).

At least in my opinion, that sounds a lot like the specification of a small class:

class bounded { 
    int current;
    int lower;
    int upper;
public:
    bounded(int lower, int upper) : upper(upper), lower(lower) {}

    bounded &operator++() { 
       ++current;
       if (current >= upper)
           current = lower;
       return *this;
    }

    bounded &operator--() { 
        --current;
        if (current < lower)
            current = upper;
        return *this;
    }
};


For other purposes, you could add things like assignment, +=, -=, and so on, but for this game I believe increment and decrement are all you really need.

Using this, the code for the main loop of the game is simplified considerably:

case Direction::UP:
    --new_head_pos.second;
    new_head_pos.first = snake_body[0].first;       
    break;

// ...


Coordinates

Rather than use an std::pair as the coordinate (and have all the other code know about its internals) I'd define a Coordinate class that knows how to do the things you have to do with coordinates (e.g., compare them):

class Coordinate {
    int x;
    int y;
public:
    Coordinate(int x, int y) : x(x), y(y) {}

    bool operator==(Coordinate const &b) const { 
        return x == b.x && y == b.y;
    }
};


Use of standard algorithms

Once you've defined Coordinate, your Game::collides becomes something like:

bool Game::collides(std::pair coordinate, std::vector> body)
{
    for (int i = 0; i < body.size(); i++)
        if (coordinate == body[i])
            return true;
    return false;
}


This, in turn, can be written something like:

bool Game::collides(Coordinate const &coordinate, std::vector const &body) { 
     return std::find(body.begin(), body.end(), coordinate) != body.end();
}


Of course, if you take the previous advice, the external interface to this would simplified a bit further, because it would be a member of the Snake class:

class Snake { 
    std::vector body;
public:
    bool collides(Coordinate const &point) { 
        return std::find(body.begin(), body.end(), point) != body.end();
    }

    // ...        
};

Code Snippets

class bounded { 
    int current;
    int lower;
    int upper;
public:
    bounded(int lower, int upper) : upper(upper), lower(lower) {}

    bounded &operator++() { 
       ++current;
       if (current >= upper)
           current = lower;
       return *this;
    }

    bounded &operator--() { 
        --current;
        if (current < lower)
            current = upper;
        return *this;
    }
};
case Direction::UP:
    --new_head_pos.second;
    new_head_pos.first = snake_body[0].first;       
    break;

// ...
class Coordinate {
    int x;
    int y;
public:
    Coordinate(int x, int y) : x(x), y(y) {}

    bool operator==(Coordinate const &b) const { 
        return x == b.x && y == b.y;
    }
};
bool Game::collides(std::pair<int, int> coordinate, std::vector<std::pair<int, int>> body)
{
    for (int i = 0; i < body.size(); i++)
        if (coordinate == body[i])
            return true;
    return false;
}
bool Game::collides(Coordinate const &coordinate, std::vector<Coordinate> const &body) { 
     return std::find(body.begin(), body.end(), coordinate) != body.end();
}

Context

StackExchange Code Review Q#122237, answer score: 5

Revisions (0)

No revisions yet.