20
\$\begingroup\$

I made a snake game in c, it runs pretty well, everything is working, I think, I'm just wondering if there are any bugs I'm not aware of or if there is anything I can do better.

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

int gameOver = 0, width = 20, height = 20, x, y, fruitX, fruitY, score;
int s;
int totalTail;
int tailX[100], tailY[100];

void Setup(){
    srand(time(NULL));
    x = height/2;
    y = width/2;
    score = 0;

    Start:
    fruitX = 1 + rand()%width;
    fruitY = 1+ rand()%height;

    if(fruitX <= 0||fruitY <= 0||fruitX == x||fruitY == y||fruitX>height||fruitY>width){
        goto Start;
    }
}

void Draw(){
    system("cls");

    for(int i = 0; i<height;i++){
        for(int j = 0; j<width; j++){
            if(i == 0||j == 0||i == width-1|| j == height-1){
                printf("#");
            }
            else if(i == x&&j == y){
                printf("O");
            } else if(fruitX == i && fruitY == j){
                printf("F");
            } else{
                int print = 0;
                for(int k = 0; k <totalTail; k++){
                    if(tailX[k] == i&& tailY[k] == j){
                        printf("o");
                        print = 1;
                    }
                }
                if(print == 0){
                    printf(" ");
                }
            }
        }
        printf("\n");
    }
    printf("Score: %d", score);

}
void Input(){

    if(kbhit()){
        switch(getch()){
        case 'w':
            s = 1;
            break;
        case 's':
            s = 2;
            break;
        case 'a':
            s = 3;
            break;
        case 'd':
            s = 4;
            break;
    }
    }
}

void Logic(){
    int prevX = tailX[0];
    int prevY = tailY[0];
    int prev2X, prev2Y;
    tailX[0] = x;
    tailY[0] = y;
    for (int i = 1; i < totalTail; i++)
    {
        prev2X = tailX[i];
        prev2Y = tailY[i];
        tailX[i] = prevX;
        tailY[i] = prevY;
        prevX = prev2X;
        prevY = prev2Y;
    }

    switch(s){
        case 1:
            x--;
            break;
        case 2:
            x++;
            break;
        case 3:
            y--;
            break;
        case 4:
            y++;
            break;
        default:
            break;
    }
    if(s == 1||s==2){Sleep(25);}

    for(int i = 0; i <totalTail; i++){
        if(tailX[i] == x&&tailY[i] == y){
            gameOver = 1;
        }
    }

    if(x > width-1||y>height-1||x<0||y<0){
        gameOver = 1;
    }

    if(x == fruitX && y == fruitY){
        score += 1;
        totalTail++;
        start2:
        fruitX = rand()%height;
        fruitY = rand()%width;

        if(fruitX <= 0||fruitY <= 0||fruitX == x||fruitY == y||fruitX>height||fruitY>width){
            goto start2;
        }
    }
}

int main()
{
    Setup();

    while(gameOver != 1){
        Draw();
        Logic();
        Input();
        Sleep(10);
    }
    return 0;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ dareesome, what is the reason for the fruitX == x||fruitY == y test? \$\endgroup\$ – chux - Reinstate Monica Apr 19 at 20:46
  • \$\begingroup\$ Where it says if(x == fruitX && y == fruitY){ or if(fruitX <= 0||fruitY <= 0||fruitX == x||fruitY == y||fruitX>height||fruitY>width){ \$\endgroup\$ – dareesome Apr 19 at 20:55
  • 2
    \$\begingroup\$ This isn't a big enough comment for a full answer, but I would think that Snake is a perfect use-case for a Linked List rather than an array. Your implementation requires you to iterate over the whole snake every time it moves, while a LL would be just a matter of adding one to the head and removing one from the tail. This would also be more memory efficient as the snake would only occupy as much memory as it needs, and could work efficiently at any length, rather than having an arbitrary cap of 100. \$\endgroup\$ – Darrel Hoffman Apr 20 at 16:09
  • 1
    \$\begingroup\$ @DarrelHoffman I agree with the linked list idea, but I'd add that the best implementation thereof would not be to allocate each link node's memory on the heap; but instead to pre-allocate a 2d array representing the playing field, and have each link node pointer point to the array element representing the node position. That way you never have to malloc()/free() anything, plus instead of using 8 byte pointers you can further save memory by using uint16_t (or uint32_t) indeces. Furthermore, if you turn each array element into a (bit-packed) struct, the same array can double as a fruit tracker. \$\endgroup\$ – Will Apr 22 at 4:06
23
\$\begingroup\$

You should keep your game loop as simple as possible with only an update and draw method and specifically in that order.


To ensure your game loop is iterating at the same speed everytime, you should calculate the duration of your update and draw methods and then subtract that from how long each iteration should last.

i.e.:

MD = 10ms // max duration for an iteration
D = 2ms // actual duration of a draw and update
MD - D = 10ms - 2ms = 8ms // calculated duration of sleep

You should not be redrawing the whole snake game every iteration. That is extrememly inefficient. You should instead move the console cursor to the coordinates that require something to be changed.

drawing (in any context) is extremely expensive, so only draw what you need

Also, a nice trick in console games is that you don't need to redraw the whole snake to show that it moved.

i.e.: steps in one iteration on how to make the snake move.

Step 0: Snake in default position

###@

Step 1: Snake with head advanced by 1

###@@

Step 2: Draw a space at the snake's last body position note: this represents deletion

 ##@@

Step 3: Snake with body part placed right behind the head note: this deletes the old @ symbol

 ###@

So no matter how long your snake is, it will only require 3 actions to move the snake.


The fruit should only appear in areas where the snake does not exist. The amount of possible coordinates is finite in your game. So I would just have a list of values to store all the unused coordinates and randomely take a value from that list to determine the location of the next food. (less guessing required).

start2:
fruitX = rand()%height;
fruitY = rand()%width;

if(fruitX <= 0||fruitY <= 0||fruitX == x||fruitY == y||fruitX>height||fruitY>width){
            goto start2;
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Second opinion. Painting only part of the screen can cause flicker. In many contexts, the most efficient way to update the screen is to draw to a back buffer and swap buffers. \$\endgroup\$ – Davislor Apr 20 at 1:30
  • \$\begingroup\$ I don't think sleep is the best method, since sleep could take a variable amount of time. Better is a busy loop or interrupt based timing. \$\endgroup\$ – qwr Apr 20 at 8:06
  • \$\begingroup\$ @qwr I actually quite like sleep for simple console games that don't require many updates. What do you mean by "sleep could take a variable amount of time" ? \$\endgroup\$ – kemicofa ghost Apr 20 at 8:13
  • 1
    \$\begingroup\$ Sleep only guarantees to suspend the process for at least the specified time. The execution of other processes can delay resumption. See e.g. docs.microsoft.com/en-us/windows/win32/api/synchapi/… \$\endgroup\$ – maxbachmann Apr 20 at 8:27
  • \$\begingroup\$ @Davislor The flicker would be caused by calling cls, not as an inherent problem of partial screen updates on a text console. I've never seen any appreciable flicker in a game, from a telnet/bbs ANSI game to local term games (e.g. rogue). Assuming you're trying to keep a sane framerate, the screen updates should be just fine as suggested. \$\endgroup\$ – phyrfox Apr 20 at 11:34
9
\$\begingroup\$

Enums

In the code s is used to indicate the movement direction. From the code it's not easy to tell what s or s == 2 means. An enum would improve readability and maintainability of the code.

typedef enum {
    Up,
    Down,
    Left,
    Right
} Direction;

Which would lead to something like this:

Direction direction;

if (kbhit()) {
    switch (getch()) {
        case 'w':
            direction = Up;
            break;
        case 's':
            direction = Down;
            break;
        case 'a':
            direction = Left;
            break;
        case 'd':
            direction = Right;
            break;
    }
}

Styling

The code contains some inconsistent styling in terms of spacing around operators, brackets and parentheses. It would be best to make the styling consistent to increase readability. Using a style guide, formatter and/or linter could help with this.

Avoid the use of goto

Opinions may differ on this one, but the goto statement can increase the likelihood of spaghetti code. It has some proper use cases, but most of the time there are better ways to structure code to avoid it's usage. More information on this can be found here: https://stackoverflow.com/questions/46586/goto-still-considered-harmful.

start:
    fruitX = rand() % height;
    fruitY = rand() % width;

    if (fruitX <= 0 || fruitY <= 0 || fruitX == x|| fruitY == y || fruitX > height || fruitY > width) {
        goto start;
    }

Could be written as:

    do {
        fruitX = rand() % height;
        fruitY = rand() % width;
    } while (fruitX <= 0 || fruitY <= 0 || fruitX == x|| fruitY == y || fruitX > height || fruitY > width)
\$\endgroup\$
4
\$\begingroup\$

A couple of things.

It’s a good idea to future-proof your program by declaring which version of the Windows SDK you’re using. This way, a future version of the compiler knows what version to stay compatible with and not to deprecate any functions you use. You can do this with the macros in <SdkDdkVer.h>, typically:

#define _WIN32_WINNT  _WIN32_WINNT_WIN10
#define NTDDI_VERSION NTDDI_WIN10_19H1
#include <SdkDdkVer.h>

In your Setup function, you use a goto, which is a shoot-yourself-in-the-foot feature of the language. You aren’t trying to break out of multiple nested loops or send all possible code branches to a single exit point. What you want here is to retry the randomization until it gives you good values. This is a do ... while loop. That is, without refactoring anything else, you would want to rewrite that control structure as something like this:

#if 0
Start:
fruitX = 1 + rand()%width;
fruitY = 1+ rand()%height;

if(fruitX <= 0||fruitY <= 0||fruitX == x||fruitY == y||fruitX>height||fruitY>width){
    goto Start;
}
#endif

do {
  fruitX = 1 + rand()%width;
  fruitY = 1 + rand()%height;
} while ( fruitX <= 0 ||
          fruitY <= 0 ||
          fruitX == x ||
          fruitY == y ||
          fruitX>height ||
          fruitY>width
        );

Note that it is impossible for fruitX and fruitY to exceed width and height, respectively, as you have defined them with % width and % height, but defensive coding is a good idea. (If the invalid values should be impossible, you might even want to abort with a descriptive error message, rather than silently handle them.) You might also define them as unsigned values.

There appears to be a significant logic error here as well: you generate a new position for the fruit whenever it is on either the same row or the same column as the head of the snake, and you probably meant to regenerate it only if it is in the exact same location.

This would get you something like

do {
  fruitX = 1 + rand()%width;
  fruitY = 1 + rand()%height;
} while ( fruitX == x && fruitY == y );

It would also be possible to subtract 1 from the range of the x and y coordinates of the fruit, and add 1 to either if it is greater than or equal to the coordinate of the head. This gets you a uniform pseudo-random distribution, on the first try, guaranteed not to be on the same row or column as the head.

This code is probably identical to the code that will place new fruits on the screen, so you can factor it out into a separate function that returns a coordinate-pair, instead of repeating yourself.

If you’re using a Boolean value, you might as well use bool from <stdbool.h>.

The recommended way to clear the screen in Windows is not with system("cls"), but by setting the console to accept ANSI virtual terminal control codes, as demonstrated here. The virtual console (which emulates a DEC VT102 from the 1970s) is documented in more detail here. The terminal also supports painting only the part of the screen that has changed, or drawing to a back buffer and swapping buffers, to avoid flicker. Either is much closer to how you would render modern graphics in a game or app than clearing the screen and then printing every character in order, although the API is very different.

If you use this interface, you would also want to read console input events, as documented here. Unfortunately, if you wanted to have your game advance by clock ticks, and not update when you press a key, you would need to fall back on deprecated APIs (which, however, are still going to be supported for the indefinite future).

Here’s a very simple test program that refactors the global game state into a struct and passes a reference to it around. (I’m trying to compromise here between not writing a new program in a code review, and providing a minimal working example of the concepts, not just links to documentation.) I left out the input handling, since I could see several different approaches you could take, but you probably would end up using ReadConsoleInput rather than getch.

#define _WIN32_WINNT  _WIN32_WINNT_WIN10
#define NTDDI_VERSION NTDDI_WIN10_19H1
#include <SdkDdkVer.h>

#include <assert.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#include <windows.h>

#define HEIGHT 20U
#define WIDTH 20U

typedef struct GameState {
  unsigned fruitX; // The x coordinate of the fruit, with the origin in the top left corner.
  unsigned fruitY; // And its y coordinate.
  long score;
  bool isGameOver;
  ptrdiff_t snakeLen; // The number of snake segments, including the head.
  unsigned snakeXs[HEIGHT*WIDTH]; // The x-coordinates of each snake segment, from tail to head
                                  // (The x-coordinate of the head is snakeXs[snakeLen-1]).
  unsigned snakeYs[HEIGHT*WIDTH]; // The y-coordinate
} GameState;

int clear_screen(const HANDLE hStdOut) {
    // Write the sequence for clearing the display.
    DWORD written = 0;
    static const wchar_t sequence[] = L"\x1b[2J\x1b[3J";

    if ( !WriteConsoleW( hStdOut, sequence, ARRAYSIZE(sequence), &written, NULL )) {
        return GetLastError();
    } else {
     return 0;
    }
}

void game_init(GameState* const state)
{
    static const unsigned x = WIDTH/2U;
    static const unsigned y = HEIGHT/2U;

    do {
      state->fruitX = rand() % WIDTH;
      state->fruitY = rand() % HEIGHT;
    } while ( state->fruitX == x && state->fruitY == y );

    memset( state, sizeof(*state), 0 );
    state->score = 0;
    state->isGameOver = false;
    state->snakeXs[0] = x;
    state->snakeYs[0] = y;
    state->snakeLen = 1;
}

void draw_screen( const GameState* const state,
                  const HANDLE hStdOut )
{
    char playfield[HEIGHT][WIDTH+1];
    char scoreline[32] = {0};

    clear_screen(hStdOut);

    for ( unsigned i = 0; i < HEIGHT; ++i ) {
      for ( unsigned j = 0; j < WIDTH; ++j ) {
        playfield[i][j] = '.';
      }
      playfield[i][WIDTH] = '\n';
    }

    const ptrdiff_t headPos = state->snakeLen - 1;
    assert( headPos >= 0 );

    for ( ptrdiff_t i = 0; i < headPos; ++i ) {
      assert(state->snakeXs[i] < WIDTH);
      assert(state->snakeYs[i] < HEIGHT);
      playfield[state->snakeYs[i]][state->snakeXs[i]] = '#';
    }

    playfield[state->snakeYs[headPos]][state->snakeXs[headPos]] = '@';
    playfield[state->fruitY][state->fruitX] = '*';

    WriteConsole( hStdOut, playfield, sizeof(playfield), NULL, NULL );
    const DWORD scorelen = snprintf( scoreline,
                                     sizeof(scoreline),
                                     "Score: %ld\n",
                                     state->score );
    WriteConsole( hStdOut, scoreline, scorelen, NULL, NULL );
}

DWORD get_console_mode( const HANDLE h )
{
  DWORD mode;
  if (!GetConsoleMode( h, &mode )) {
    exit(GetLastError());
  }

  return mode;
}

int main(void)
{
    srand(time(NULL));
    const HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);

    const DWORD originalMode = get_console_mode(hStdOut);

    const DWORD newMode = originalMode | 
                          ENABLE_VIRTUAL_TERMINAL_PROCESSING;

    if (!SetConsoleMode( hStdOut, newMode ))
    {
        return GetLastError();
    }

    GameState game_state;
    game_init(&game_state);

    draw_screen( &game_state, hStdOut );

    // Restore the mode on the way out to be nice to other command-line applications.
    SetConsoleMode( hStdOut, originalMode );

    return EXIT_SUCCESS;
}

I admit that I inconsistently switched between WriteConsoleW and WriteConsole there. This is primarily because I buffer the entire playfield and write it out in one system call. Most real-world apps should use Unicode consistently for character I/O, but do not necessarily have to waste memory storing wide-character strings.

It’s generally considered a bad idea to have global variables that aren’t const. They’re bug-attractors, because it’s impossible to reason about which part of the program might have modified them and how. You’ll especially appreciate being able to use the receive-copy-update pattern on state objects if you ever want to make your game multi-threaded—for example, if you turn it into a TRON game, the AI thread could be examining the current game state safely, and another thread could be rendering the next frame and selecting appropriate sound effects, while the physics engine is preparing the new game state.

I would probably turn the case blocks into a finite state machine, which either becomes a loop similar to while(!state->isGameOver) or a tail-recursive call similar to return update_state( &game_state, player_move );. These optimize to equally-efficient code on a modern compiler.

Hope that helps!

\$\endgroup\$
1
\$\begingroup\$
  1. You aren't ever interested in where the nth part of your snake is, but whether one (or sometimes any other obstacle) is at a specific place.
    Lay out your data-structures accordingly!

    Have a single array representing the whole playing field, with codes reserved for space, edibles, walls, snake-head, and 4 types of snake-body (depending on where the body went). If you later decide on adding multi-player, add the same for new snakes.

    Add a function to translate from linear coordinate to x/y and back.

    That does imply that a snake only stores the the head-coordinate, the length (current and wanted), and the ultimate tail-coordinate. The rest is inferred from the playing-field as and when needed.

  2. You know that srand() and rand() are a bad PRNG? Oh well, it should be enough to get you going at least.

  3. Others already commented on structured do-while loops being superior to hand-rolling your own with goto. I just wanted to add that assert() is there for testing whether your program's invariants hold. Always retesting them in the general code clutters it up, makes readers double-check and detracts from the point.

  4. You are using gameOver as a boolean variable. Except that you disregard the conventions baked into the very language.
    In C, zero is false, all else is truthy. (Let's disregard NaN for now.)

    Anyway, running with inverted logic would have been my preference. Which can be upgraded to lifes without trouble.

  5. Doing the update-step after drawing and before processing input is most peculiar. There is no reason to adding gratuitious lag. Anyway, calling it Logic() instead of Update() is surprising.

\$\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.