1
\$\begingroup\$

Made a simple snake game using C and raylib for graphics, Would like to be critiqued on the clearity and efficiency of the code.

#include <raylib.h>

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <stdbool.h>

#define DIRECTION_UP (Vec2i){0, -1}
#define DIRECTION_DOWN (Vec2i){0, 1}
#define DIRECTION_LEFT (Vec2i){1, 0}
#define DIRECTION_RIGHT (Vec2i){-1, 0}

enum GameState
{ 
    GAME_START,
    GAME_RUNNING,
    GAME_OVER 
};

enum CellState
{
    CELL_EMPTY,
    CELL_SNAKE,
    CELL_APPLE
};

typedef unsigned uint;

typedef struct
{
    int x, y;
} Vec2i;

typedef struct
{
    uint *cells;
    uint height, width;
} Board;

typedef struct
{
    bool collided;
    uint tailLength;
    Vec2i headPosition;
    Vec2i headDirection;
    Vec2i *tail;
} Snake;

static uint gameState = GAME_START;
static uint gameScore = 0;

static Board board;
static Snake snake;
static Vec2i applePosition;

void board_set(Vec2i v, uint c)
{
#ifdef DEBUG
    if (v.x > board.width || v.y > board.height)
    {
        fprintf(stderr, "Can not get board at (%d,%d).\n", v.x, v.y);
        exit(1);
    }
#endif

    *(board.cells + v.x * board.width + v.y) = c;
}

uint board_get(Vec2i v)
{
#ifdef DEBUG
    if (v.x > board.width || v.y > board.height)
    {
        fprintf(stderr, "Can not set board at (%d,%d).\n", v.x, v.y);
        exit(1);
    }
#endif
    return *(board.cells + v.x * board.width + v.y);
}

void apple_spawn()
{
    while (true)
    {
        //Spawn within bounds of the board
        int x = (rand() % (board.width - 2)) + 1;
        int y = (rand() % (board.height - 2)) + 1;
        //Do not spawn on snake cells
        if (board_get((Vec2i){x, y}) != CELL_SNAKE)
        {
            applePosition.x = x;
            applePosition.y = y;

            break;
        }
    }
}

void snake_tail_grow()
{
    snake.tailLength++;
    if (snake.tailLength == 1)
    {
        snake.tail[snake.tailLength].x = snake.headPosition.x ;
        snake.tail[snake.tailLength].y = snake.headPosition.y ;
        return;
    }

    //Previous tail 
    snake.tail[snake.tailLength] = snake.tail[snake.tailLength - 1];
}

bool apple_eaten()
{
    //Check if snake head is on the same cell as the apple
    if (snake.headPosition.x == applePosition.x && snake.headPosition.y == applePosition.y)
        return true;

    return false;
}
void game_create(uint w, uint h)
{
    board.cells = calloc(h * w, sizeof *(board.cells));
    snake.tail = calloc(h * w, sizeof (Vec2i));//Allocate for longest tail length possible on the board
    if (board.cells == NULL || snake.tail == NULL)
    {
        fprintf(stderr, "Failed to allocate memory.\n");
        exit(1);
    }

    board.width = w;
    board.height = h;

    snake.tailLength = 0;

    snake.headDirection.x = 0;
    snake.headDirection.y = 0;

    //Spawn snake at the center of the board
    uint centerX = board.width / 2;
    uint centerY = board.height / 2;
    snake.headPosition.x = centerX;
    snake.headPosition.y = centerY;
}

void game_destroy()
{
    free(snake.tail);
    free(board.cells);
}

void game_update()
{
    //Clear board
    memset(board.cells, CELL_EMPTY, board.height * board.width * sizeof *(board.cells)); 

    //Update cells to be on the new positions
    board_set(snake.headPosition, CELL_SNAKE);
    board_set(applePosition, CELL_APPLE);
    for (uint i = 0; i < snake.tailLength; i++)
        board_set(snake.tail[i], CELL_SNAKE);

    //Snake collision with board boundary
    if (snake.headPosition.x >= board.width || snake.headPosition.x <= 0)
        snake.collided = true;
    if (snake.headPosition.y >= board.height || snake.headPosition.y <= 0)
        snake.collided = true;

    //Shift to last to make space for new tail
    memmove(snake.tail + 1, snake.tail, snake.tailLength * sizeof *(snake.tail));
    snake.tail[0] = snake.headPosition;

    //Move snake in the direction
    snake.headPosition.x += snake.headDirection.x;
    snake.headPosition.y += snake.headDirection.y;

    if (apple_eaten())
    {
        gameScore++;
        snake_tail_grow();
        apple_spawn();
    }

    if (snake.collided)
        gameState = GAME_OVER;
}

void game_input()
{
    if (IsKeyDown(KEY_W))
        snake.headDirection = DIRECTION_UP;
    if (IsKeyDown(KEY_S))
        snake.headDirection = DIRECTION_DOWN;
    if (IsKeyDown(KEY_D))
        snake.headDirection = DIRECTION_LEFT;
    if (IsKeyDown(KEY_A))
        snake.headDirection = DIRECTION_RIGHT;
}

void game_draw()
{
    uint widthRatio = GetScreenWidth() / board.width;
    uint heightRatio = GetScreenHeight() / board.height;

    for (uint x = 0; x < board.width; x++)
        for (uint y = 0; y < board.height; y++)
        {
            switch (board_get((Vec2i){x, y}))
            {
                case CELL_SNAKE :
                    DrawRectangle(x * widthRatio, y * heightRatio, widthRatio, heightRatio, GREEN);
                    break;
                case CELL_APPLE :
                    DrawRectangle(x * widthRatio, y * heightRatio, widthRatio, heightRatio, RED);
                    break;
            }
        }
    //Draw score
    DrawText(TextFormat("Score : %u", gameScore), 0, 0, 20, WHITE);
}

int main()
{
    InitWindow(640, 640, "Snake");
    SetTargetFPS(10);

    game_create(20, 20);
    apple_spawn();

    while (!WindowShouldClose())
    {
        BeginDrawing();
        ClearBackground(BLACK);
        switch (gameState)
        {
            case GAME_START :

                DrawText("Press space to start.", 0, 0, 40, WHITE);
                DrawText("W,A,S,D keys for movement.", 0, 40, 25, WHITE);
                if (IsKeyPressed(KEY_SPACE))
                    gameState = GAME_RUNNING;

                break;

            case GAME_RUNNING :

                game_input();
                game_update();
                game_draw();

                break;

            case GAME_OVER :
                DrawText("Game Over :(", 0, 0, 40, RED);
                DrawText(TextFormat("Score : %u", gameScore), 0, 40, 40, WHITE);
                DrawText("Press escape to exit.", 0, 80, 40, WHITE);
        }

        EndDrawing();
    }

    game_destroy();
    CloseWindow();
    return 0;
}

How did I do?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Since there's only one translation unit here, mark your functions static similar to what you've already done with your global variables.

I don't see a lot of value in typedef unsigned uint; - I'd delete that.

You follow what I consider to be a reasonable pattern for struct declaration - no tags, all typedefs. You do not do the same with your enum but for consistency's sake you should.

Your code is non-reentrant due to the reliance on global state. This isn't the end of the world, but a proper refactor would involve moving all of those variables to a game state structure and operating only on instances of that structure instead of globals. You ask:

should the game state structure be static as well?, also would you mind elaborating the global state part a bit more?

No, your game state data should not be made static: once it's removed from the global namespace, it will be passed around in parameters and return values. For an example of what this could look like:

typedef struct {
    unsigned score;
    // ...
} GameState;



static void game_update(GameState *game) {
    // ...
    if (apple_eaten())
    {
        game->score++;
    }

You need more const arguments, particularly for struct arguments.

Why should your if (v.x > board.width || v.y > board.height) check be surrounded in an #ifdef DEBUG? This is surely not performance-impactful so should be left in release builds.

*(board.cells + v.x * board.width + v.y) is slightly awkward. You can instead use an index-indirection like

board.cells[v.x * board.width + v.y] = c;

This while(true):

while (true)
{
    //Spawn within bounds of the board
    int x = (rand() % (board.width - 2)) + 1;
    int y = (rand() % (board.height - 2)) + 1;
    //Do not spawn on snake cells
    if (board_get((Vec2i){x, y}) != CELL_SNAKE)
    {
        applePosition.x = x;
        applePosition.y = y;

        break;
    }
}

can more legibly capture the termination condition as

int x, y;

do {
    // Spawn within bounds of the board
    x = (rand() % (board.width - 2)) + 1;
    y = (rand() % (board.height - 2)) + 1;
    // Do not spawn on snake cells
} while (board_get((Vec2i){x, y}) == CELL_SNAKE);

applePosition.x = x;
applePosition.y = y;

I consider the early-return in snake_tail_grow to be more legibly replaced by an else:

static void snake_tail_grow()
{
    snake.tailLength++;
    if (snake.tailLength == 1)
    {
        snake.tail[snake.tailLength].x = snake.headPosition.x;
        snake.tail[snake.tailLength].y = snake.headPosition.y;
    }
    else
    {
        //Previous tail 
        snake.tail[snake.tailLength] = snake.tail[snake.tailLength - 1];
    }
}

The boolean expression in apple_eaten should be returned directly rather than returning literals conditionally:

return snake.headPosition.x == applePosition.x && snake.headPosition.y == applePosition.y;

Since the results of the conditionals in game_input are mutually exclusive, you should use else on all but the first if.

Why are your widthRatio and heightRatio uint? Why should they not be promoted to floats?

\$\endgroup\$
2
  • \$\begingroup\$ should the game state structure be static aswell?, also would you mind elaborating the global state part a bit more? \$\endgroup\$ Jun 11 at 13:54
  • \$\begingroup\$ @throwaway364 Sure; edited. \$\endgroup\$
    – Reinderien
    Jun 11 at 16:39
2
\$\begingroup\$

In addition to @Reinderien good review.

Allocate consistently

Consider:

//                          Size of refenced object 
board.cells = calloc(h * w, sizeof *(board.cells));

//                         Size of refenced type.  
snake.tail = calloc(h * w, sizeof (Vec2i));

Instead consider a uniform style. I recommend the size of refenced object one. Also check return values.

// Example
snake.tail = calloc(h * w, sizeof snake.tail[0]);
if (snake.tail == NULL) {
  Handle_OutOfMemory();
}

Simplify code

    // snake.tail[snake.tailLength].x = snake.headPosition.x ;
    // snake.tail[snake.tailLength].y = snake.headPosition.y ;

    snake.tail[snake.tailLength] = snake.headPosition;

Think big (for the future)

Use size_t math rather than uint math for array index calculation.

// h * w
// ...
// *(board.cells + v.x * board.width + v.y) = c;

(size_t)h * w
...
*(board.cells + (size_t) v.x * board.width + v.y) = c;

Or simply use size_t for .x, .y, game_create(), ... rather than unsigned.

rand() % (board.width - 2) is insufficient/unbalanced when board.width on the order of RAND_MAX and RAND_MAX may be as small as 32767. Consider an assertion.

assert(board.width > RAND_MAX/4);

Avoid overflow. Perform math with the wider of size_t, unsigned, rather than unsigned.

//                                 unsigned     * unsigned
// memset(board.cells, CELL_EMPTY, board.height * board.width * sizeof *(board.cells)); 
//                              size_t                * unsigned
memset(board.cells, CELL_EMPTY, sizeof board.cells[0] * board.height * board.width); 
\$\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.