I posted this question originally in Code Review, but then thought that I could possibly get more feedback about the design here.
I just finished writing a simple Snake clone in C++ with the goal of having an actual design this time and to adhere to the SOLID principles of OOP. So I made a class diagram of the game and wrote all the code, and it works fine, but there are some areas where I feel like I could have done better. For example, there are some hard coded elements which I wasn't able to elegantly remove, and there's one place where I use inheritance, but then I also have an enum to figure out which child a message is coming from when I would prefer not to have to care at all what type the child is.
Anyway, here's the class diagram:
Engine is a simple engine I wrote to display 2d graphics easily, I only use it for rendering and some helper classes like v2di which is a 2d integer vector for storing positional information
Here's the game class which is responsible for starting and running the game.
OnCreate()
is called once on startup,
OnUpdate()
and OnRender()
are called once per frame and should contain the game loop,
OnDestroy()
is called when the inner game loop is exiting and the program is about to quit:
///////////////// .h
class SnakeGame : public rge::DXGraphicsEngine {
public:
SnakeGame();
bool OnCreate();
bool OnUpdate();
bool OnRender();
void OnDestroy();
protected:
Snake snake;
FieldGrid field;
int score;
bool gameOver;
int updateFreq;
int updateCounter;
};
//////////////////////// .cpp
SnakeGame::SnakeGame() : DXGraphicsEngine(), field(10, 10), snake(3, rge::v2di(3, 1), Direction::RIGHT), score(0), gameOver(false), updateFreq(10), updateCounter(0){
}
bool SnakeGame::OnCreate() {
field.AddSnake(snake.GetBody());
field.GenerateFood();
return true;
}
bool SnakeGame::OnUpdate() {
//check user input
if(GetKey(rge::W).pressed) {
snake.SetDirection(Direction::UP);
}
if(GetKey(rge::S).pressed) {
snake.SetDirection(Direction::DOWN);
}
if(GetKey(rge::A).pressed) {
snake.SetDirection(Direction::LEFT);
}
if(GetKey(rge::D).pressed) {
snake.SetDirection(Direction::RIGHT);
}
updateCounter++;
if(!gameOver && updateCounter >= updateFreq) {
updateCounter = 0;
//clear snake body from field
field.ClearSnake(snake.GetBody());
//move
snake.MoveSnake();
//add snake body to field
field.AddSnake(snake.GetBody());
//testcollision
CollisionMessage cm = field.CheckCollision(snake.GetHead());
gameOver = cm.gameOver;
score += cm.scoreChange ? snake.GetLength() * 10 : 0;
if(cm.tileType == TileType::Food) {
field.GenerateFood();
snake.ExtendSnake();
}
}
return true;
}
bool SnakeGame::OnRender() {
std::cout << score << std::endl;
field.Draw(&m_colorBuffer, 100, 20, 10);
snake.DrawHead(&m_colorBuffer, 100, 20, 10);
return true;
}
Next up is the Snake
class that moves and extends the snake. There's also an enum for the Direction
the snake can move in:
///////////// .h
enum class Direction {
UP, DOWN, LEFT, RIGHT
};
class Snake {
public:
Snake();
Snake(int length, rge::v2di position, Direction direction);
rge::v2di GetHead() { return head; }
std::vector<rge::v2di> GetBody() { return body; }
void MoveSnake();
void ExtendSnake();
Direction GetDirection() { return direction; }
void SetDirection(Direction direction);
int GetLength() { return body.size() + 1; }
void DrawHead(rge::Buffer* buffer, int x, int y, int size);
protected:
std::vector<rge::v2di> body;
rge::v2di head;
Direction direction;
Direction oldDirection;
};
////////////// .cpp
Snake::Snake(): head(rge::v2di(0, 0)), direction(Direction::UP), oldDirection(Direction::UP), body(std::vector<rge::v2di>()){
body.push_back(rge::v2di(head.x, head.y + 1));
}
Snake::Snake(int length, rge::v2di position, Direction direction) : head(position), direction(direction), oldDirection(direction), body(std::vector<rge::v2di>()) {
for(int i = 0; i < length-1; ++i) {
rge::v2di bodyTile;
switch(direction) {
case Direction::UP:{
bodyTile.x = head.x;
bodyTile.y = head.y + (i + 1);
break;
}
case Direction::DOWN:{
bodyTile.x = head.x;
bodyTile.y = head.y - (i + 1);
break;
}
case Direction::LEFT: {
bodyTile.y = head.y;
bodyTile.x = head.x + (i + 1);
break;
}
case Direction::RIGHT: {
bodyTile.y = head.y;
bodyTile.x = head.x - (i + 1);
break;
}
}
body.push_back(bodyTile);
}
}
void Snake::MoveSnake() {
oldDirection = direction;
for(int i = body.size()-1; i > 0; --i) {
body[i] = body[i - 1];
}
body[0] = head;
switch(direction) {
case Direction::UP: {
head.y--;
break;
}
case Direction::DOWN: {
head.y++;
break;
}
case Direction::LEFT: {
head.x--;
break;
}
case Direction::RIGHT: {
head.x++;
break;
}
}
}
void Snake::ExtendSnake() {
body.push_back(body[body.size() - 1]);
}
void Snake::SetDirection(Direction direction) {
switch(this->oldDirection) {
case Direction::UP:
case Direction::DOWN: {
if(direction != Direction::UP && direction != Direction::DOWN) {
this->direction = direction;
}
break;
}
case Direction::LEFT:
case Direction::RIGHT: {
if(direction != Direction::LEFT && direction != Direction::RIGHT) {
this->direction = direction;
}
break;
}
}
}
void Snake::DrawHead(rge::Buffer* buffer, int x, int y, int size) {
rge::Color c(100, 100, 200);
buffer->DrawRegion(x + head.x * size, y + head.y * size, x + head.x * size + size, y + head.y * size + size, c.GetHex());
}
Then there's the FieldGrid
class responsible for collision detection, food generation and storing the state of the map:
//////////// .h
class FieldGrid {
public:
FieldGrid();
FieldGrid(int width, int height);
~FieldGrid();
void GenerateFood();
CollisionMessage CheckCollision(rge::v2di head);
void ClearSnake(std::vector<rge::v2di> body);
void AddSnake(std::vector<rge::v2di> body);
void Draw(rge::Buffer* buffer, int x, int y, int size);
protected:
std::vector<std::vector<Tile*>> field;
int width;
int height;
};
//////////// .cpp
FieldGrid::FieldGrid() : width(10), height(10), field(std::vector<std::vector<Tile*>>()) {
for(int i = 0; i < width; ++i) {
field.push_back(std::vector<Tile*>());
for(int j = 0; j < height; ++j) {
field[i].push_back(new EmptyTile());
}
}
}
FieldGrid::FieldGrid(int width, int height): width(width), height(height), field(std::vector<std::vector<Tile*>>()) {
for(int i = 0; i < width; ++i) {
field.push_back(std::vector<Tile*>());
for(int j = 0; j < height; ++j) {
field[i].push_back(new EmptyTile());
}
}
}
FieldGrid::~FieldGrid() {
for(int i = 0; i < field.size(); ++i) {
for(int j = 0; j < field[i].size(); ++j) {
delete field[i][j];
}
field[i].clear();
}
field.clear();
}
void FieldGrid::GenerateFood() {
int x = rand() % width;
int y = rand() % height;
while(!field[x][y]->IsFree()) {
x = rand() % width;
y = rand() % height;
}
delete field[x][y];
field[x][y] = new FoodTile();
}
CollisionMessage FieldGrid::CheckCollision(rge::v2di head) {
if(head.x < 0 || head.x >= width || head.y < 0 || head.y >= height) {
CollisionMessage cm;
cm.scoreChange = false;
cm.gameOver = true;
return cm;
}
return field[head.x][head.y]->OnCollide();
}
void FieldGrid::ClearSnake(std::vector<rge::v2di> body) {
for(int i = 0; i < body.size(); ++i) {
delete field[body[i].x][body[i].y];
field[body[i].x][body[i].y] = new EmptyTile();
}
}
void FieldGrid::AddSnake(std::vector<rge::v2di> body) {
for(int i = 0; i < body.size(); ++i) {
delete field[body[i].x][body[i].y];
field[body[i].x][body[i].y] = new SnakeTile();
}
}
void FieldGrid::Draw(rge::Buffer* buffer, int x, int y, int size) {
for(int xi = 0; xi < width; ++xi) {
for(int yi = 0; yi < height; ++yi) {
int xp = x + xi * size;
int yp = y + yi * size;
field[xi][yi]->Draw(buffer, xp, yp, size);
}
}
}
Tile
class used in FieldGrid
:
class Tile {
public:
virtual CollisionMessage OnCollide() = 0;
virtual bool IsFree() = 0;
void Draw(rge::Buffer* buffer, int x, int y, int size) {
buffer->DrawRegion(x, y, x + size, y + size, color.GetHex());
}
protected:
rge::Color color;
};
class EmptyTile : public Tile {
public:
EmptyTile() {
this->color = rge::Color(50, 50, 50);
}
CollisionMessage OnCollide() {
CollisionMessage cm;
cm.scoreChange = false;
cm.gameOver = false;
cm.tileType = TileType::Empty;
return cm;
}
bool IsFree() { return true; }
};
class FoodTile : public Tile {
public:
FoodTile() {
this->color = rge::Color(50, 200, 70);
}
CollisionMessage OnCollide() {
CollisionMessage cm;
cm.scoreChange = true;
cm.gameOver = false;
cm.tileType = TileType::Food;
return cm;
}
bool IsFree() { return false; }
};
class SnakeTile : public Tile {
public:
SnakeTile() {
this->color = rge::Color(120, 130, 250);
}
CollisionMessage OnCollide() {
CollisionMessage cm;
cm.scoreChange = false;
cm.gameOver = true;
cm.tileType = TileType::Snake;
return cm;
}
bool IsFree() { return false; }
};
Finally here's the CollisionMessage
class used to send messages to the game when the snake head collides with any Tile
:
enum class TileType {
Empty,
Snake,
Food
};
class CollisionMessage {
public:
bool scoreChange;
bool gameOver;
TileType tileType;
};
I omitted all the includes and the main method, as they aren't relevant to the design and would just take up extra space.
I appreciate the time you take to read through all my code (or just look at the class diagram) and would really like to hear what you think about the overall design I chose.
Best Answer
You are at the right place for a review of your design. But this is not a code review site, so don't expect an in-depth inspection here.
Overview
What strikes me first looking at the classes is that:
Snake
and eitherFieldGrid
orTile
: it feels hard to believe that both could be as independent as this diagram suggests, especially when considering that someTile
represent parts of theSnake
.CollisionMessage
is associated to theTile
and not toSnakeGame
that orchestrates the collaboration between the entitiesTileType
is associated to the message but not tho the tile itselfTileType
seems redundant with the specialization ofTile
intoEmptyTile
,FoodTile
,SnakeTile
.CollisionMessage, TileType, and separation of concerns
The UML model:
In your code, no tile is associated with a
CollisionMessage
(which is in fact more an event than a message). The tiles create the message and returns it to theGame
that owns it temporarily and discard when it's handled.So the class diagram should not show an association between
Tile
andCollisionMessage
.It should instead show an association between
CollisionMessage
andGame
, and could show a creation dependency betweenTile
andCollisionMessage
.The other weak point is a lack of separation concerns:
the calss reveals the
tileType
to theGame
, and the game decides what to do based on its (leaked) knowledge of the tile. Shouldn't the game loop coordinate, and let the other classes decide about the details ?Now imagine that you add some more kind of tiles:
MountainTile
(randomly fatal or not because sometimes rocks fall and kill the snake),SeeTile
(some snakes swimm),MongooseTile
(always fatal): would you really want the game loop to know what to do in each case ?Fortunately, there is a solution to improve this design by adding separation of concerns and strengthening the opend/close principle:
FieldGrid::CheckCollision()
with the support ofTile::OnColide()
) determine the event. For example if the collision is withFoodTile
create aFeedingEvent
whereas aSnakeTile
orMongooseTile
would create aFatalEvent
. All those even being derived from an abstractCollisionMessage
CollisionMessage
in away to refer to aCollisionType
instead of aTileType
. At least, the actions in the game loop would be more logic to read (eg if type is FeedignEvent ..., if type is FatalEvent... etc...)shared_ptr
. The advatage could be that the game loop could call some virtual functions of the CollisionMessage instead of examining all the case by itself.Finally, I spotted a nasty bug:
FieldGrid::CheckCollision()
that creates aCollisionMessage
when the snakes goes out of bounds.tileType
and by the way it's neither snake, nor empty nor food.Grids and snakes
About the UML:
you should use composition between
Snake
andv2di
andDirection
. These classes are used as or in a couple ofSnake
members, but by value. So it's really an exclusive ownership and lifecycle control.despite my initial question, the model is ok. In your code, you really keep a snake separate from the grid: at each move, you clear the snake tiles in the diagram and you add the snake to change the tiles of the new position. You could perhaps show a usage dependency of the snake by the grid, to make the diagram more expressive.
this design limits of course the game to a single snake, since in the grid you do not know to which snake a tile belongs.
Then a code recommendation: forget about raw pointers: it's so easy to make errors in memory management. Let the library care for memory management for you:
So
could be replaced with
and all the
would then be replaced by
The shared pointers take care of deallocating tiles which are no longer used. It's much safer.
So, that's all folks ! Sorry for the long answer to the long question. But hope it helps.