5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ The second question is off-topic as we cannot help with adding new implementation. \$\endgroup\$ – Jamal Jan 17 '15 at 18:47
  • \$\begingroup\$ +1 ...please make it OOP style. i was think what to do next in text-based game. i will post mine but it is in window platform. \$\endgroup\$ – MORTAL Jan 18 '15 at 2:24
4
\$\begingroup\$

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';
}
|improve this answer|||||
\$\endgroup\$
4
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$

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.