1

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:

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.

5
  • 2
    Contentious opinion: for small/simple programs, SOLID offers few benefits, and often many drawbacks. Where SOLID is beneficial for large, complicated software projects. Make your snake program larger, more complicated, with more features, and only once you start dealing with the pain of those aspects, will SOLID start to make any real sense. Commented Jan 12, 2020 at 3:15
  • @whatsisname the intention of this project was to maybe get some ideas of good design, so I can expand in future projects and not end up with spaghetti code quarter way through and end up having to scrap the whole thing.
    – Renge
    Commented Jan 12, 2020 at 14:15
  • @Sam: the most successful way of making something expandable for the future is keeping things as simple as possible and avoiding overdesign. That puts focus on the "S" from SOLID; if you need "the other four letters", you are usually off-track.
    – Doc Brown
    Commented Jan 12, 2020 at 16:41
  • @DocBrown I think my issue in the past has been keeping things too simple or perhaps not thought out well enough, leading to unclear code. Maybe I'm oscillating between overdesign and underdesign.
    – Renge
    Commented Jan 12, 2020 at 17:21
  • @Sam: I completely understand what you are trying to do. But honestly, you need to build the massive spaghetti monster to really understand all the trade-offs. Additionally, "good design" isn't universal. Good design for a vending machine is not going to be the same as good design for a nationwide online insurance-quoting website. SOLID is for the insurance-quote scale of complexity, you need to try to tackle a project of that degree of hairyness to see what is good design in that situation. Commented Jan 13, 2020 at 15:38

2 Answers 2

2

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:

  • there is no relation between the Snake and either FieldGrid or Tile: it feels hard to believe that both could be as independent as this diagram suggests, especially when considering that some Tile represent parts of the Snake.
  • CollisionMessage is associated to the Tile and not to SnakeGame that orchestrates the collaboration between the entities
  • TileType is associated to the message but not tho the tile itself
  • TileType seems redundant with the specialization of Tile into EmptyTile, 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 the Game that owns it temporarily and discard when it's handled.

  • So the class diagram should not show an association between Tile and CollisionMessage.

  • It should instead show an association between CollisionMessage and Game, and could show a creation dependency between Tile and CollisionMessage.

The other weak point is a lack of separation concerns:

  • the calss reveals the tileType to the Game, 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:

  • Let the collision detector (here FieldGrid::CheckCollision() with the support of Tile::OnColide() ) determine the event. For example if the collision is with FoodTile create a FeedingEvent whereas a SnakeTile or MongooseTile would create a FatalEvent. All those even being derived from an abstract CollisionMessage
  • Each kind of Tile having already its own implementation of OnColide(), that'll be super easy: just return the right event for each tile.
  • The easiest implementation would be to change CollisionMessage in away to refer to a CollisionType instead of a TileType. At least, the actions in the game loop would be more logic to read (eg if type is FeedignEvent ..., if type is FatalEvent... etc...)
  • A more elaborate way would be to use polymorphism , and return sepcialized subtypes of CollisionMessage. But polymorphism to work safely on return type in C++ would need the use of 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:

  • Currently you also have a FieldGrid::CheckCollision() that creates a CollisionMessage when the snakes goes out of bounds.
  • Unfortunately you do not set tileType and by the way it's neither snake, nor empty nor food.
  • Furthermore, the constructor does not initialize this member. So its value could be anything: a legal value corresponding to the wrong tile type (which could cause the game loop to draw wrong conclusions) or an illegal value. See also this SO question about enum initialization.

Grids and snakes

About the UML:

  • you should use composition between Snake and v2di and Direction. These classes are used as or in a couple of Snake 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

std::vector<std::vector<Tile*>> field;

could be replaced with

std::vector<std::vector<shared_ptr<Tile>>> field;

and all the

    delete field[body[i].x][body[i].y];
    field[body[i].x][body[i].y] = new EmptyTile();  // or SnakeTile or...

would then be replaced by

    field[body[i].x][body[i].y] = make_shared<EmptyTile>();  // or SnakeTile or...

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.

1
  • Thank you so much for taking the time to write out this extensive answer, this is exactly the kind of response I was hoping for! Do you think there is a better way to design the interaction between snake and fieldgrid? I feel like the current Interaction is somewhat tedious, since I have to clear the snake and add it, almost like drawing to a canvas, also as you said, there's no multiplayer support (although on first glance I feel like I could just add another tile SnakeTile2 to compensate).
    – Renge
    Commented Jan 12, 2020 at 14:06
0

I'd say the problem here is the concept of the FieldGrid as an array of Tiles.

You limit yourself to a classic implementation of the game where the UI, an ascii display, is tightly coupled to the game itself

Fine if that what you want, but then you don't really need the Snake class. The snake is just the collection of SnakeTiles.

A more modern design would model the snake and food position with 3d float vectors, disassociating the UI from the model and allowing you to disregard the concept of Tiles an especially EmptyTile.

This allows you more flexibility with feature requests, make the snake move diagonally! make the snake on the z index! make the play area infinitely large etc

3
  • Wouldn't this approach also make the collision detection a lot more general, since there is potential to go off grid? I see that the grid of tiles might be restricting though. Maybe if I keep the 2d vector and just have a list of tiles with their respective positions in the grid, eliminating the need for an empty tile.
    – Renge
    Commented Jan 12, 2020 at 14:13
  • "This allows you more flexibility with feature requests, make the snake move diagonally! make the snake on the z index! make the play area infinitely large etc" - see, this is the danger when making things "extensible" and the source of YAGNI. You're just randomly focusing on expanding software in areas that are interesting from a programming perspective, without considering whether it is good for the overall goal. Itt's analogous to a dentist appointment booking software using arbitrary calendars so you can book appointments on mars. It's just completely the wrong area to focus. Commented Jan 13, 2020 at 15:34
  • @whatsisname its commonly recognised good practice to separate the presentation and business layers. YAGNI is also called "Thinking ahead"
    – Ewan
    Commented Jan 13, 2020 at 15:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.