5
\$\begingroup\$

this is my first post to Code Review, so please bare with me.

I just finished writing a simple Snake clone in C++ with the goal of having an actual design this time and not just winging it. 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 and would really like to hear what you think about my code and the overall design I chose.


EDIT:

I've received some helpful feedback so far to the code and how I could improve some areas of my c++ knowledge, but I haven't received much feedback on the primary reason for this post, which was to get feedback on the overall design. In this project I tried to adhere to the SOLID principles of OOP, but I find that to be a difficult task and would like some pointers as to how I could improve the overall design.

\$\endgroup\$
  • \$\begingroup\$ 1st impression is good. It would be much easier if you created a gist or a github with the actual filles. For something this size, I want to compile it on my own machine in my own IDE, because I find it easier to understand that way. \$\endgroup\$ – Oliver Schönrock Jan 11 at 1:49
  • \$\begingroup\$ @OliverSchonrock I do have this on github, but there's a lot of other stuff in the same repository, so it might be a little confusing. github.com/Renge-Games/Renge-Engine/tree/dev \$\endgroup\$ – Sam Jan 11 at 2:11
2
\$\begingroup\$

Member variables are initialized by constructors in the order they are declared, not the order that are listed in the mem-initializer-list for a constructor. This can lead to unexpected results. Many compilers will issue a warning for this if you set the warning level higher (which you should be routinely doing, typically with /W4 or -w4). For Snake, the members will be initialized in this order: body, head, direction, oldDirection. This is the order they should be listed in the constructor.

Some of your initializers (DXGraphicsEngine(), body(std::vector<rge::v2di>())) aren't doing anything that doesn't happen by default and should be omitted.

The switch statement in some functions like the second Snake constructor and in Snake::MoveSnake have unnecessary braces in the case statements. For readability, the case labels are often indented.

The body for Snake::ExtendSnake can be rewritten as body.emplace_back(body.back());

There are places where you use this-> to access member variables where it is unnecessary. Where it is necessary (like Snake::SetDirection), you should rename the local variable to something that does not shadow a member variable. For SetDirection, you can use Snake::SetDirection(Direction dir). Elsewhere, you're assigning to this->color =, which can just be color =. Or place the initializer in the mem-iniitializer-list (SnakeTile(): color(120, 130, 250) { }).

Rather than using vector<Tile *>, look into using a smart pointer (like shared_ptr or unique_ptr) so you don't have to do all that manual memory management.

The FieldGrid constructors can start off with field.resize(width) to avoid growing the array in the i loop. This could also be accomplished with the mem-initializer-list, but you'd have to be careful about specifying the list sine the width member will be initialized after field.

Consider using some of the standard C++ random library classes rather than using rand.

Some functions can take parameters by const & to avoid making copies of the parameters. FieldGrid::ClearSnake(const std::vector<rge::v2di> &body). Also, you can use range-based for loops:

for (const auto &b: body) {
    delete field[b.x][b.y];
    field[b.x][b.y] = new EmptyTile();
}

In your classes derived from Tile you should use the override keyword when overriding virtual members from a base class. This will result in a compilation error if you make a mistake in the function signature or change something.

CollisionMessage OnCollide() override {
}
bool IsFree() override { return true; }

In addition, IsFree can be const.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions, i had no idea the member variables weren't initialized in the order they appear in the constructor. Same with the default constructor for DXGraphicsEngine() I thought it was necessary to add the constructor of the base class in the constructor for the child class. The braces in the switch statement I usually have there so I don't run into any issues when I declare a variable like "i" or something in each case statement. How about the design of my project? Is there some OOP principle I violated or something I could improve there? \$\endgroup\$ – Sam Jan 11 at 15:50
2
\$\begingroup\$

The posted code looks pretty good.

Constants: You declare a lot of inline literals. It will be hard to alter things like snake colors.


Virtual destructors: it functionally works, but it's generally good practice to make your base class destructors virtual. It doesn't make your objects bigger since they already have a vtable.


Use the override keyword when appropriate.


But actually, why are tiles virtual? It appears that they could be structs that only store a type enum. Consider that currently all the empty tiles store a color, but all their colors will always be the same. You could have a small configuration table that associates tile types with color or behavior.


Consider making the collision message be a std::optional result. Then you can communicate a non-collision semantically. Also consider making it a struct since it has public fields.


Prefer smart pointers instead of adding a bunch of raw pointers to the vector. But actually, try avoiding needing big collections of polymorphic objects.


MoveSnake: Perhaps use a std::deque so you can push_front. Or use iterators to move the items. Or store the snake backwards in the vector to avoid needing to move all the memory. Or just insert a head at the beginning because the vector is always likely to be small and a memcpy to move the rest isn't a big deal.


Try splitting out rendering logic from your objects. For example, instead of looping over tiles and calling their draw method, try making the method that does the looping also do the drawing. Later, you could batch up similar tiles to render faster.


SnakeGame::OnRender: Do you really want to cout every time you render? How does the body get rendered?

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the response! The constants were one of my big issues writing this. I don't know how to fix that problem. I made the tiles virtual so I wouldn't need a big switch statement, and because I was trying to satisfy the open closed principle, but I see that I failed at that to an extent, because I have an enum to find the tile type and there's some other conditional stuff going on outside the scope of the the tiles. The body is drawn in FieldGrid with the snake tiles. \$\endgroup\$ – Sam Jan 11 at 16:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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