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 <SFML/Graphics.hpp>
#include <random>
#include <utility>
#include <vector>
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<int, int> 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<int> 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<int> distrY;
static std::vector<std::pair<int, int>> snake_body;
static Direction snake_direction;
static Direction new_direction;
static std::pair<int, int> getNewFoodLocation();
static void moveSnake();
static void draw();
static bool collides(std::pair<int, int>, std::vector<std::pair<int, int>>);
static sf::Texture tile;
static sf::Texture body;
static sf::Texture food;
static sf::Sprite tile_spr;
static sf::Sprite body_spr;
static sf::Sprite food_spr;
};
#endif
Game.cpp
#include "Game.h"
#include <iostream>
// Initialization of static variables
sf::RenderWindow * Game::window = new sf::RenderWindow(sf::VideoMode(640, 480), "Snake", sf::Style::Titlebar | sf::Style::Close);
int Game::score = 0;
int Game::level = 1;
bool Game::alive = true;
const int Game::rangex_from = 0;
const int Game::rangex_to = 39;
const int Game::rangey_from = 0;
const int Game::rangey_to = 29;
std::random_device Game::rand_devX;
std::random_device Game::rand_devY;
std::mt19937 Game::generatorX(Game::rand_devX());
std::mt19937 Game::generatorY(Game::rand_devY());
std::uniform_int_distribution<int> Game::distrX(Game::rangex_from, Game::rangex_to);
std::uniform_int_distribution<int> Game::distrY(Game::rangey_from, Game::rangey_to);
std::vector<std::pair<int, int>> Game::snake_body;
Direction Game::snake_direction;
Direction Game::new_direction;
std::pair<int, int> Game::foodLocation = Game::getNewFoodLocation();
sf::Texture Game::body;
sf::Texture Game::tile;
sf::Texture Game::food;
sf::Sprite Game::body_spr;
sf::Sprite Game::tile_spr;
sf::Sprite Game::food_spr;
std::pair<int, int> Game::getNewFoodLocation()
{
std::pair<int, int> temporalLocation;
do
{
temporalLocation = std::pair<int, int>(Game::distrX(Game::generatorX), Game::distrY(Game::generatorY));
} while (collides(temporalLocation, Game::snake_body));
return temporalLocation;
}
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.first == body[i].first && coordinate.second == body[i].second)
return true;
return false;
}
void Game::moveSnake()
{
std::pair<int, int> new_head_pos;
// Calculates the new position the snakes head will take
switch (new_direction)
{
case Direction::UP:
if (snake_body[0].second - 1 >= rangey_from)
new_head_pos.second = snake_body[0].second - 1;
else
new_head_pos.second = rangey_to;
new_head_pos.first = snake_body[0].first;
break;
case Direction::DOWN:
if (snake_body[0].second + 1 <= rangey_to)
new_head_pos.second = snake_body[0].second + 1;
else
new_head_pos.second = rangey_from;
new_head_pos.first = snake_body[0].first;
break;
case Direction::LEFT:
if (snake_body[0].first - 1 >= rangex_from)
new_head_pos.first = snake_body[0].first - 1;
else
new_head_pos.first = rangex_to;
new_head_pos.second = snake_body[0].second;
break;
case Direction::RIGHT:
if (snake_body[0].first + 1 <= rangex_to)
new_head_pos.first = snake_body[0].first + 1;
else
new_head_pos.first = rangex_from;
new_head_pos.second = snake_body[0].second;
break;
}
//Fast hack <_<
std::vector<std::pair<int, int>> body_minus_tail = snake_body;
body_minus_tail.pop_back();
// Checks if the snake will collide with its own body
if(collides(new_head_pos, body_minus_tail))
{
std::cout << "You Lost! Score: " << score << std::endl;
alive = false;
}
if(new_head_pos == foodLocation)
{
score++;
snake_body.insert(snake_body.begin(), new_head_pos);
foodLocation = getNewFoodLocation();
}
else
{
snake_body.insert(snake_body.begin(), new_head_pos);
snake_body.pop_back();
}
snake_direction = new_direction;
}
void Game::run()
{
window->setFramerateLimit(60);
snake_body.push_back(std::pair<int, int>(2, 0));
snake_body.push_back(std::pair<int, int>(1, 0));
snake_body.push_back(std::pair<int, int>(0, 0));
snake_direction = Direction::RIGHT;
new_direction = Direction::RIGHT;
body.loadFromFile("body.png");
tile.loadFromFile("tile.png");
food.loadFromFile("food.png");
body_spr.setTexture(body);
tile_spr.setTexture(tile);
food_spr.setTexture(food);
sf::Clock clock;
sf::Time time1;
while (window->isOpen())
{
sf::Event event;
while (window->pollEvent(event))
{
switch (event.type)
{
// Calculates the new direction the snake is heading based on user input
case sf::Event::KeyPressed:
switch (event.key.code)
{
case sf::Keyboard::Up:
case sf::Keyboard::W:
if (snake_direction != Direction::DOWN)
new_direction = Direction::UP;
break;
case sf::Keyboard::Right:
case sf::Keyboard::D:
if (snake_direction != Direction::LEFT)
new_direction = Direction::RIGHT;
break;
case sf::Keyboard::Down:
case sf::Keyboard::S:
if (snake_direction != Direction::UP)
new_direction = Direction::DOWN;
break;
case sf::Keyboard::Left:
case sf::Keyboard::A:
if (snake_direction != Direction::RIGHT)
new_direction = Direction::LEFT;
break;
}
break;
case sf::Event::Closed:
window->close();
break;
}
}
time1 = clock.getElapsedTime();
if (time1.asMilliseconds() >= 1000/20)
{
moveSnake();
clock.restart();
}
if (!alive)
window->close();
window->clear();
draw();
window->display();
}
}
void::Game::draw()
{
for (int i = 0; i < rangex_to; i++)
for (int j = 0; j < rangey_to; j++)
{
tile_spr.setPosition(i*16, j*16);
window->draw(tile_spr);
}
food_spr.setPosition(foodLocation.first*16, foodLocation.second*16);
window->draw(food_spr);
for (int i = 0; i < snake_body.size(); i++)
{
body_spr.setPosition(snake_body[i].first*16, snake_body[i].second*16);
window->draw(body_spr);
}
}
1 Answer 1
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<int, int> 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<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;
}
This, in turn, can be written something like:
bool Game::collides(Coordinate const &coordinate, std::vector<Coordinate> 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<Coordinate> body;
public:
bool collides(Coordinate const &point) {
return std::find(body.begin(), body.end(), point) != body.end();
}
// ...
};
__, (__GAME_H__), that's a reserved prefix: stackoverflow.com/a/228797/1198654 \$\endgroup\$