patterncppMinor
Snake game in C++ (using SFML)
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
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
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
Worse, your
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
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:
For other purposes, you could add things like assignment,
Using this, the code for the main loop of the game is simplified considerably:
Coordinates
Rather than use an
Use of standard algorithms
Once you've defined
This, in turn, can be written something like:
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
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.