Simple snake game with C and raylib

Question

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?

Answer 1

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?

• throwaway364: should the game state structure be static aswell?, also would you mind elaborating the global state part a bit more?
• Reinderien: @throwaway364 Sure; edited.

Answer 2

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);