Console Snake Game in C++ with Classes

Question

I would appreciate some criticsm on my code so that I could improve it. I tried making the Snake game in OOP but don't think I did it right. Any suggestions would be appreciated.

The code is is commented in my language which isn't english hope that won't be a problem :)

Here is the code:

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <vector>
#include <conio.h>
#include <windows.h>
using namespace std;
//Mihael Petricevic

enum objects {EMPTY, WALL, SNAKE_HEAD, SNAKE_TAIL, FRUIT};
objects** Map;

//KONSTANTE
const int m_x = 40;
const int m_y = 10;

class GameObject
{
public:

    //INHERITED BY CHILD CLASSES
    virtual void update() = 0;
};

class Snake : public GameObject
{
public:

   void update();

private:
    int _x = 19;
    int _y = 4;
    int _x_old, _y_old;
    int _direction = 3;
    int _tsize = 0;

    //PRIVATE STRUCT
    struct Tail
    {
        int x;
        int y;
    };
    //ARRAY OR TAILS
    vector<Tail>tails;

    //HELPER METHODS
    void _KeyBoard();
    void _MoveSnake();
    void _AddTail();
    bool _Ate();
    void _MoveTail();
    void _Collision();
};

/*

    UZIMA INPUT OD IGRACA I MJENJA SMJER ZMIJE

*/

void Snake::_KeyBoard()
{
    /*
        1                up
    2       3     left        right
        4               down
    */

    if (kbhit())
    {
        char input = getch();

        if (input == 'w')
        {
            _direction = 1;
        }
        else if (input == 'a')
        {
            _direction = 2;
        }
        else if (input == 'd')
        {
            _direction = 3;
        }
        else if (input == 's')
        {
            _direction = 4;
        }
    }
}

/*
*/

void Snake::_MoveSnake()
{
    //SPREMA ZADNJE KORDINATE PRIJE PROMJENE ZA PRVI REP
    _x_old = _x;
    _y_old = _y;

    switch(_direction)
    {
    case 1:
        {
            _y--;
            break;
        }
    case 2:
        {
            _x--;
            break;
        }
    case 3:
        {
            _x++;
            break;
        }
    case 4:
        {
            _y++;
            break;
        }
    }

    //STAVLJA MJESTO GLAVE ZMIJE U IGRACE POLJE
    Map[_y_old][_x_old] = EMPTY;
    Map[_y][_x] = SNAKE_HEAD;
}

/*

    FUNKCIJA DODAVA NOVI REP
    - NA POCETKU DODAVA 2 REPA JER TOLIKO IMA ZMIJA POCETNO
    - KASNIJE AKO JE POJELA VOCE SE DODAVA JOS JEDAN IZA ZADNJEG BAZIRAN O DIREKCIJU U KOJOJ SE MICE ZMIJA

*/

void Snake::_AddTail()
{
    //NA POCETKU IGRE ZMIJA IMA 2 REPA
    if (_tsize == 0)
    {
        for (int i=0;i<2;i++)
        {
            tails.push_back(Tail());
            tails[i].x = 19 - i;    //OVO SAMO STAVLJA REPA 1 IZA GLAVE PA ONDA 2 IZA GLAVE (NIJE 18 JER SE POMAKNE ZA 1 ODMA CIM SE STVORI OBJEKT SNAKE)
            tails[i].y = 4;

            //STAVLJANJE REPA U IGRACE POLJE
            Map[tails[i].y][tails[i].x] = SNAKE_TAIL;
            //POVECANJE BROJA REPA KOJI JE NA POCETKU 0
            _tsize++;
        }
    }

    if (_Ate())
    {
        //DODAVA NOVI REP
        tails.push_back(Tail());

        //ODREDIVANJA KORDINATA NOVOG REPA
        switch(_direction)
        {
        case 1:
            {
                tails[_tsize].x = tails[_tsize - 1].x;
                tails[_tsize].y = tails[_tsize - 1].y - 1;
                break;
            }
        case 2:
            {
                tails[_tsize].x = tails[_tsize - 1].x + 1;
                tails[_tsize].y = tails[_tsize - 1].y;
                break;
            }
        case 3:
            {
                tails[_tsize].x = tails[_tsize - 1].x - 1;
                tails[_tsize].y = tails[_tsize - 1].y;
                break;
            }
        case 4:
            {
                tails[_tsize].x = tails[_tsize - 1].x;
                tails[_tsize].y = tails[_tsize - 1].y + 1;
                break;
            }
        }
        //POVECAVA SE KOLIKI JE BROJ REPA
        _tsize++;
    }
}

/*

    GLEDA AKO JE ZMIJA POJELA VOCE

*/

bool Snake::_Ate()
{
    //AKO  ZMIJA IDE U SMJERU VOCA I AKO JE VOCE JEDNO POLJE ISPRED ONDA ZNACI DA GA JE IGRAC POJEO
    if (_direction == 1 && Map[_y - 1][_x] == FRUIT)
    {
        return true;
    }
    else if (_direction == 2 && Map[_y][_x - 1] == FRUIT)
    {
        return true;
    }
    else if (_direction == 3 && Map[_y][_x + 1] == FRUIT)
    {
        return true;
    }
    else if (_direction == 4 && Map[_y + 1][_x] == FRUIT)
    {
        return true;
    }

    return false;
}

/*

    POMICE ZMIJIN REP TAKO DA UZIMA KORDINATE ZADNJEG REPA I PREBACUJE GA NE SLJEDECI

*/

void Snake::_MoveTail()
{
    //ZADNJI REP SE BRISE
    Map[tails[_tsize - 1].y][tails[_tsize - 1].x] = EMPTY;

    //UZIMA KORDINATE ZADNJEG REPA BRISE SVOJE MJESTO I UZIMA KORDINATE SLJEDECOG REPA
    for (int i=_tsize - 1;i!=0;i--)
    {
        tails[i].x = tails[i-1].x;
        tails[i].y = tails[i-1].y;
    }
    //STAVLJAM DA JE PRVI REP SADA NA STAROM MJESTU GLAVE ZMIJE
    tails[0].x = _x_old;
    tails[0].y = _y_old;

    //STAVLJA DA JE PRVI REP NA POZICIJ
    Map[_y_old][_x_old] = SNAKE_TAIL;
}

/*

    PROVJERAVA DA LI JE IGRAC IZGUBIO

*/

void Snake::_Collision()
{
    //JE LI IGRAC UDARIO ZID
    if (_x == 0 || _x == 39)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }

    if (_direction == 1 && Map[_y - 1][_x] == WALL || _direction == 4 && Map[_y + 1][_x] == WALL)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }

    //PROVJERA AKO JE IGRAC SE ZALETIO U SVOJ REP
    if (_direction == 1 && Map[_y - 1][_x] == SNAKE_TAIL)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }
    else if (_direction == 2 && Map[_y][_x - 1] == SNAKE_TAIL)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }
    else if (_direction == 3 && Map[_y][_x + 1] == SNAKE_TAIL)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }
    else if (_direction == 4 && Map[_y + 1][_x] == SNAKE_TAIL)
    {
        cout << "YOU LOST, GET GUD SCRUB!";
        system("pause");
        exit(0);
    }
}

/*

    ZOVE SVE HELPER METHODS ZA ZMIJU STVARA LOOP UPDATA SVE PODATKE O ZMIJI I NJEZINOM REPU

*/

void Snake::update()
{
    _KeyBoard();
    _AddTail();
    _Collision();
    _MoveSnake();
    _MoveTail();
}

class Fruit : public GameObject
{
public:

   void update();

private:
    int _x;
    int _y;

    //HELPER METHODS
    void _spawnFruit();
    bool _check_for_fruit();
};

/*

    STVARA VOCE NA RANDOM LOKACIJI NA MAPI

*/

void Fruit::_spawnFruit()
{
    int x,y;
    x = 1 + rand() % 38;
    y = 1 + rand() % 8;

    Map[y][x] = FRUIT;
}

/*

    PROVJERAVA AKO JE KORISNIK POJEL VOCE TJ. AKO IMA VOCA NA MAPI

*/

bool Fruit::_check_for_fruit()
{
    for (int i=1;i<m_y - 1;i++)
    {
        for (int j=1;j<m_x - 1;j++)
        {
            if (Map[i][j] == FRUIT)
            {
                return true;
            }
        }
    }
    return false;
}

/*

    ZOVE SVE HELPER METHODS ZA VOCE I STVARA GA AKO GA NEMA NA IGRACEM POLJU

*/

void Fruit::update()
{
    if (!_check_for_fruit())
    {
        _spawnFruit();
    }
}

/*

    STVARA LISTU KOJA UPDATA SVAKI OBJEKT IGRE

*/

void Scene(vector<GameObject*>&updates)
{
    //AKO NEMA OVOGA SE BUDE SVAKI PUT SPREMILA NOVA INSTANCA OBJEKTA SNAKE I FRUIT TE BUDU IMALI DEFAULT VRIJEDNOST I NEBUDU SE UOPCE MJENJALI
    //GLEDAM AKO JE VEKTOR PRAZAN I AKO JE GA POPUNJAVAM SA NOVIM INSTANCAMA OBJEKTI
    //ISTO KAO DA NAPRAVIM Snake a.update(); I ONDA NAKON OPET STVARAM TAJ OBJEKT PA SE ON RESETIRA PA ZA TO SLUZI IF
    //MOGLO SE SAMO I NAPISATI TU Snake a.update(); ISTO BI DOSLO ALI MORA BITI OVAJ IF
    if (updates.size() == 0)
    {
        //SPREAMNJE SUBKLASI U VEKTOR TAKO DA SLAZEMO NOVI POINTER PREMA TOJ SUBKLASI
        updates.push_back(new Snake());
        updates.push_back(new Fruit());
    }

    //ZOVE SE UPDATE SVAKE SUBKLASSE
    for (vector<GameObject*>::iterator itr = updates.begin(), end = updates.end();itr != end; itr++)
    {
        (*itr)->update();
    }


}

/*

    CRTA IGRACE POLJE I OBJEKTE IGRE

*/

void draw()
{
    //REFRESH SCREEN
    COORD cur = {0,0};
    SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), cur);

    //CRATNJE IGRACEG POLJA
    for (int i=0;i<m_y;i++)
    {
        for (int j=0;j<m_x;j++)
        {
            if (Map[i][j] == WALL)
            {
                cout << "*";
            }
            else if (Map[i][j] == EMPTY)
            {
                cout << " ";
            }
            else if (Map[i][j] == SNAKE_HEAD)
            {
                cout << "O";
            }
            else if (Map[i][j] == SNAKE_TAIL)
            {
                cout << "o";
            }
            else if (Map[i][j] == FRUIT)
            {
                cout << "+";
            }
        }
        cout << endl;
    }
}

/*

    FUNKCIJA MAIN()

*/

int main()
{
    //srand(time(0));

    //KREACIJA MAPE
    Map = new objects*[m_y];
    for (int i=0;i<m_y;i++)
    {
        Map[i] = new objects[m_x];
    }

    //ISPUNJAVANJE MAPE
    for (int i=0;i<m_y;i++)
    {
        for (int j=0;j<m_x;j++)
        {
            if (i == 0 || i == m_y-1 || j == 0 || j == m_x-1)
            {
                Map[i][j] = WALL;
            }
            else
            {
                Map[i][j] = EMPTY;
            }
        }
    }

    //KREIRANJE LISTE KOJA UPDATA SVE OBJEKTE CLASSE GAMEOBJECT
    vector<GameObject*>updates;

    //MAIN GAME LOOP
    while (1)
    {
        draw();
        Scene(updates);
        Sleep(200);
    }


    return 0;
}

Answer 1

I see a number of things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, the pause function might be rewritten like this:

void pause_for_user()
{
    std::string line;
    std::cin >> line;
}

I called this function pause_for_user insteaad of just pause because pause is already the name of a different function in POSIX.

Use a switch instead of long if ...else chain

The pattern matching logic is much easier to see if a swtich statement is used instead of the long if...else chain. For example, instead of this:

if (Map[i][j] == WALL)
{
    cout << "*";
}
else if (Map[i][j] == EMPTY)
{
    cout << " ";
}
else if (Map[i][j] == SNAKE_HEAD)
{
    cout << "O";
}
else if (Map[i][j] == SNAKE_TAIL)
{
    cout << "o";
}
else if (Map[i][j] == FRUIT)
{
    cout << "+";
}

one could instead write this:

switch (Map[i][j]) {
    case WALL:
        cout << '*';
        break;
    case EMPTY:
        cout << ' ';
        break;
    case SNAKE_HEAD:
        cout << 'O';
        break;
    case SNAKE_TAIL:
        cout << 'o';
        break;
    case FRUIT:
        cout << '+';
        break;
}

It's shorter and a little easier to read. However, see the next suggestion.

Shorten code by simplifying conversion

The code represents the various screen objects using an enum. A plain enum in C++ can still be used as an int which can be handy here. The enum declaration is this:

enum objects {EMPTY, WALL, SNAKE_HEAD, SNAKE_TAIL, FRUIT};

so one way to write the contents of draw() would be this:

static constexpr char object_rep[]{" *Oo+"};
for (int i=0; i < m_y; ++i) {
    for (int j=0; j < m_x; ++j) {
        std::cout << object_rep[Map[i][j]];
    }
    std::cout << '\n';
}

However, we can simplify still further and make it much easier to see the association between the object representation and the object itself:

enum objects {EMPTY=' ', WALL='*', SNAKE_HEAD='O', SNAKE_TAIL='o', FRUIT='+'};

Now the printing looks like this:

for (int i=0; i < m_y; ++i) {
    for (int j=0; j < m_x; ++j) {
        std::cout << static_cast<char>(Map[i][j]);
    }
    std::cout << '\n';
}

Avoid the use of global variables

I see that Map is declared as a global variable rather than as a local variable. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.

Use "range for" and simplify your code

The code currently contains these lines:

for (std::vector<GameObject*>::iterator itr = updates.begin(), end = updates.end();itr != end; itr++)
{
    (*itr)->update();
}

It's much easier to read an understand like this:

for (auto &item : updates) {
    item->update();
}

Rethink your object design

The Map variable and the updates vector and the Snake and Fruit objects are related but only by their use rather than their structure. I'd suggest that instead there could be a Game object that would contain, effectively, the Map and Snake and Fruit objects, as well as having member functions draw() and update() and a constructor that performs the functions that are currently done at the beginning of main. Also, since Fruit and Snake both derive from GameObject and both have x and y coordinates, the coordinates probably belong in the base class. In my rewritten version, here's what main looks like now:

int main()
{
    Game game;
    game.play();
}

I would also recommend using a Point class to store the coordinates of items.

Consider using a better random number generator

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_real_distribution and friends in the <random> header.

Fix the bug

The is a small chance that when the snake eats a fruit that the next fruit is generated on top of the snake. Better would be to make sure that the fruit is only place on an empty square.

Eliminate magic numbers

The constants 38, 19 and others are used in multiple places. It would be better to have them as named const values so that it would be clear what those numbers represent.

Don't use leading underscores in names

Anything in global space with a leading underscore is a reserved name in C++ (and in C). See this question for details.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

for (int i=0;i<m_y;i++)

most people find it more easily readable if you use more space:

for (int i=0; i < m_y; ++i)

Don't Repeat Yourself (DRY)

In the Snake::_Collision() function, there is a lot of repeated code. This could be restructed to be much easier to read and understand. Essentially, the job of the function is to find out whether the snake is about to run into something. Here's another way to do it:

void Snake::_Collision(Game &game)
{
    switch (game.at(next(game._direction, location))) {
        case Game::EMPTY:
            break;
        case Game::FRUIT:
            break;
        default:
            std::cout << "YOU LOST, GET GUD SCRUB!";
            pause_for_user();
            exit(0);
    }
}

This assumes a number of changes. For instance, there is a next member function that takes a direction and a Point and and returns the next Point. There is also an at function that returns a reference to the map location given the Point.

Answer 2

Welcome to Code Review. I hope my criticism will be constructive and you take it as opportunity to improve yourself. We all are here to learn something, including myself.

using namespace std

This may be the easy way, but not so good, it can cause a lot of trouble and did for many people. You simply don't know what is inside std and what names are you bringing in by this. The collisions can surprise you, code not behaving as you think, because some calls are not what they appear to be.

You have two choices:

1. Always use the prefix, e.g. std::vector. Not so much typing and saves a lot of trouble. Improves readability.
2. Selective usings like using std::vector; using std::whatever.... A lot of lines but you can place these in some common header that you may reuse (and precompile). You will at least know what you are using and avoid the main issue with using namespace std;.

objects** Map

This sounds more like old C than C++. Consider std::vector or std::array. I would personally use std::vector to be able to have levels of different dimensions.

enum objects

It is good that you are not using hard-coded numbers but enum (and not #define), but it could be even better to create class Tile, that also knows how to draw itself (and possibly more, whatever you need).

class Tile {
public:
    enum Kind { EMPTY, WALL, HEAD, TAIL, FRUIT };
    Kind kind;
    Tile(Kind kind = EMPTY): kind(kind) {}
    char screenChar() {
        switch(kind)
        {
        default:
            return ' ';
        case WALL:
            return '*';
        //...
        }
    }
    void draw(std::ostream& out) {
        out << screenChar();
    }
};
class Level {
    std::vector<Tile> tiles;
    size_t width;
public:
    void setDimensions(int width, int height) {
        //maybe tiles.resize(0) first to clean it
        tiles.resize(width*height);
        this->width = width;
        // for..for... initialize the wall as in your main()
    }
    Tile& operator[](size_t x, size_t y) {
        assert(x < width && x+y*width < tiles.size());
        return tiles[x+y*width];
    }
    void draw() { draw(std::cout); }
    void draw(std::ostream& out) {
        for (int i = 0; i < tiles.size();) {
            tiles[i].draw(out);
            if ((++i % width) == 0) out << std::endl;
        }
    }
};

Now this looks more C++ and changes the map from array of arrays to internally flat vector with two-dimensional access (operator[x,y]) and resize, protecting what you should not access/change directly (the vector). It also introduces assert (good for finding bugs) and some basic generalization in screenChar and draw, the later accepting any ouput stream (std::ostream) instead of hard-coded std::cout (but still with default draw() using std::cout).

int _direction = 3;

You have correctly used enum objects but now using hard-coded constants. Not good, use enum here as well. Improves readability and avoids problems (e.g. assigning wrong value, refactoring).

struct Tail

Good, but it would probably be better to have global struct Point.

void Snake::_KeyBoard()

switch is better than sequence of if..else if and I think that this logic belongs to game-execution engine rather than to snake. Snake knows its direction, but should be commanded by player/engine, not consuming keyboard input directly. And if you want some logic inside Snake, then it should rather be something like event - bool processKeyboardInput(char key) and kbhit() + getch() should be outside. Snake reacts to key-press, but should not access keyboard, somebody has to inform snake to react to the key. The engine can process other keys, e.g. ESC.

Snake::_AddTail() + _Ate()

This is a bit confusing. _AddTail does not always add tail, as the name would suggest, but rather first checks if it has to add a tail and then adds it if the check is successful. It also somehow handles part of level initialization. That should be separated / redesigned (update() should check _Ate to maybe call _AddTail and startup initialization should be completely separated and made part of level initialization).

void Snake::_Collision()

Repeated code and again part of level-logic inside snake. bool Snake::isColliding() should test and some game-engine should ask (each update) and respond accordingly (cout << ...; system("pause"); exit(0);).

Fruit::_check_for_fruit() + update() + _spawnFruit()

I can understand your grouping of methods, but I would rather make it kind of state machine. If snake eats a fruit then new should be spawned, rather then having fruit check each update if it exists and spawn if not.