Snake game for C++

Question

This is my first game that I made. How could this be improved?

#include <iostream>
#include <time.h>
#include <stdlib.h>
#include <unistd.h>
#include <conio.h>

using namespace std;

int snakelenght=2;
char direction='d';
char field[20][30];

class Past
{
    int m_nx;
    int m_ny;
public:
    void Init(int x,int y)
    {
        m_nx=x;
        m_ny=y;
    }
    int GetX(){return m_nx;}
    int GetY(){return m_ny;}
 }past[50];


void MakeField();
void DrawField();
void Random(int &x, int &y);
bool IsFree(int &x,int &y);
void MakeFood();
bool Food(int &x,int &y);
bool Move(int &x, int &y);
void Erase(int x, int y);
void Past(int x,int y);
void Key(int &x,int &y);
bool End(int &x, int &y);

int main()
{
    int x=10,y=15;
    MakeField();
    MakeFood();
    DrawField();
    while(End(x,y))
    {
        cout << direction;
        if(kbhit())
        {
            direction=getch();
            Key(x,y);
        }
        else
            Key(x,y);
        if(!Move(x,y))
            break;
        Past(x,y);
        system("cls");
        DrawField();
        usleep(1000);
    }
    cout << "End!";
    return 0;
}

void MakeField()
{
    for(int i=0;i<20;i++)
        for(int j=0;j<30;j++)
            field[i][j]=' ';
}

void DrawField()
{
    for(int i=0;i<=30;i++)
        cout << "- ";
    cout << endl;
    for(int i=0;i<20;i++)
    {
        cout << "| ";
        for(int j=0;j<30;j++)
            cout << field[i][j] << " ";
        cout << "|"<< endl;
    }
    for(int i=0;i<=30;i++)
        cout << "- ";
    cout << endl;
}

void Random(int &x, int &y)
{
    srand(time(0));
    x=rand()%10;
    y=rand()%10;
}


bool IsFree(int &x,int &y)
{
    if(field[x][y]==' ')
        return 1;
    return 0;
}

void MakeFood()
{
    again:
    int x=0,y=0;
    Random(x,y);
    if(IsFree(x,y))
    field[x][y]='H';
    else
        goto again;
}

bool Food(int &x,int &y)
{
    if(field[x][y]=='H')
    {
        snakelenght++;
        return 1;
    }
    return 0;
}

bool Move(int &x, int &y)
{
    if(Food(x,y))
    {
        field[x][y]='X';
        MakeFood();
        return 1;
    }
    if(IsFree(x,y))
    {
        field[x][y]='X';
        return 1;
    }
    return 0;
}

void Erase(int x, int y)
{
    field[x][y]=' ';
}

void Past(int x,int y)
{
    Erase(past[0].GetX(),past[0].GetY());
    for (int i=0;i<snakelenght;i++)
        past[i]=past[i+1];
    past[snakelenght].Init(x,y);
}

void Key(int &x,int &y)
{
    switch(direction)
    {
        case 'd':
        {
            y++;
            break;
        }
        case 'a':
        {
            y--;
            break;
        }
        case 's':
        {
            x++;
            break;
        }
        case 'w':
        {
            x--;
            break;
        }
    }
}

bool End(int &x, int &y)
{
    if(field[x][y]==' '||'H')
        return 1;
    return 0;
}

Answer 1

using namespace std is considered a bad practice. Don't do it.

---

The magic numbers 20 and 30 for the game dimensions would be better as global constants. That way you can easily extend the game to other dimensions.

---

Instead of:

#include <time.h>
#include <stdlib.h>

You should use:

#include <ctime>
#include <cstdlib>

---

In the boolean functions, instead of returning 0 and 1 values, it would be more natural to return false and true.

---

In the case blocks of the switch statement, you don't really need the braces, you could write simpler this way:

switch(direction)
{
    case 'd':
        y++;
        break;
    case 'a':
        y--;
        break;

---

The condition in the End function looks odd:

bool End(int &x, int &y)
{
    if (field[x][y] == ' ' ||'H')
        return 1;
    return 0;
}

The condition will always evaluate to true. You can correct and simplify:

bool End(int &x, int &y)
{
    return field[x][y] == ' ' || field[x][y] == 'H';
}

Answer 2

A few remarks and pointers:

• Try moving your code and data into a class. This will be a good leaning exercise for you. Classes are a very important concept in C++, so it is important that you master them soon.

• C++11 is the current language standard (to be precise, C++14 is the current at the time of this writing, but most compilers are still catching up with it), so if you have access to C++11/14, you can replace some platform-specific calls, such as usleep, and outdated function like rand, with more modern stuff. Take a look at the sleep_for function, from the <thread> library and at the functionalities of the <random> library.

Getter methods aren't always the best way to go:

The class Past is a very simple record with two integer values. In such simple cases there is no benefit in declaring a full-blown class with get/set methods. A simple C-like struct with direct access to the fields would have been a much more elegant and straightforward solution:

struct Past
{
    int nx;
    int ny;
};

And add a constructor or two if you need them for initialization.

Use consistent spacing between arithmetical operators:

Blank spaces between operators and operands can improve the readability of your code. Compare:

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

with:

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

Don't use goto unless it is actually provem to improve readability or simplify code:

Your use of goto in this function is a gratuitous spaghettization of the code:

void MakeFood()
{
    again:
    int x=0,y=0;
    Random(x,y);
    if(IsFree(x,y))
    field[x][y]='H';
    else
        goto again;
}

You can replace it with a for or while loop:

void MakeFood()
{
    while (true) // Loop until we can find a free slot.
    {
        int x = 0, y = 0;
        Random(x, y);
        if (IsFree(x, y))
        {
            field[x][y] = 'H';
            break;
        }
    }
}

That's just one way of restructuring it in a neater way.

Don't re-seed rand every time:

void Random(int &x, int &y)
{
    srand(time(0));
    x=rand()%10;
    y=rand()%10;
}

There is not much purpose in doing that, plus, it will probably make your random number distribution a lot worse. Seed it once when the program starts and that's it.