8
\$\begingroup\$

We started programming in school this year. I wasn't expecting to learn much but the pace is really slow so I tried to learn some things myself. I decided to do a little project for a start and that is a simple snake game. I would like to know what I am doing bad ( I know about the lack of consistency in naming ) or if I actually did something good. Some best practices I am not following and should etc.

#include <stdlib.h>
#include <stdio.h>
#include <windows.h>
#include <conio.h>

#define myY 12
#define myX 24

char map[myY][myX], key;
int alive = 1, x, y, gameSpeed = 200, score = 0, dirX, dirY, oldBodyCor[myY*myX][2], body[myY*myX][2], numOfBody = 0;

void genMap();
void draw();
void move();
void genPlayer();
void genEnemy();
void hideCursor();
void kInp();
void genDefDir();
void goToXY(int column, int line);
void grow();
void freeMove();
void reset();

int main() {
    hideCursor();
    do {
        system("cls");
        genMap();
        genPlayer();
        genDefDir();
        genEnemy();
        while (alive) {
            Sleep(gameSpeed);
            kInp();
            move();
            draw();
        }
        system("cls");
        while (1) {
            goToXY(0, 0);
            printf("YOU DIED");
            printf("\nFINAL SCORE : %d", score);
            printf("\nDo you want to try again?[y/n]");
            key = _getch();
            if (key == 'y') {
                alive = 1;
                reset();
                break;
            } else if (key == 'n') {
                EXIT_SUCCESS;
                break;
            }
        }
    } while (alive);
}

void genMap() {
    int x, y;
    for (y = 0; y < myY; ++y) {
        for (x = 0; x < myX; ++x) {
            if (y == 0 || y == myY - 1) {
                map[y][x] = '#';
            } else if (x == 0 || x == myX - 1) {
                map[y][x] = '#';
            } else {
                map[y][x] = ' ';
            }
        }
    }
}
void grow() {
    if (numOfBody == 0) {
        ++numOfBody;
        map[body[0][0] - dirY][body[0][1] - dirX] = '*';
        body[1][0] = body[0][0] - dirY;
        body[1][1] = body[0][1] - dirX;
        return;
    }
    for (int i = 0; i <= numOfBody; ++i) {
        if (i == numOfBody) {
            body[i + 1][0] = oldBodyCor[i][0];
            body[i + 1][1] = oldBodyCor[i][1];
            map[body[i + 1][0]][body[i + 1][1]] = '*';
            ++numOfBody;
            return;
        }
    }
    return;
}
void genDefDir() {
    srand(time(NULL));
    for (y = 0; y < myY; ++y) {
        for (x = 0; x < myX; ++x) {
            switch (map[y][x]) {
            case '@':
                if (y <= (myY / 2) - 1 && x <= (myX / 2) - 1) {
                    if ((rand() % 2) == 0) {
                        dirX = 1;
                        dirY = 0;
                        return;
                    } else {
                        dirX = 0;
                        dirY = 1;
                        return;
                    }
                } else if (y <= (myY / 2) - 1 && x > (myX / 2) - 1) {
                    if ((rand() % 2) == 0) {
                        dirX = -1;
                        dirY = 0;
                        return;
                    } else {
                        dirX = 0;
                        dirY = 1;
                        return;
                    }
                } else if (y > (myY / 2) - 1 && x <= (myX / 2) - 1) {
                    if ((rand() % 2) == 0) {
                        dirX = 1;
                        dirY = 0;
                        return;
                    } else {
                        dirX = 0;
                        dirY = -1;
                        return;
                    }
                } else if (y > (myY / 2) - 1 && x > (myX / 2) - 1) {
                    if ((rand() % 2) == 0) {
                        dirX = -1;
                        dirY = 0;
                        return;
                    } else {
                        dirX = 0;
                        dirY = -1;
                        return;
                    }
                }
            }
        }
    }
}
void kInp() {
    if (_kbhit()) {
        key = _getch();
    }
    if (key == 'w') {
        dirX = 0;
        dirY = -1;
    } else if (key == 's') {
        dirX = 0;
        dirY = 1;
    } else if (key == 'a') {
        dirX = -1;
        dirY = 0;
    } else if (key == 'd') {
        dirX = 1;
        dirY = 0;
    }
}
void genPlayer() {
    srand(time(NULL));
    int x = rand() % (myX - 3) + 1;
    int y = rand() % (myY - 3) + 1;
    map[y][x] = '@';
    body[0][0] = y;
    body[0][1] = x;
}
void genEnemy() {
    srand(time(NULL));
    while (1) {
        int x = rand() % (myX - 3) + 1;
        int y = rand() % (myY - 3) + 1;
        if (map[y][x] != '@' && map[y][x] != '*') {
            map[y][x] = '&';
            return;
        }
    }

}
void draw() {
    for (y = 0; y < myY; ++y) {
        goToXY(myX / 2, y);
        for (x = 0; x < myX; ++x) {
            printf("%c", map[y][x]);
        }
        printf("\n");
    }
    goToXY(0, 0);
    printf("Score : %d", score);
}
void move() {
    for (y = 0; y < myY; ++y) {
        for (x = 0; x < myX; ++x) {
            switch (map[y][x]) {
            case '@':
                if (map[y + dirY][x + dirX] == ' ') {
                    freeMove();
                    return;
                } else if (map[y + dirY][x + dirX] == '&') {
                    map[y + dirY][x + dirX] = ' ';
                    score += 10;
                    freeMove();
                    grow();
                    genEnemy();
                    return;
                } else if (map[y + dirY][x + dirX] == '#' || map[y + dirY][x + dirX] == '*') {
                    alive = 0;
                }
            }
        }
    }
}
void freeMove() {
    char headY = y;
    char headX = x;
    char temp = map[y][x];
    map[y][x] = map[y + dirY][x + dirX];
    map[y + dirY][x + dirX] = temp;
    body[0][0] = y + dirY;
    body[0][1] = x + dirX;
    for (int i = 1; i <= numOfBody; ++i) {
        if (i == 1) {
            oldBodyCor[i][0] = body[i][0];
            oldBodyCor[i][1] = body[i][1];
            char temp = map[body[i][0]][body[i][1]];
            map[body[i][0]][body[i][1]] = map[headY][headX];
            map[headY][headX] = temp;
            body[i][0] = headY;
            body[i][1] = headX;
            continue;
        }
        oldBodyCor[i][0] = body[i][0];
        oldBodyCor[i][1] = body[i][1];
        char temp = map[body[i][0]][body[i][1]];
        map[body[i][0]][body[i][1]] = map[oldBodyCor[i - 1][0]][oldBodyCor[i - 1][1]];
        map[oldBodyCor[i - 1][0]][oldBodyCor[i - 1][1]] = temp;
        body[i][0] = oldBodyCor[i - 1][0];
        body[i][1] = oldBodyCor[i - 1][1];
    }
    return;
}
void reset() {
    score = 0;
    for (int i = 0; i <= numOfBody; ++i) {
        body[i][0] = 0;
        body[i][0] = 0;
    }
    numOfBody = 0;
}
void hideCursor() {
    HANDLE consoleHandle = GetStdHandle(STD_OUTPUT_HANDLE);
    CONSOLE_CURSOR_INFO info;
    info.dwSize = 100;
    info.bVisible = FALSE;
    SetConsoleCursorInfo(consoleHandle, &info);
}
void goToXY(int column, int line) {
    // Create a COORD structure and fill in its members.
    // This specifies the new position of the cursor that we will set.
    COORD coord;
    coord.X = column;
    coord.Y = line;

    // Obtain a handle to the console screen buffer.
    // (You're just using the standard console, so you can use STD_OUTPUT_HANDLE
    // in conjunction with the GetStdHandle() to retrieve the handle.)
    // Note that because it is a standard handle, we don't need to close it.
    HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE);

    // Finally, call the SetConsoleCursorPosition function.
    if (!SetConsoleCursorPosition(hConsole, coord)) {
        // Uh-oh! The function call failed, so you need to handle the error.
        // You can call GetLastError() to get a more specific error code.
        // ...
    }
}
\$\endgroup\$
6
\$\begingroup\$

As a complement to JS1's already good review, here are some things that may help you improve your code.

Fix the bug

The code currently includes this:

} else if (key == 'n') {
    EXIT_SUCCESS;
    break;
}

However, that middle line is not doing what you think it is. Instead, you need to write something like this:

    return EXIT_SUCCESS;

Otherwise just EXIT_SUCCESS has no effect.

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

The key matching logic is much easier to see if a switch statement is used instead of the long if...else chain. The default case can then be used for the keys that are not valid (if you choose to do something with that).

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()
{
    printf("\x1b[2J");
}
\$\endgroup\$
  • \$\begingroup\$ I wrote "exit" hoping visual studio would help me out. And EXIT_SUCCESS sounded right to me so I tried it. I read about the switch instead of long chains and I will be more careful next time. And for the system("cls"), I just googled how to clear the console window and most top answers were "Just use system("cls") if on Windows". Thank you for your help. \$\endgroup\$ – user126946 Dec 30 '16 at 14:01
  • 1
    \$\begingroup\$ @chux Good point; I've fixed my answer. \$\endgroup\$ – Edward Dec 30 '16 at 21:01
5
\$\begingroup\$

Unnecessary loops

There are a few places where you have loops for no reason:

  1. In genDefDir() you search the whole game board for the head of the snake, when the coordinates for that are just body[0][0] and body[0][1]. You also do the same thing in move().

  2. In grow(), you have a for loop but you only do anything when i == numOfBody. So just eliminate the loop and set i = numOfBody.

Shadowed variables

You have global variables named x and y, but you sometimes use local variables of the same name. Actually, you never even need the global variables if you just added x,y arguments to the functions that required coordinates to be passed in.

Calling srand() multiple times

You should only call srand() once. By calling it every time you generate a random number, you run the risk of generating the same number if you generated two random numbers in the same second.

Random numbers have wrong range

Your enemies can never appear in the bottom row or rightmost column because you are off by one here:

int x = rand() % (myX - 3) + 1;

It should be myX - 2 instead of myX - 3 (and same for myY).

freeMove() simplification

Your freeMove() and grow() functions are overly complicated. You could combine them into one simpler function like this:

void freeMove(int grow)
{
    // Extend tail or move it (by erasing the old tail).
    if (grow)
        numOfBody++;
    else
        map[body[numOfBody][0]][body[numOfBody][1]] = ' ';

    // Move body segments up one.
    for (int i = numOfBody; i >= 1; --i) {
        body[i][0] = body[i-1][0];
        body[i][1] = body[i-1][1];
    }

    // Move head and draw it.
    body[0][0] += dirY;
    body[0][1] += dirX;
    map[body[0][0]][body[0][1]] = '@';
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the review. I got stuck multiple times while writing it and instead of staring all over again I tried "repairing" the code with a whole new "idea". For some reason I didn't store the head position at first and that is why I used the loops. They worked so I let them be. Same goes for the weird variables. As for the random numbers, I have no idea why I wrote 3. The functions really were messy. \$\endgroup\$ – user126946 Dec 30 '16 at 13:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy