6
\$\begingroup\$

I have a c# background and mostly worked on Web Applications. Recently I wanted to learn c++. I read many online resources and inspect 10s of source code shared on Stack Overflow / Code Review or from C++ forums.

During my research I've come accross this post: Snake game in C++ and based on answer to that I decided to try and rewrite it in an object-oriented manner.

I have many questions about the language itself but I think working on a project is a good way to learn any language's dynamics. I'd like to represent this source to you and hoping to get pointed out what I did wrong and what I did correct.

My thought proccess was to keep it as standart compliant as possible. _kbhit() and _getch() are platform specific but I didn't want to refactor them yet as it was already a hard work for me to refactor this much.

To be able to keep it standart compliant I decided to use a base Renderer class and inherited a Win32ConsoleRenderer class from it to render it on Windows Console. My thought was, if someone else want to port this to Linux or Mac they'd simply need to create a XXRenderer class based on Renderer and compile it without issues.

I tried to avoid using raw pointers and used std::unique_ptr for my pointer needs, however I'm not sure if I used them correct or not.

Below is the entire project's source. I'm really eager to learn c++ and would like to know what areas I should be looking to improve myself and how much of my thought proccess was actually correct in my approach.

Initially I wanted to pass a "mapData" reference to my renderer. Bu then I couldn't manage to initialize a std::vector>& in base Renderer and then I decided to use a pointer container instead.

Also I'm having a hard time getting a grasp of when to use references and when to use pointers (hence my struggle with Renderer.uptrMapData).

One last problem I solved but didn't understand what went wrong; My initial thought was that if I avoid raw pointers and use std containers, I wouldn't need to worry much about "delete"ing those resources. However, if I remove this line from GameEngine.cpp:

if (p_renderer != nullptr)
    {
        p_renderer.release();
    }

My program crashes on quit (after Renderer's destructor called, to be more specific) with an "can't delete incomplete object" exception being thrown in memory library.

My tests confirmed that destructors are called in proper order (i.e: Win32ConsoleRenderer's destructor first, and then Renderer's destructor) but I didn't quite understand what object would left incomplete in that situation.

My development environment is Visual Studio 2017 and I used it's compiler.

Snake.h

#ifndef SNAKE_H
#define SNAKE_H

namespace SnakeGame
{
    struct Snake
    {
        unsigned headX;
        unsigned headY;
        unsigned bodyLength;
        int currentDirection;

        void move(int dX, int dY)
        {
            headX += dX;
            headY += dY;
        }
    };
}

#endif

Shared.h

#ifndef SHARED_H
#define SHARED_H

namespace SnakeGame
{
    namespace Shared
    {
        constexpr unsigned MAP_WIDTH = 40;
        constexpr unsigned MAP_HEIGHT = 30;
        constexpr unsigned FPS = 10;

        enum MapTile
        {
            wall = -2,
            food,
            walkable,
            snakeBody
        };

        enum Direction
        {
            up,
            right,
            down,
            left
        };
    }
}

#endif

Map.h

#ifndef MAP_H
#define MAP_H

#include <vector>

namespace SnakeGame
{
    class Map
    {
    public:
        Map(unsigned mapWidth, unsigned mapHeight);
        void initalizeMapData();
        void generateFood();
        void clearFood();

        //Methods
        int getMapValue(unsigned x, unsigned y);
        void setMapValue(unsigned x, unsigned y, int val);
        void clearSnakeTiles();

        //Accessors
        const std::vector<std::vector<int>>& getMapData() const;

    private:
        unsigned m_width, m_height;
        unsigned m_lastFoodX, m_lastFoodY;
        std::vector<std::vector<int>> m_mapData;
    };
}

#endif

Map.cpp

#include "Map.h"
#include "Shared.h"

SnakeGame::Map::Map(unsigned mapWidth, unsigned mapHeight) :
    m_width(mapWidth),
    m_height(mapHeight),
    m_lastFoodX(0),
    m_lastFoodY(0),
    m_mapData(mapHeight, std::vector<int>(mapWidth, 0))
{
}

void SnakeGame::Map::initalizeMapData()
{
    for (unsigned i = 0; i < m_width; i++)
    {
        //all columns of the first row is wall.
        m_mapData[0][i] = Shared::MapTile::wall;
        //all columns of the last row is wall.
        m_mapData[m_height - 1][i] = Shared::MapTile::wall;
    }

    for (unsigned i = 0; i < m_height; i++)
    {
        //first column of each row is wall.
        m_mapData[i][0] = Shared::MapTile::wall;
        //last column of each row is wall.
        m_mapData[i][m_width - 1] = Shared::MapTile::wall;
    }
}

void SnakeGame::Map::generateFood()
{
    unsigned x, y;
    do
    {
        x = rand() % (m_width - 2) + 1;
        y = rand() % (m_height - 2) + 1;
    } while (m_mapData[y][x] != Shared::MapTile::walkable);

    m_mapData[y][x] = Shared::MapTile::food;
    m_lastFoodX = x;
    m_lastFoodY = y;
}

void SnakeGame::Map::clearFood()
{
    m_mapData[m_lastFoodY][m_lastFoodX] = Shared::MapTile::walkable;
}

int SnakeGame::Map::getMapValue(unsigned x, unsigned y)
{
    if (x >= m_width) x = m_width - 1;
    if (y >= m_height) y = m_height - 1;

    return m_mapData[y][x];
}

void SnakeGame::Map::setMapValue(unsigned x, unsigned y, int val)
{
    m_mapData[y][x] = val;
}

const std::vector<std::vector<int>>& SnakeGame::Map::getMapData() const
{
    return m_mapData;
}

void SnakeGame::Map::clearSnakeTiles()
{
    for (unsigned y = 0; y < m_height; y++)
    {
        for (unsigned x = 0; x < m_width; x++)
        {
            if (m_mapData[y][x] > Shared::MapTile::walkable)
            {
                m_mapData[y][x]--;
            }
        }
    }
}

GameEngine.h

#ifndef GAMEENGINE_H
#define GAMEENGINE_H

#include "Map.h"
#include "Snake.h"
#include <chrono>
#include "Win32ConsoleRenderer.h"

namespace SnakeGame
{
    class GameEngine
    {
        typedef std::chrono::milliseconds ms;
        typedef std::chrono::high_resolution_clock clock;

    public:
        explicit GameEngine(Core::Renderer* const pRenderer);
        ~GameEngine();

        //Methods
        void run();

        //Accessors
        unsigned getScore() const { return m_score; }

    private:
        bool m_running;
        int m_score;
        int m_msPerFrame;
        Map m_map;
        Snake m_snake;
        std::unique_ptr<Core::Renderer> p_renderer;

        //Methods
        void processInput();
        void update();
        void draw() const;
    };
}

#endif

GameEngine.cpp

#include <conio.h>
#include <thread>
#include "GameEngine.h"
#include "Shared.h"

SnakeGame::GameEngine::GameEngine(Core::Renderer* const pRenderer) :
    m_running(false),
    m_score(0),
    m_msPerFrame(1000 / Shared::FPS),
    m_map(Shared::MAP_WIDTH, Shared::MAP_HEIGHT),
    m_snake{ Shared::MAP_WIDTH / 2, Shared::MAP_HEIGHT / 2, 3, 0 },
    p_renderer(pRenderer)
{
    if (p_renderer != nullptr)
    {
        p_renderer->setMapData(m_map.getMapData());
    }
}

SnakeGame::GameEngine::~GameEngine()
{
    if (p_renderer != nullptr)
    {
        p_renderer.release();
    }
}

void SnakeGame::GameEngine::run()
{
    //initialize map walls
    m_map.initalizeMapData();
    //place snake object at it's initialized position (center of the map)
    m_map.setMapValue(m_snake.headX, m_snake.headY, Shared::MapTile::snakeBody);
    //generate first food on the map.
    m_map.generateFood();
    //set game state to running
    m_running = true;

    //main game loop
    while (m_running)
    {
        auto start = clock::now();

        //process user input
        processInput();

        //update game objects & conditions
        update();

        //draw (render) the scene.
        draw();

        auto sleep = std::chrono::duration_cast<ms>(start + ms(m_msPerFrame) - clock::now());
        std::this_thread::sleep_for(sleep);
    }
}

void SnakeGame::GameEngine::processInput()
{
    //check if there is a keyboard interrupt
    if (_kbhit())
    {
        auto key = static_cast<char>(_getch());
        switch (key)
        {
        case 'w':
            if (m_snake.currentDirection != Shared::Direction::down)
            {
                m_snake.currentDirection = Shared::Direction::up;
            }
            break;
        case 's':
            if (m_snake.currentDirection != Shared::Direction::up)
            {
                m_snake.currentDirection = Shared::Direction::down;
            }
            break;
        case 'a':
            if (m_snake.currentDirection != Shared::Direction::right)
            {
                m_snake.currentDirection = Shared::Direction::left;
            }
            break;
        case 'd':
            if (m_snake.currentDirection != Shared::Direction::left)
            {
                m_snake.currentDirection = Shared::Direction::right;
            }
            break;
        default:;
        }
    }
}


void SnakeGame::GameEngine::update()
{
    //update snake position
    switch (m_snake.currentDirection)
    {
    case Shared::Direction::up:
        m_snake.move(0, -1);
        break;
    case Shared::Direction::right:
        m_snake.move(1, 0);
        break;
    case Shared::Direction::left:
        m_snake.move(-1, 0);
        break;
    case Shared::Direction::down:
        m_snake.move(0, 1);
        break;
    default:;
    }

    int currentMapValue = m_map.getMapValue(m_snake.headX, m_snake.headY);
    //check if we hit a food
    if (currentMapValue == Shared::MapTile::food)
    {
        //increase snake body length
        m_snake.bodyLength++;
        //clear current food
        m_map.clearFood();
        //generate new food.
        m_map.generateFood();
        //update score
        m_score += 10;
    }
    else if (currentMapValue != Shared::MapTile::walkable)
    {
        m_running = false;
    }

    m_map.clearSnakeTiles();
    m_map.setMapValue(m_snake.headX, m_snake.headY, m_snake.bodyLength);
}

void SnakeGame::GameEngine::draw() const
{
    if (m_running)
    {
        p_renderer->render(Shared::MAP_WIDTH, Shared::MAP_HEIGHT);
    }
    else
    {
        p_renderer->clearScreen();
    }
}

Renderer.h

#ifndef RENDERER_H
#define RENDERER_H
#include <vector>
#include <memory>

namespace SnakeGame
{
    namespace Core
    {
        class Renderer
        {
        public:
            Renderer();
            virtual ~Renderer();
            virtual void setMapData(const std::vector<std::vector<int>>& mapData) = 0;
            virtual void render(unsigned sceneWidth, unsigned sceneHeight) = 0;
            virtual void clearScreen() = 0;
        protected:
            std::unique_ptr<const std::vector<std::vector<int>>> uptrMapData;
        };
    }
}

#endif

Renderer.cpp

#include "Renderer.h"

SnakeGame::Core::Renderer::Renderer() : uptrMapData(nullptr)
{

}

SnakeGame::Core::Renderer::~Renderer()
{
    if(uptrMapData != nullptr)
    {
        uptrMapData.release();
    }
}

Win32ConsoleRenderer.h

#ifndef WIN32_CONSOLE_RENDERER_H
#define WIN32_CONSOLE_RENDERER_H
#define NOMINMAX
#define WIN32_LEAN_AND_MEAN
#include <vector>
#include <Windows.h>
#include "Shared.h"
#include "Renderer.h"

namespace SnakeGame
{
    namespace Core
    {
        class Win32ConsoleRenderer : public Renderer
        {
        public:
            Win32ConsoleRenderer();
            ~Win32ConsoleRenderer();
            void setMapData(const std::vector<std::vector<int>>& mapData) override;
            void render(unsigned sceneWidth, unsigned sceneHeight) override;
            void clearScreen() override;
        private:
            bool m_buffered;
            std::vector<std::vector<int>> m_mapDataCache;
            static HANDLE m_outputHandle;
            static void setWindowSize();
            static void setCursorPosition(unsigned x, unsigned y);
            static void hideCursor();
            static char mapValueToChar(Shared::MapTile mapValue);
            void initializeBuffer();
        };
    }
}

#endif

Win32ConsoleRenderer.cpp

#include "Win32ConsoleRenderer.h"
#include <iostream>

HANDLE SnakeGame::Core::Win32ConsoleRenderer::m_outputHandle = nullptr;

SnakeGame::Core::Win32ConsoleRenderer::Win32ConsoleRenderer() :
    m_buffered(false),
    m_mapDataCache({})
{
    m_outputHandle = GetStdHandle(STD_OUTPUT_HANDLE);
    Win32ConsoleRenderer::clearScreen();
    setWindowSize();
    hideCursor();
}

SnakeGame::Core::Win32ConsoleRenderer::~Win32ConsoleRenderer()
{
    if (this->uptrMapData != nullptr)
    {
        this->uptrMapData.release();
    }
}

void SnakeGame::Core::Win32ConsoleRenderer::setMapData(const std::vector<std::vector<int>>& mapData)
{
    uptrMapData.reset(&mapData);
    m_mapDataCache.resize(uptrMapData->size());
    initializeBuffer();
}

void SnakeGame::Core::Win32ConsoleRenderer::render(unsigned sceneWidth, unsigned sceneHeight)
{
    for (unsigned y = 0; y < sceneHeight; y++)
    {
        for (unsigned x = 0; x < sceneWidth; x++)
        {
            auto currentMapValue = this->uptrMapData->at(y)[x];
            auto cachedMapValue = m_mapDataCache[y][x];
            if (currentMapValue == cachedMapValue) continue;

            this->m_mapDataCache[y][x] = currentMapValue;
            setCursorPosition(x, y);
            std::cout << mapValueToChar(static_cast<Shared::MapTile>(currentMapValue));
        }
    }

    std::cout.flush();
}

void SnakeGame::Core::Win32ConsoleRenderer::setWindowSize()
{
    HWND windowHandle = GetConsoleWindow();
    RECT r;

    GetWindowRect(windowHandle, &r);
    MoveWindow(windowHandle, r.left, r.top, Shared::MAP_WIDTH * 10, Shared::MAP_HEIGHT * 20, TRUE);
}

void SnakeGame::Core::Win32ConsoleRenderer::clearScreen()
{
    CONSOLE_SCREEN_BUFFER_INFO bufferInfo;
    COORD topLeft{ 0,0 };

    std::cout.flush();

    if (!GetConsoleScreenBufferInfo(m_outputHandle, &bufferInfo))
    {
        std::cout << "BUFFER ERROR" << std::endl;
    }

    DWORD length = bufferInfo.dwSize.X * bufferInfo.dwSize.Y;
    DWORD written;

    FillConsoleOutputCharacter(m_outputHandle, TEXT(' '), length, topLeft, &written);
    FillConsoleOutputAttribute(m_outputHandle, bufferInfo.wAttributes, length, topLeft, &written);

    SetConsoleCursorPosition(m_outputHandle, topLeft);
}

void SnakeGame::Core::Win32ConsoleRenderer::setCursorPosition(unsigned x, unsigned y)
{
    COORD coord{ static_cast<short>(x), static_cast<short>(y) };
    SetConsoleCursorPosition(m_outputHandle, coord);
}

void SnakeGame::Core::Win32ConsoleRenderer::hideCursor()
{
    CONSOLE_CURSOR_INFO cursorInfo{ 100,FALSE };
    SetConsoleCursorInfo(m_outputHandle, &cursorInfo);
}

char SnakeGame::Core::Win32ConsoleRenderer::mapValueToChar(Shared::MapTile mapValue)
{
    switch (mapValue)
    {
    case Shared::MapTile::wall:
        return '=';
    case Shared::MapTile::food:
        return '@';
    case Shared::MapTile::snakeBody:
        return 'o';
    case Shared::MapTile::walkable:
        return ' ';
    default:
        return 'o';
    }
}

void SnakeGame::Core::Win32ConsoleRenderer::initializeBuffer()
{
    if (!m_buffered)
    {
        for (unsigned int y = 0; y < m_mapDataCache.size(); y++)
        {
            auto colSize = this->uptrMapData->at(y).size();
            m_mapDataCache[y].resize(colSize);
        }

        m_buffered = true;
    }
}

main.cpp

#include "GameEngine.h"
#include <iostream>

int main()
{

    //Create renderer
    SnakeGame::Core::Win32ConsoleRenderer renderer;
    //Create game engine and pass renderer to it.
    SnakeGame::GameEngine snakeGame(&renderer);
    //run the game.
    snakeGame.run();

    std::cout << "== Game over! ==" << std::endl;
    std::cout << "Your score: " << snakeGame.getScore() << std::endl;
    std::cin.ignore();

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ For some reason people are voting to close this question. Apparently they have the impression your code currently does not work as intended. While I don't see anything obvious at first glance, it might be a good idea to mention explicitly whether the current version works as intended or not. A request for optimization and you mentioning 'wrong approaches to improve from the past' are not enough grounds to close this question. \$\endgroup\$ – Mast Jun 8 '17 at 17:46
  • \$\begingroup\$ Strange? The code compiles (with MS compiler) and works as intended -like you can play the game, controls work, map works, game logic works- I specifically stated that development was on VS2017 and on Windows platform. Any ideas for me to alleviate those misunderstandings? \$\endgroup\$ – Tequilalime Jun 9 '17 at 9:54
  • \$\begingroup\$ Good enough for me. My comment is primarily intended to prevent more (uninformed) close votes. Looks like a decent question to me, although it never hurts to read our checklist on how to write a good question :-) \$\endgroup\$ – Mast Jun 9 '17 at 11:58
1
\$\begingroup\$

One problem I see is with your use of unique_ptr for p_renderer in GameEngine.

Fundamentally, you have a problem with ownership of the Renderer object. Assigning a pointer to a unique_ptr variable is a transfer of ownership of that pointer to the variable. Once that happens, the original pointer (or object) should not call delete or the destructor for it.

The GameEngine constructor is assuming that it should take ownership of the Renderer pointer it is given, while the caller of the constructor (main) keeps ownership (since the pointer is to a local, stack based variable).

The fix is to change the definition of p_renderer to be just a pointer (Core::Renderer *p_renderer;) and not do anything with it in the destructor. Leave it up to the caller to clean up (delete) the pointer.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, from your response I get that my understanding of RAII is wrong maybe. I initially thought by passing p_renderer to GameEngine, I mean "hey, GameEngine here is your renderer and it's yours to manage now". But since the original allocator of p_renderer is main itself, it should be main's job to delete it correct? What would happen if I didn't pass renderer to GameEngine but GameEngine created it's own renderer inside constructor or a "initRenderer()" method? \$\endgroup\$ – Tequilalime Jun 9 '17 at 10:02
  • \$\begingroup\$ @Alaminut If you really want to transfer ownership to GameEngine you should make it explicit by changing constructor parameter to std::unique_ptr<Core::Renderer>. In main write auto pRenderer = std::make_unique<Core::Renderer>() to allocate renderer, then SnakeGame::GameEngine snakeGame(std::move(pRenderer)); to transfer the ownership to snakeGame. But personally I would go with the suggestion in this answer here. \$\endgroup\$ – zett42 Jun 9 '17 at 12:30

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.