5
\$\begingroup\$

Is there anything you would suggest to improve this code? In particular, any code to make the snake move more smoothly and make the cursor disappear.

#include <iostream.h>
#include <conio.h>
#include <stdlib.h>
#include <dos.h>

unsigned long int score, high_score = 0;

const int no_rows = 22, no_columns = 77, head_position = 0;

char choice, token, token_food, key_press, direction_head, overlay;

char board[no_rows][no_columns];

unsigned int i, j, k, snake_length, grow, crash;

struct position
{
    int row;
    int column;
} head_map[no_rows * no_columns], food, temp1, temp2;

void Initialize()
{
start:
    clrscr();

    cout << "\t\t\t\tWELCOME TO SNAKE GAME\n\n\n";
    cout << "Choose the Body of the Snake(Enter a symbol):\n";
    cin >> token;
    cout << "\nChoose the Food of the Snake(Enter a symbol):\n";
    cin >> token_food;

    if(token_food == token)
    {
        clrscr();
        cout << "\nDon't use the same symbols for food and body of the snake!!!\n\n\n";
        goto start;
    }

    cout << "\n\nUP\t-\tW\nDOWN\t-\tS\nRIGHT\t-\tD\nLEFT\t-\tA\n\n\nARE YOU
         READY(Y / N) ?? \n";
         cin >> choice;

    for (i = 0; i < no_rows; i++)
    {
        for (j = 0; j < no_columns; j++)
            board[i][j] = ' ';
    }

    head_map[0].row = no_rows / 2;
    head_map[0].column = no_columns / 2;

    board[head_map[0].row][head_map[0].column] = token;

    direction_head = 'R';
    snake_length = 1;

    grow = crash = score = 0;
}

void Food()
{
    do
    {
        food.row = rand() % no_rows;
        food.column = rand() % no_columns;
    }
    while(board[food.row][food.column] == token);

    board[food.row][food.column] = token_food;
}

void Display()
{
    clrscr();

    for(k = 0; k < (no_columns + 2); k++)
        cout << "-";

    cout << endl;

    for(i = 0; i < no_rows; i++)
    {
        cout << "|";
        for(j = 0; j < no_columns; j++)
            cout << board[i][j];
        cout << "|" << endl;
    }

    for(k = 0; k < (no_columns + 2); k++)
        cout << "-";

    cout << "\t\t\t\t\t  SCORE :" << score;
}

void Input()
{
    if(kbhit())
    {
        key_press = getch();

        if((key_press == 'd' || key_press == 'D') && (key_press != 'a' || key_press != 'A') && (direction_head != 'L'))
            direction_head = 'R';

        if((key_press == 's' || key_press == 'S') && (key_press != 'w' || key_press != 'W') && (direction_head != 'U'))
            direction_head = 'D';

        if((key_press == 'a' || key_press == 'A') && (key_press != 'd' || key_press != 'D') && (direction_head != 'R'))
            direction_head = 'L';

        if((key_press == 'w' || key_press == 'W') && (key_press != 's' || key_press != 'S') && (direction_head != 'D'))
            direction_head = 'U';

        if(key_press == 'p' || key_press == 'P')
            delay(5000);
    }

    temp1 = head_map[head_position];
    board[head_map[head_position].row][head_map[head_position].column] = ' ';

    switch(direction_head)
    {
    case 'R':
    {
        if(head_map[head_position].column == no_columns)
            crash = 1;

        overlay = board[head_map[head_position].row][++head_map[head_position].column];

        break;
    }

    case 'L':
    {
        if(head_map[head_position].column == 0)
            crash = 1;

        overlay = board[head_map[head_position].row][--head_map[head_position].column];

        break;
    }

    case 'D':
    {
        if(head_map[head_position].row == no_rows)
            crash = 1;

        overlay = board[++head_map[head_position].row][head_map[head_position].column];

        break;
    }

    case 'U':
    {
        if(head_map[head_position].row == 0)
            crash = 1;

        overlay = board[--head_map[head_position].row][head_map[head_position].column];

        break;
    }

    default:
        break;
    }
}

void Move()
{
    board[head_map[head_position].row][head_map[head_position].column] = token;

    for(i = 1; i < snake_length; i++)
    {
        temp2 = head_map[i];
        head_map[i] = temp1;

        board[head_map[i].row][head_map[i].column] = token;
        board[temp2.row][temp2.column] = ' ';

        temp1 = temp2;
    }

    if(grow == 1)
    {
        head_map[i] = temp1;
        board[head_map[i].row][head_map[i].column] = token;

        ++snake_length;
        score += 10;
        Food();
        grow = 0;
    }
}

void Change()
{
    if(overlay == token)
        crash = 1;

    if(overlay == token_food)
        grow = 1;
}

void Game_over()
{
    getch();
    clrscr();

    cout << "\n\n\n\n\n\n\n\n\n\n\t\t\t\tGAME OVER!!!\n\t\t\t\tFINAL SCORE: " << score;

    if(score > high_score)
    {
        high_score = score;
        cout << "\n\t\t\tYOU BEAT THE HIGH SCORE!!!";
    }

    else
    {
        if(score == high_score)
            cout << "\n\t\t\tYOU TIED THE HIGH SCORE!!!";

        else
            cout << "\n\t\t\tHIGH SCORE: " << high_score;
    }
    getch();
    clrscr();

    cout << "\n\n\n\n\n\n\n\n\n\n\n\t\t\tDO YOU WANT TO CONTINUE(Y/N)??\n\t\t\t\t\t";
    cin >> choice;

    getch();
}

int main()
{
    do
    {
        Initialize();
        Food();
        Display();

        do
        {
            Input();
            Change();

            if(crash == 1)
                break;

            Move();
            delay(200);

            Display();

        }
        while(crash != 1);

        Game_over();

    }
    while(choice == 'Y' || choice == 'y');

    return 0;
}
\$\endgroup\$
3
\$\begingroup\$

Making the code more modern

Your code looks very old. This is because the <iostream.h> header in line 1 is not used anymore since about the year 1998. Same for the <dos.h> and <conio.h> headers.

To get your program to compile with a modern C++ compiler, I had to replace the first paragraph of your code with this:

#include <iostream>

using std::cin;
using std::cout;
using std::endl;

extern "C" {
    void clrscr();
    bool kbhit();
    void delay(int);
    int getch();
}

I only did this change to make your code valid for my compiler. It won't run after these changes. Therefore you should not apply these changes to your code. Just leave your code as it is. Don't be surprised though when you try to run your code in a more modern environment, as this won't work.

Putting text on the screen

I noticed that you use long sequences of \n and \t to place the text on the screen. Since you are using the clrscr function, I suppose that the function gotoxy is also defined. If so, you can replace this code:

    clrscr();

    cout << "\n\n\n\n\n\n\n\n\n\n\t\t\t\tGAME OVER!!!\n\t\t\t\tFINAL SCORE: " << score;

with this code:

    clrscr();

    gotoxy(32, 10);
    cout << "GAME OVER!!!";
    gotoxy(32, 11);
    cout << "FINAL SCORE: " << score;

This code takes more vertical space than before, but there is no need anymore to count the number of \n characters in the string.

Input methods

You are using two fundamentally different input methods:

  1. cin >> choice, which reads a character, but the program only sees this character after Enter has been pressed. This is not suitable for a snake game.

  2. getch(), which reads a single key without requiring the Enter key. This function works closely together with kbhit().

You should not mix these two, at least not in the same phase of the game. There is the dialog phase ("play again?"), which should use cin >> choice, and there is the playing phase, which should use kbhit() and getch() (in this order).

Reducing the scope of variables

Your code declares (among others) these variables:

unsigned int i, j, k;

These variables are used later by pieces of code that are completely unrelated to each other. Therefore it doesn't make sense that these unrelated code pieces use the same variables. One of these pieces is:

    for (i = 0; i < no_rows; i++)
    {
        for (j = 0; j < no_columns; j++)
            board[i][j] = ' ';
    }

After the opening parenthesis of each for loop, you should declare the variable, which is then in scope for the rest of the for loop. The changed code is:

    for (unsigned int i = 0; i < no_rows; i++)
    {
        for (unsigned int j = 0; j < no_columns; j++)
            board[i][j] = ' ';
    }

When you do that in the other for loops as well, there will be a compile error:

        head_map[i] = temp1;
        board[head_map[i].row][head_map[i].column] = token;

This compile error means that your code is somewhat unusual. You used i in a loop, and usually that variable is not needed after the loop. Not so in this case.

When the for loop is finished, i will be the same as snake_length. Therefore you can replace the code with the very similar:

        head_map[snake_length] = temp1;
        board[head_map[snake_length].row][head_map[snake_length].column] = token;

This makes the intention of the code a bit clearer, since for experienced programmers the variable name i means a variable that changes its value often, such as in your loops that fill the board with spaces. That name i would be misleading here, since the code handles the tail of the snake. The expression head_map[snake_length] expresses this more clearly than the expression head_map[i].

Redundant conditions

Further down, you have this code:

        if((key_press == 'w' || key_press == 'W') && (key_press != 's' || key_press != 'S') && (direction_head != 'D'))

This code is redundant. If the pressed key is 'w' or 'W', it cannot be 's' at the same time. Therefore you don't need to check for 's' at all.

Furthermore, the expression key_press != 's' || key_press != 'S' will always be true. There are 3 cases:

  1. key_press != 's': the first condition is true, therefore the whole expression is true.
  2. key_press != 'S': the second condition is true, therefore the whole expression is true.
  3. any other key: both conditions are true, therefore the whole expression is true.

Therefore, the simplified code is:

        if((key_press == 'w' || key_press == 'W') && (true) && (direction_head != 'D'))

This can be further simplified to:

        if((key_press == 'w' || key_press == 'W') && direction_head != 'D')

Pausing the game

        if(key_press == 'p' || key_press == 'P')
            delay(5000);

This looks wrong. When I press the 'p' key, I expect the game to pause until I explicitly continue it by pressing 'p' again. Waiting 5 seconds is something entirely different.

To implement the pause correctly, you should define a global variable:

bool paused = false;

After that, adjust the code from above:

    if(kbhit())
    {
        ...

        if(key_press == 'p' || key_press == 'P')
            paused = !paused;
    }

    if (paused)
        return;

Since the second part of the Input function does not deal with the input at all but instead moves the snake, that part is skipped as long as the game is paused.

Final words

Your code is structured well, especially since the code of the main function gives a rough overview over the whole game flow, just as it should.

You named the functions well, which makes the main function easy to grasp.

There are many more things that can be said about your code, but they are not urgent. Getting a few ideas and thinking about them is easier than getting a hundred tips at once.

\$\endgroup\$
  • \$\begingroup\$ That was really helpful. I've modified the code to include classes and objects. I didn't know if i should reduce the scope of variables as it might decrease efficiency due to repeated definitions. I was intending to use goto(x,y). I didn't know how it worked till now. I know there is redundant code in the Input() function which I realized just after this post. I also apologize for the out of date code but my school uses TurboC++ 3.2 and it's necessary to use it. I am trying to learn newer standards on Visual Studio. That pause function is really great. \$\endgroup\$ – Tarun Nov 14 at 10:58
  • \$\begingroup\$ I wasn't taught exactly what getch() did but was just told to add it at the end of the program to get the output screen. Also, how would I make the game more smooth or make it fps-based? Also how do I remove the flashing cursor? \$\endgroup\$ – Tarun Nov 14 at 11:03
  • 1
    \$\begingroup\$ TurboC++ is a good language since it lives in a small and understandable environment that is easy to grasp for beginners. I just wanted you to be aware that when you leave that world, it will get a little more complicated. \$\endgroup\$ – Roland Illig Nov 14 at 19:39
  • 1
    \$\begingroup\$ Regarding the reduced efficiency: with modern compilers there is no efficiency penalty for using variables with a small scope. Most probably the code will get faster by using the smaller scope. Therefore there's no reason to make the scope of these variables global. \$\endgroup\$ – Roland Illig Nov 14 at 19:41
3
\$\begingroup\$

Code improvements:

  1. Do not use using namespace std it saves you some time typing the code, but when other namespace get involved later on its a mess to debug this code. Furthermore I personally find it more aesthetic to see from which namespace a functions "comes from".
  2. Consider using a function that checks wether the user-input is valid.
    cout << "Choose the Body of the Snake(Enter a symbol):\n";
    cin >> token;
    
    This, for instance is dangerous. What if I (the user) enter a non ASCII symbol like an emoji. 😀 Or even worse - a character that isn't even printable.
\$\endgroup\$
  • \$\begingroup\$ You call a global unsigned int i good work? You have a strange sense of quality. Or you may have been in a hurry. I hope for the latter. \$\endgroup\$ – Roland Illig Nov 12 at 23:26
  • \$\begingroup\$ Why shouldn't I use a global unsigned variable? I did go with a signed int initially but I figured at least no errors could make it go negative. And I made them global to reduce passing to functions. \$\endgroup\$ – Tarun Nov 13 at 2:11
  • 1
    \$\begingroup\$ @RolandIllig He/She is clearly a beginner and I said something about global variables. If you want to correct any other style ‚mistakes‘, go for it \$\endgroup\$ – SchnJulian Nov 13 at 5:56
  • 1
    \$\begingroup\$ @Tarun Roland is right. Although it’s not the worst thing you can do, it‘s not very secure to have a variable is accessible from every part of the program. Consider using a class that contains your game and auxiliary variables. \$\endgroup\$ – SchnJulian Nov 13 at 6:01
  • \$\begingroup\$ So should I include the board/map and all associated variables and functions in a class of its own? And create an array of objects for each square on the map? \$\endgroup\$ – Tarun Nov 13 at 10:56
2
\$\begingroup\$

Your main bottleneck is, of course, using the console for drawing the frame. I suggest you to stop using std::endl in your Display() function, and write a new line character instead.

I believe this only helps you, though, if you turn off automatic flushing usingstd::ios::sync_with_stdio. You have to see yourself if this is an improvement on your machine anyway.

Actually there are other problems I see in this code you should care about; quality, and not performance-wise. But I wouldn't address those now, since you didn't ask for it.

\$\endgroup\$
  • \$\begingroup\$ Actually I wouldn't mind any suggestions to improve quality either. But is this algorithm efficient enough or am I beating the bush around too much? Also should I implement classes in this program? \$\endgroup\$ – Tarun Nov 13 at 2:13

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.