HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Snake-like console game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
consolegamelikesnake

Problem

This is what I came up with when trying to make my first console game. I think it turned out quite okay so I wanted to know what you think about it.
If there is anything to improve, may it be coding style, readability, efficiency or anything else, please let me know.

main.cpp

#include 
#include 
#include 
#include 
#include "game.h"

int main()
{
    bool exit = false;

    std::cout   << "Controls:\n"
                << " - Use w, a, s, d to change direction.\n"
                << " - Press space to pause the game.\n\n";
    system("pause");
    system("cls");

    while( !exit )
    {
        size_t hight = 20, width = 20;

        Game game(hight , width);
        game.StartGame();
        while( !game.isGameOver )
        {
            game.Update();
            Sleep( 100 );
        }

        std::cout << "\nGame Over.\nPress 1 to play again.\nPress Esc to exit.";
        char input = 0;
        while( input != '1' && input != 27 )
        {
            input = getch();
        }

        if( input == 27)
            exit = true;

        system("cls");
    }

    return 0;
}


field.h

#ifndef FIELD_H
#define FIELD_H

#include 
#include 

class Field
{
public:
    // Constructor & Destructor
    Field( size_t hight, size_t width );
    ~Field();

    // Methods
    void Draw();

    // Members
    char **field;
    enum BlockType{ EMPTY = ' ', WALL = '#', FOOD = '*', sHEAD = 2, sBODY = 'o' };

private:
    size_t HIGHT;
    size_t WIDTH;
};

#endif // FIELD_H


field.cpp

```
#include "field.h"

Field::Field( size_t hight, size_t width )
: field( nullptr ), HIGHT( hight ), WIDTH( width )
{
// allocate field array
field = new char* [HIGHT];

for( size_t i = 0; i < HIGHT; i++)
{
field[i] = new char[WIDTH];
}

// initialize field array
for( size_t i = 0; i < WIDTH; i++ )
field[0][i] = field[HIGHT - 1][i] = WALL; // first and last line are borders

for( size_t i = 1; i < HIGHT - 1; i++ )
{

Solution

I have found a couple of things that could help you improve your code.

Don't use system("cls")

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, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}


Isolate platform-specific code

In this code, there are several things that are DOS/Windows only including #include and the getch() and kbhit() functions within that, and also system("cls"); that I've already mentioned. Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS already in the code so that one could recompile without having to alter the source code.

Consider using a better random number generator

You are currently using

x = rand() % WIDTH;


There are problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither is x. On my machine, there's a slight but measurable bias toward 0 with that. A better solution, if your compiler and library supports it, would be to use the C++11 std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use. Here's an example:

unsigned rand_uint(unsigned low, unsigned high)
{
    static std::default_random_engine re{};
    using Dist = std::uniform_int_distribution;
    static Dist uid{};
    return uid(re, Dist::param_type{low,high});
}


Eliminate "magic numbers"

There are a few numbers in the code, such as
20 and 27 that have a specific meaning in their particular context. By using named constants such as WIDTH or ESC, the program becomes easier to read and maintain.

Use correct spellings

The word "height" is misspelled as "hight" throughout the program. It's not a major point but it makes your code harder for others to use and understand if common words are not spelled correctly.

Use
const where practical

The
hight and width variables in main.cpp are never altered in the program and should be declared as const. Similarly, member functions that don't alter the underlying object, such as Field::Draw() should be declared as const.

Separate class responsibilities more completely

The
Game class reaches into the Snake class and directly manipulates Snake member variables. This is a sign that the classes are not as well designed as they could be. Specifically, it seems that much of the code that is currently in Game::PutSnake() should be delegated instead to the Snake object. To take another specific example, consider rewriting the Game::SpawnFood() function to eliminate the need for direct access to the field member data:

void Game::SpawnFood()
{
    size_t  x = 0,
            y = 0;
    while( !FIELD.isEmpty(x,y) )
    {
        x = rand_uint(0, WIDTH);
        y = rand_uint(0, HIGHT);
    }
    FIELD.dropFood(x, y);
}


Carefully separate interface from implementation

The
#include files that you put into a .h file are a part of its interface, while the ones in the corresponding .cpp file are part of the implementation. So for instance, conio.h is not required to understand and use the interface for Field and so that #include should not appear in the field.h file. It should instead be put into the field.cpp file since it's a detail of the implementation and not of the interface.

Omit
return 0

When a C++ program reaches the end of
main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main`.

Code Snippets

void cls()
{
    std::cout << "\x1b[2J";
}
x = rand() % WIDTH;
unsigned rand_uint(unsigned low, unsigned high)
{
    static std::default_random_engine re{};
    using Dist = std::uniform_int_distribution<unsigned>;
    static Dist uid{};
    return uid(re, Dist::param_type{low,high});
}
void Game::SpawnFood()
{
    size_t  x = 0,
            y = 0;
    while( !FIELD.isEmpty(x,y) )
    {
        x = rand_uint(0, WIDTH);
        y = rand_uint(0, HIGHT);
    }
    FIELD.dropFood(x, y);
}

Context

StackExchange Code Review Q#96766, answer score: 12

Revisions (0)

No revisions yet.