16

0x2048

enter image description here


This is my implementation of the classic game "2048" in C. The instruction to build/run the project (GitHub repo) can be found here.

I started with the core game and then built a GUI interface out of it. I would love some feedback on my design. How the same implementation can be better written. Idioms, conventions, anything that comes to your mind.

There is some information about the functions in the header files. Hope that improves readability and understanding.

Improvements I am aware of:

  • I create the texture for the text 2,4,8.. every time I draw the tiles. This is wasting resource since I can simply create the 16 tiles once and never have to worry about it again.

  • There might be some brace inconsistency between Java and C style since my IDE doesn't do this automatically.

Improvements I am unaware of:

  • Am I leaking memory anywhere?

  • style/convention/performance/memory and everything else.

  • techniques to make my code more versatile/generic. Using matrix structs, as suggested by one of the answers, would help to create rectangular boards.

Edit: The repo is updated constantly. Please use the link above to see the version of repo when this question was posted. I wouldn't mind comments on the latest version either.


game.h

/**
 * @file game.h
 * @author Gnik Droy
 * @brief File containing function declarations for the game gui.
 *
 */
#pragma once
#include "../include/core.h"
#include "SDL2/SDL.h"
#include "SDL2/SDL_ttf.h"

#define SCREEN_WIDTH 500
#define SCREEN_HEIGHT 600

/**
 * @brief Initializes the SDL window.
 *
 * When two pointers to the pointer of gWindow and gRenderer are provided,
 * the function initializes both values with the values of created window
 * and renderer. 
 * 
 * If initialization is failed it may display error to stderr but
 * does not exit.
 * 
 * @param gWindow The window of the game.
 * @param gRenderer The renderer for the game
 * @return If the initialization was successful.
 */
bool initSDL(SDL_Window **gWindow,SDL_Renderer** gRenderer);

/**
 * @brief Closes the SDL window.
 *
 * Frees up resource by closing destroying the SDL window 
 * 
 * @param gWindow The window of the game.
 */
void SDLclose(SDL_Window* gWindow);

/**
 * @brief Draws text centered inside a rect. 
 *
 * When two pointers to the pointer of gWindow and gRenderer are provided,
 * the function initializes both values with the values of created window
 * and renderer. 
 * 
 * If initialization is failed it may display error to stderr but
 * does not exit.
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the text
 * @param text The text to write
 * @param rect The SDL_Rect object inside which text is written
 * @param color The color of the text
 */
void draw_text(SDL_Renderer* gRenderer,TTF_Font* font,const char* text, SDL_Rect rect, SDL_Color color);

/**
 * @brief Draws white text centered inside a rect. 
 *
 * Same as draw_text(..., SDL_Color White)
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the text
 * @param text The text to write
 * @param rect The SDL_Rect object inside which text is written
 */
void draw_text_white(SDL_Renderer* gRenderer,TTF_Font* font,const char* text, SDL_Rect rect);


/**
 * @brief Clears the window
 *
 * Fills a color to entire screen.
 * 
 * @param gRenderer The renderer for the game
 */
void SDLclear(SDL_Renderer* gRenderer);


/**
 * @brief Draws black text centered inside the window. 
 *
 * @param gRenderer The renderer for the game
 * @param size The size for the text
 * @param text The text to write
 */
void display_text(SDL_Renderer* gRenderer,const char* text,int size);


/**
 * @brief Draws the game tiles. 
 *
 * It draws the SIZE*SIZE game tiles to the window.
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the tiles
 * @param matrix The game matrix.
 */
void draw_matrix(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font);

/**
 * @brief Draws the new game button. 
 *
 * It draws the new game button to the bottom corner.
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the button
 * @param matrix The game matrix. Needed to reset game.
 */
void draw_button(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font);

/**
 * @brief Handles the action of New Game button. 
 *
 * Resets the game board for a new game, if the correct mouse event 
 * had occured.
 * Function is run if left mouse button is released 
 * 
 * @param gRenderer The renderer for the game
 * @param e The mouse event
 * @param matrix The game matrix.
 */
void button_action(SDL_Event e,unsigned char matrix[][SIZE]);

/**
 * @brief Draws the current game score
 *
 * It draws the current game score to the window
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the tiles
 * @param matrix The game matrix.
 */
void draw_score(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font);

/**
 * @brief Draws everything for the game and renders it to screen. 
 *
 * It calls SDLclear(),draw_matrix(),draw_score() and draw_button()
 * and also renders it to screem.
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the tiles
 * @param matrix The game matrix.
 */
void render_game(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font);

/**
 * @brief This is the main game loop that handles all events and drawing 
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the tiles
 * @param matrix The game matrix.
 */
void gameLoop(unsigned char matrix[][SIZE],SDL_Renderer* gRenderer);

/**
 * @brief Handles keyboard presses that correspond with the arrowkeys. 
 * 
 * It transforms the game matrix according to the keypresses.
 * It also checks if the game has been finished, draws game over screen 
 * and resets the board if game over. 
 * 
 * @param gRenderer The renderer for the game
 * @param font The font for the tiles
 * @param matrix The game matrix.
 */
void handle_move(SDL_Event e,unsigned char matrix[][SIZE], SDL_Renderer * gRenderer);

styles.h

/**
 * @file styles.h
 * @author Gnik Droy
 * @brief File containing tile colors and related structs.
 *
 */
#pragma once
/** @struct COLOR
 *  @brief This structure defines a RBGA color
 *  All values are stored in chars.
 * 
 *  @var COLOR::r
 *  The red value 
 *  @var COLOR::g
 *  The green value 
 *  @var COLOR::b 
 *  The blue value
 *  @var COLOR::a 
 *  The alpha value
 * 
 */

//Screen dimension constants
#define SCREEN_WIDTH 500
#define SCREEN_HEIGHT 600
#define SCREEN_PAD 10

//FONT settings
#define FONT_PATH "UbuntuMono-R.ttf"
#define TITLE_FONT_SIZE 200
#define GOVER_FONT_SIZE 100  //Game Over font size
#define CELL_FONT_SIZE 40

struct COLOR{
    char r;
    char g;
    char b;
    char a;
};
struct COLOR g_bg={211, 204, 201, 255};
struct COLOR g_fg={80, 80, 80, 255};
struct COLOR g_button_bg={255, 153, 102,255};
struct COLOR g_score_bg={143, 122, 102,255};

struct COLOR g_COLORS[]={
    {230, 227, 232,255},
    {255, 127, 89,255},
    {224, 74, 69,255},
    {237, 207, 114,255},
    {65, 216, 127,255},
    {54, 63, 135,255},
    {78, 89, 178,255},
    {109, 118, 191,255},
    {84, 47, 132,255},
    {125, 77, 188,255},
    {163, 77, 188,255},
    {176, 109, 196,255},
    {0, 102, 204,255},
    {0, 153, 255,255},
    {51, 153, 255,255},
    {153, 204, 255,255},
    {102, 255, 102,255}
};

core.h

/**
 * @file core.h
 * @author Gnik Droy
 * @brief File containing function declarations for the core game.
 *
 */
#pragma once
#include <stdio.h>

#define SIZE 4
#define BASE 2

typedef char bool;

/**
 * @brief Write the game matrix to the stream.
 *
 * The matrix is written as a comma seperated list of indices.
 * Each row is seperated by a '\n' character.
 * Each empty cell is represented by '-' character.
 * 
 * The indices can be used to calculate the actual integers.
 * 
 * You can use the constant stdout from <stdio.h> for printing to 
 * standard output
 * 
 * @param matrix The game matrix that is to be printed.
 * @param stream The file stream to use.
 */
void print_matrix(unsigned char matrix[][SIZE],FILE* stream);


/**
 * @brief Checks if there are possible moves left on the game board.
 *
 * Checks for both movement and combinations of tiles.
 * 
 * @param matrix The game matrix.
 * @return Either 0 or 1
 */
bool is_game_over(unsigned char matrix[][SIZE]);

/**
 * @brief This clears out the game matrix
 *
 * This zeros out the entire game matrix.
 * 
 * @param matrix The game matrix.
 */
void clear_matrix(unsigned char matrix[][SIZE]);

/**
 * @brief Adds a value of 1 to random place to the matrix.
 *
 * The function adds 1 to a random place in the matrix.
 * The 1 is placed in empty tiles. i.e tiles containing 0.
 * 1 is kept since you can use raise it with BASE to get required value.
 * Also it keeps the size of matrix to a low value.
 * 
 * NOTE: It has no checks if there are any empty places for keeping 
 * the random value.
 * If no empty place is found a floating point exception will occur.
 */
void add_random(unsigned char matrix[][SIZE]);

/**
 * @brief Calculates the score of a game matrix
 *
 * It score the matrix in a simple way.
 * Each element in the matrix is used as exponents of the BASE. And the 
 * sum of all BASE^element is returned.
 * 
 * @return An integer that represents the current score
 */
int calculate_score(unsigned char matrix[][SIZE]);





/**
 * @brief Shifts the game matrix in X direction.
 *
 * It shifts all the elements of the game matrix in the X direction.
 * If the direction is given as 0, it shifts the game matrix in the left
 * direction. Any other non zero value shifts it to the right direction.
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the shift.
 * 
 * @return If the shift was successful
 */
bool shift_x(unsigned char matrix[][SIZE], bool opp);


/**
 * @brief Merges the elements in X direction.
 *
 * It merges consecutive successive elements of the game matrix in the X direction.
 * If the direction is given as 0, it merges the game matrix to the left
 * direction. Any other non zero value merges it to the right direction.
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the shift.
 * 
 * @return If the merge was successful
 */
bool merge_x(unsigned char matrix[][SIZE],bool opp);


/**
 * @brief Moves the elements in X direction.
 *
 * It simply performs shift_x() and merge_x().
 * If either of them were successful, it also calls add_random()
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the move.
 * 
 */
void move_x(unsigned char matrix[][SIZE], bool opp);



/**
 * @brief Shifts the game matrix in Y direction.
 *
 * It shifts all the elements of the game matrix in the Y direction.
 * If the direction is given as 0, it shifts the game matrix in the top
 * direction. Any other non-zero value shifts it to the bottom.
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the shift.
 * 
 * @return If the shift was successful
 */
bool shift_y(unsigned char matrix[][SIZE], bool opp);


/**
 * @brief Merges the elements in Y direction.
 *
 * It merges consecutive successive elements of the game matrix in the Y direction.
 * If the direction is given as 0, it merges the game matrix to the top
 * direction. Any other non zero value merges it to the bottom.
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the shift.
 * 
 * @return If the merge was successful
 */
bool merge_y(unsigned char matrix[][SIZE],bool opp);


/**
 * @brief Moves the elements in Y direction.
 *
 * It simply performs shift_y() and merge_y().
 * If either of them were successful, it also calls add_random()
 * 
 * @param matrix The game matrix.
 * @param opp The direction of the move.
 * 
 */
void move_y(unsigned char matrix[][SIZE],bool opp);

core.c

#include <stdlib.h>
#include <time.h>
#include <math.h>
#include "../include/core.h"


void clear_matrix(unsigned char matrix[][SIZE])
{
    for (unsigned int x=0;x<SIZE;x++)
    {
        for(unsigned int y=0;y<SIZE;y++)
        {
            matrix[x][y]=0;
        }
    }
}

int calculate_score(unsigned char matrix[][SIZE])
{
    int score=0;
    for (unsigned int x=0;x<SIZE;x++)
    {
        for(unsigned int y=0;y<SIZE;y++)
        {
            if(matrix[x][y]!=0)
            {
                score+=pow(BASE,matrix[x][y]);
            }
        }
    }
    return score;
}

void print_matrix(unsigned char matrix[][SIZE],FILE* stream)
{
    for (unsigned int x=0;x<SIZE;x++)
    {
        for(unsigned int y=0;y<SIZE;y++)
        {
            if (matrix[x][y])
            {
                fprintf(stream,"%d," ,matrix[x][y]);
            } else{
                fprintf(stream,"-,");
            }
        }
        fprintf(stream,"\n");
    }
    fprintf(stream,"\n");
}

void add_random(unsigned char matrix[][SIZE])
{
    unsigned int pos[SIZE*SIZE];
    unsigned int len=0;
    for(unsigned int x=0;x<SIZE;x++)
    {
        for (unsigned int y=0;y<SIZE;y++)
        {
            if (matrix[x][y]==0){
               pos[len]=x*SIZE+y;
               len++; 
            }
        }
    }
    unsigned int index=rand() % len;
    matrix[pos[index]/SIZE][pos[index]%SIZE] = 1;
}

bool is_game_over(unsigned char matrix[][SIZE])
{
    for(unsigned int x=0;x<SIZE-1;x++)
    {
        for (unsigned int y=0;y<SIZE-1;y++)
        {
            if (  matrix[x][y]==matrix[x][y+1] ||
                  matrix[x][y]==matrix[x+1][y] ||
                  matrix[x][y]==0)
            {return 0;}
        }
        if( matrix[x][SIZE-1]==matrix[x+1][SIZE-1] || 
            matrix[x][SIZE-1]==0) return 0;
        if( matrix[SIZE-1][x]==matrix[SIZE-1][x+1] ||
            matrix[SIZE-1][x]==0) return 0;
    }
    return 1;
}

bool shift_x(unsigned char matrix[][SIZE], bool opp)
{
    bool moved=0;
    int start=0,end=SIZE,increment=1;
    if (opp)
    {
        start=SIZE-1;
        end=-1;
        increment=-1;
    }
    for (int x=0;x<SIZE;x++)
    {
        int index=start;
        for(int y=start;y!=end;y+=increment)
        {
            if (matrix[x][y]!=0)
            {
                matrix[x][index]=matrix[x][y];
                if(index!=y) {
                    matrix[x][y]=0;
                    moved=1;
                }
                index+=increment;
            }
        }
    }
    return moved;
}
bool merge_x(unsigned char matrix[][SIZE],bool opp)
{
    bool merged=0;
    int start=0,end=SIZE-1,increment=1;
    if (opp)
    {
        start=SIZE-1;
        end=0;
        increment=-1;
    }
    for (int x=0;x<SIZE;x++)
    {
        int index=start;
        for(int y=start;y!=end;y+=increment)
        {
            if(matrix[x][y]!=0)
            {
                if(matrix[x][y]==matrix[x][y+increment])
                {
                    matrix[x][index]=matrix[x][y]+1;
                    matrix[x][y+increment]=0;
                    if(index!=y) matrix[x][y]=0;
                    merged=1;
                    index+=increment;
                }
                else
                {
                    matrix[x][index]=matrix[x][y];
                    if(index!=y) matrix[x][y]=0;
                    index+=increment;
                }
            }
        }

        if(matrix[x][end]!=0)
        {
            matrix[x][index]=matrix[x][end];
            if(index!=end) matrix[x][end]=0;
        }
    }
    return merged;
}
bool merge_y(unsigned char matrix[][SIZE],bool opp)
{
    bool merged=0;
    int start=0,end=SIZE-1,increment=1;
    if (opp)
    {
        start=SIZE-1;
        end=0;
        increment=-1;
    }
    for (int y=0;y<SIZE;y++)
    {
        int index=start;
        for(int x=start;x!=end;x+=increment)
        {
            if(matrix[x][y]!=0)
            {
                if(matrix[x][y]==matrix[x+increment][y])
                {
                    matrix[index][y]=matrix[x][y]+1;
                    matrix[x+increment][y]=0;
                    if(index!=x) matrix[x][y]=0;
                    index+=increment;
                    merged=1;
                }
                else
                {
                    matrix[index][y]=matrix[x][y];
                    if(index!=x) matrix[x][y]=0;
                    index+=increment;
                }
            }
        }
        if(matrix[end][y]!=0)
        {
            matrix[index][y]=matrix[end][y];
            if(index!=end) matrix[end][y]=0;
        }

    }
    return merged;
}
bool shift_y(unsigned char matrix[][SIZE],bool opp)
{
    bool moved=0;
    int start=0,end=SIZE,increment=1;
    if (opp)
    {
        start=SIZE-1;
        end=-1;
        increment=-1;
    }
    for (int y=0;y<SIZE;y++)
    {
        int index=start;
        for(int x=start;x!=end;x+=increment)
        {
            if (matrix[x][y]!=0)
            {
                matrix[index][y]=matrix[x][y];
                if(index!=x)
                { 
                    matrix[x][y]=0;
                    moved=1;
                }
                index+=increment;
            }
        }
    }
    return moved;
}


inline void move_y(unsigned char matrix[][SIZE],bool opp)
{
    //Assigning values insted of evaluating directly to force both operations
    //Bypassing lazy 'OR' evaluation
    bool a=shift_y(matrix,opp),b=merge_y(matrix,opp);
    if( a||b) add_random(matrix);
}

inline void move_x(unsigned char matrix[][SIZE],bool opp)
{
    //Assigning values insted of evaluating directly to force both operations
    //Bypassing lazy 'OR' evaluation
    bool a=shift_x(matrix,opp), b=merge_x(matrix,opp);
    if(a||b)add_random(matrix);
}

game.c

#include "../include/styles.h"
#include "../include/game.h"
#include <time.h>
#include <stdlib.h>

bool initSDL(SDL_Window **gWindow,SDL_Renderer** gRenderer)
{
    bool success = 1;
    TTF_Init();
    if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
    {
        perror( "SDL could not initialize!" );
        success = 0;
    }
    else
    {
        *gWindow = SDL_CreateWindow( "2048", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN );
        if( gWindow == NULL )
        {
            perror( "Window could not be created!" );
            success = 0;
        }
        else
        {
            *gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
            if( gRenderer == NULL )
            {
                perror( "Renderer could not be created!" );
                success = 0;
            }
            else
            {
                SDL_SetRenderDrawColor( *gRenderer, g_bg.r,g_bg.g,g_bg.b,g_bg.a );

            }
        }
    }

    return success;
}

void draw_text(SDL_Renderer* gRenderer,TTF_Font* font,const char* text, SDL_Rect rect, SDL_Color color){
    SDL_Surface* surfaceMessage = TTF_RenderText_Blended(font, text, color); 
    SDL_Texture* Message = SDL_CreateTextureFromSurface(gRenderer, surfaceMessage);
    SDL_Rect message_rect;

    TTF_SizeText(font, text, &message_rect.w, &message_rect.h);
    message_rect.x = rect.x+rect.w/2-message_rect.w/2;     
    message_rect.y = rect.y+rect.h/2-message_rect.h/2; 

    SDL_RenderCopy(gRenderer, Message, NULL, &message_rect); 
    SDL_DestroyTexture(Message);
    SDL_FreeSurface(surfaceMessage);
}

void draw_text_white(SDL_Renderer* gRenderer,TTF_Font* font,const char* text, SDL_Rect rect)
{
    SDL_Color White = {255, 255, 255};
    draw_text(gRenderer,font,text,rect,White);
}


void SDLclose(SDL_Window* gWindow)
{
    SDL_DestroyWindow( gWindow );
    gWindow = NULL;
    TTF_Quit();
    SDL_Quit();
}

void SDLclear(SDL_Renderer* gRenderer)
{

    SDL_SetRenderDrawColor( gRenderer, g_bg.r,g_bg.g,g_bg.b,g_bg.a );
    SDL_RenderClear( gRenderer );

}

void display_text(SDL_Renderer* gRenderer,const char* text,int size)
{
    TTF_Font* font =NULL;
    font= TTF_OpenFont(FONT_PATH, size);
    if(font==NULL){
        perror("The required font was not found");
        exit(1);
    }
    SDL_Color black = {g_fg.r,g_fg.g, g_fg.b};
    SDLclear(gRenderer);
    SDL_Rect rect = {SCREEN_PAD ,SCREEN_HEIGHT/4 , SCREEN_WIDTH-2*SCREEN_PAD , SCREEN_HEIGHT/2 };
    draw_text(gRenderer,font,text,rect,black);
    SDL_RenderPresent( gRenderer );
    SDL_Delay(1000);
    TTF_CloseFont(font);
    font=NULL;
}
void draw_matrix(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font)
{
    int squareSize=(SCREEN_WIDTH - 2*SCREEN_PAD)/SIZE-SCREEN_PAD;

    for(int x=0;x<SIZE;x++)
    {
        for(int y=0;y<SIZE;y++)
        {
            SDL_Rect fillRect = { SCREEN_PAD+x*(squareSize+SCREEN_PAD), SCREEN_PAD+y*(squareSize+SCREEN_PAD), squareSize , squareSize };
            struct COLOR s=g_COLORS[matrix[y][x]];
            SDL_SetRenderDrawColor( gRenderer, s.r, s.g, s.b, s.a );        
            SDL_RenderFillRect( gRenderer, &fillRect );
            char str[15]; // 15 chars is enough for 2^16
            sprintf(str, "%d", (int)pow(BASE,matrix[y][x]));

            if(matrix[y][x]==0){
                 str[0]=' ';
                 str[1]='\0';
             }
            draw_text_white(gRenderer,font,str,fillRect);
        }
    }
}



void handle_move(SDL_Event e,unsigned char matrix[][SIZE], SDL_Renderer * gRenderer)
{
    if(is_game_over(matrix))
    {
        display_text(gRenderer,"Game Over",GOVER_FONT_SIZE);
        clear_matrix(matrix);
        add_random(matrix);
        return;
    }
    switch(e.key.keysym.sym)
    {
        case SDLK_UP: 
        move_y(matrix,0);
        break;
        case SDLK_DOWN: 
        move_y(matrix,1);
        break;
        case SDLK_LEFT: 
        move_x(matrix,0);
        break;
        case SDLK_RIGHT: 
        move_x(matrix,1);
        break;
        default:;
    }
}

void draw_button(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font)
{
    char txt[]="New Game";
    SDL_Rect fillRect = { SCREEN_PAD/2 ,
                            SCREEN_WIDTH+SCREEN_PAD ,
                            SCREEN_WIDTH/2-2*SCREEN_PAD ,
                            (SCREEN_HEIGHT-SCREEN_WIDTH)-2*SCREEN_PAD };
    SDL_SetRenderDrawColor( gRenderer,g_button_bg.r, g_button_bg.g, g_button_bg.b,g_button_bg.a );      
    SDL_RenderFillRect( gRenderer, &fillRect );
    draw_text_white(gRenderer,font,txt,fillRect);

}
void button_action(SDL_Event e,unsigned char matrix[][SIZE])
{
    SDL_Rect draw_rect = { SCREEN_PAD/2 ,
                            SCREEN_WIDTH+SCREEN_PAD ,
                            SCREEN_WIDTH/2-2*SCREEN_PAD ,
                            SCREEN_HEIGHT-SCREEN_WIDTH-2*SCREEN_PAD };
    if(e.button.button == SDL_BUTTON_LEFT &&
                e.button.x >= draw_rect.x &&
                e.button.x <= (draw_rect.x + draw_rect.w) &&
                e.button.y >= draw_rect.y &&
                e.button.y <= (draw_rect.y + draw_rect.h))
    {
        clear_matrix(matrix);
        add_random(matrix);
    }
}
void draw_score(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font)
{
    char score[15]; //15 chars is enough for score.
    sprintf(score, "%d", calculate_score(matrix));
    char scoreText[30]="Score:";
    strncat(scoreText,score,15);
    SDL_Rect fillRect = {   SCREEN_WIDTH/2+5,
                            SCREEN_WIDTH+SCREEN_PAD,
                            SCREEN_WIDTH/2-2*SCREEN_PAD,
                            SCREEN_HEIGHT-SCREEN_WIDTH-2*SCREEN_PAD };
    SDL_SetRenderDrawColor( gRenderer,g_score_bg.r,g_score_bg.g,g_score_bg.b,g_score_bg.a );        
    SDL_RenderFillRect( gRenderer, &fillRect );
    draw_text_white(gRenderer,font,scoreText,fillRect);

}
void render_game(SDL_Renderer* gRenderer,unsigned char matrix[][SIZE], TTF_Font* font)
{
    SDLclear(gRenderer);
    draw_matrix(gRenderer,matrix,font);
    draw_score(gRenderer,matrix,font);
    draw_button(gRenderer,matrix,font);
    SDL_RenderPresent( gRenderer );
}

void gameLoop(unsigned char matrix[][SIZE],SDL_Renderer* gRenderer)
{
    TTF_Font* font =NULL;
    font= TTF_OpenFont(FONT_PATH, CELL_FONT_SIZE);
    if(font==NULL){
        perror("The required font was not found");
        exit(1);
    }

    render_game(gRenderer,matrix,font);

    bool quit=0;
    SDL_Event e;
    while (!quit)
    {
        while( SDL_PollEvent( &e ) != 0 )
        {
            //User requests quit
            if( e.type == SDL_QUIT )
            {
                quit = 1;
            }
            else if(e.type==SDL_KEYUP)
            {
                handle_move(e,matrix,gRenderer);
                //Redraw all portions of game
                render_game(gRenderer,matrix,font);
            }
            else if(e.type==SDL_MOUSEBUTTONUP)
            {
                button_action(e,matrix);
                render_game(gRenderer,matrix,font);
            }
        }
    }
    TTF_CloseFont(font);
    //No need to null out font.
}


int main(int argc,char** argv)
{
    //Set up the seed 
    srand(time(NULL));

    //Set up the game matrix.
    unsigned char matrix[SIZE][SIZE];
    clear_matrix(matrix);
    add_random(matrix);

    //Init the SDL gui variables
    SDL_Window* gWindow = NULL;
    SDL_Renderer* gRenderer = NULL;
    if(!initSDL(&gWindow,&gRenderer)){exit(0);};

    display_text(gRenderer,"2048",TITLE_FONT_SIZE);
    gameLoop(matrix,gRenderer);

    //Releases all resource
    SDLclose(gWindow);   
    return 0;
}
  • 3
    This is a great question and I enjoyed reading it and the review. Just a note: only the code embedded directly into the question is eligible for review. (The GitHub link can stay for anyone interested in viewing it BUT it isn't valid for review.) If you want a review of the updated code it is perfectly acceptable to post a new question when you've implemented the changes. We love iterative reviews especially on fun questions. – bruglesco 11 hours ago
  • Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend? – austingae 10 hours ago
  • 1
    @austingae 2048 was originally written in javascript. It was a single player game so no backend was used. But you can easily use a python backend and make a (multiplayer) 2048 with leaderboards and other modes/variants. – Gnik 5 hours ago
16

Well done. This is not a complete review, but instead a (short) list of possible improvements I found when I skimmed your code.

Documentation

First of all: thank you! It's great to have documentation.

Note that there is some debate whether to put the documentation into the header or the source. I'd like to remark that I would put only a @brief description in the header and a complete documentation into the source. That way, one can get a quick overview of all functions and look into the details if they found the correct one. However, that's personal preference, and in a team project you would stick to whatever guideline is already present. The generated documentation by doxygen will stay the same, either way.

Matrices and arr[][SIZE]

While it's possible to model a matrix this way, it's inflexible. The game is now stuck at a size that was chosen when it was compiled. If you want to enable other board sizes, you have to add some logic to keep the board in the matrix anyway, so a variable SIZE will be necessary. Also, interesting boards like 4x6 are downright impossible at the moment.

Furthermore, there is a lot of duplication, as unsigned char matrix[][SIZE] is everywhere in the code. If you really want to follow the approach, use a type alias:

typedef unsigned char board_type[][SIZE];

However, with arbitrary board sizes in mind, you probably want to introduce a proper matrix type at some point:

struct matrix_type {
    unsigned width;
    unsigned height;
    unsigned char * actual_board;
};

Whether you use a single allocated malloc(sizeof(*actual_board)*SIZE*SIZE) or SIZE times malloc(sizeof(*actual_board)*SIZE) is, at least for small sizes, not important. The former is easier to handle in terms of memory, the latter is easier in terms of access.

pow is not for integers

The pow function takes a double for both arguments, and that's fine. However, it's a complete overkill for simple integers. In a past review, I share some more details, but for your game a simple bitshift is enough:

unsigned long pow_integral(unsigned char base, unsigned char exponent) {
    if(base == 2) {
        return (1 << exponent);
    } else {
        // exercise; use "double-and-add" method for logarithmic speed
    }
}

No magic numbers

Just like documentation, this is a great feature of your code. There are no magic numbers in the code, every number is properly defined to provide some self-documentation. However, some comments on #defines are usually expected, and Doxygen should give out some warnings.

There is a single magic number, though, in main. See "blindness" below.

C99 has bool

That being said, occasionally there is bool success = 1 or 0. Due to bool, it's clear that they mean true and false. You could, however, just #include <stdbool.h> and instead use the language defined boolean.

Tabs and spaces

There are tabs and spaces mixed in the code. It's not evident in the code here on StackExchange, but on GitHub. You probably want to fix this, as several editors use 8 spaces for tabs, not 4.

perror is not for general errors

perror will show the user supplied string, as well as a textual description of the error code stored on errno. None of the SDL functions set errno as far as I know, so perror won't report the correct errors.

Instead, use printf or fprintf and SDL_GetError.

Early returns

Some of your functions have a return code ready, for example initSDL:

bool initSDL(SDL_Window **gWindow,SDL_Renderer** gRenderer)
{
    bool success = 1;
    TTF_Init();
    if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
    {
        perror( "SDL could not initialize!" );
        success = 0;
    }
    else
    {
        *gWindow = SDL_CreateWindow( "2048", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN );
        if( gWindow == NULL )
        {
            perror( "Window could not be created!" );
            success = 0;
        }
        else
        {
            *gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
            if( gRenderer == NULL )
            {
                perror( "Renderer could not be created!" );
                success = 0;
            }
            else
            {
                SDL_SetRenderDrawColor( *gRenderer, g_bg.r,g_bg.g,g_bg.b,g_bg.a );

            }
        }
    }

    return success;
}

This code suffers a little bit from the pyramid of doom. However, in none of the ifs do we actually clean up resources, so we can instead write the following:

bool initSDL(SDL_Window **gWindow,SDL_Renderer** gRenderer)
{
    TTF_Init();
    if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
    {
        fprintf(stderr, "SDL could not initialize: %s\n", SDL_GetError());
        return false;
    }
    *gWindow = SDL_CreateWindow( "2048", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_SHOWN );
    if( gWindow == NULL )
    {
        fprintf(stderr, "Window could not be created: %s\n", SDL_GetError());
        return false;
    }
    *gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
    if( gRenderer == NULL )
    {
        fprintf(stderr, "Renderer could not be created: %s\n", SDL_GetError());
        // What about gWindow?
        // We should probably do something about it
        return false;
    }
    SDL_SetRenderDrawColor( *gRenderer, g_bg.r,g_bg.g,g_bg.b,g_bg.a );
}

Resource management and boolean blindness

As hinted in the code above, when CreateWindow succeeds but CreateRenderer fails, the gWindow isn't properly destroyed. Furthermore, initSDL's caller cannot find out why the initialization failed. Enums are usually the solution in that circumstance, at least as long as we don't clean up.

That being said, exit(0) in main is off. A zero exit value indicates that the game was able to run and exit. However, if initSDL fails, then the game cannot run, and we should probably report that to the operating system:

if(!initSDL(&gWindow,&gRenderer)){exit(EXIT_FAILURE);};

Always use (proper) blocks for ifs

However, the line is strange for another reason: it doesn't follow your usual indentation. Let's fix that:

if(!initSDL(&gWindow,&gRenderer)){
    exit(EXIT_FAILURE);
}; // <<--- ?

There was a stray semicolon. While it's not an error, it indicates that the code was previously if(!initSDL(&gWindow,&gRenderer))exit(0);, then some things got changed, and changed back.

If you use braces all the time (with proper indentation), it's easier to see conditionals, so make sure to make the code as clean as possible.

Readability

The compiler doesn't care whether you write

a=foo(), b=too(), c=quux()+a*b; if(a<b)c-=t;

but a human will have a hard time. Make your code easy for humans too:

a = foo();
b = too();
c = quux() + a * b;

if ( a < b ) {
    c -= t;
}

At least move_x and move_y can be improved that way.

Naming

In computer sciences, there are two hard problems: naming, caches and off-by one errors. Here we focus on the first one.

Use prefixes only if they have a meaning

The gRenderer and gWindow have a g prefix that's not explained. None of the other variables have a prefix.

If g is a common prefix for SDL objects, then it's fine, however, I guess it's for game. However, it's strange that the board itself then is prefix-free.

That being said...

Name by function, not by form

The board is called matrix throughout the whole game. However, matrix is a term from Mathematics or a film title, but doesn't quite fit the "board" function. board on the other hand would be a perfect name. Also, the plural is a lot easier, in case you ever want to implement a variant where the player plays on two boards at the same time.

Don't surprise the others

The SDLclose and SDLclear took me by surprise. Both functions don't follow the usual SDL_<name> approach, because both aren't from SDL.

In C++, you would put those functions into your own namespace, but in C, use a prefix that you defined. Alternatively, follow the initSDL approach and call the functions clearSDL and closeSDL. Those names are completely unambiguous.

Symmetry between creation and destruction

There is something amiss in SDLclose:

void SDLclose(SDL_Window* gWindow)
{
    SDL_DestroyWindow( gWindow );
    gWindow = NULL;
    TTF_Quit();
    SDL_Quit();
}

Given that we act on a local pointer, gWindow = NULL isn't visible from the outside. This is probably just a small mistake. However, if SDLclos and initSDL were to be used in a symmetric way, we'd end up with

void SDLclose(SDL_Window** gWindow)
{
    SDL_DestroyWindow( *gWindow );
    *gWindow = NULL;
    TTF_Quit();
    SDL_Quit();
}

Here, *gWindow = NULL makes sense. However, as SDLclose is one of the few functions that don't have accommodating documentation, it's not clear what the intended behaviour is, so I'd stick to the former, e.g.

/**
 * @brief Destroy the @a gWindow and quit SDL.
 *
 * @param gWindow is the window that will be destroyed.
*/
void SDLclose(SDL_Window* gWindow)
{
    //! @warning `gWindow` **must not** be used afterwards.
    SDL_DestroyWindow( gWindow );
    TTF_Quit();
    SDL_Quit();
}
  • Thank you for the excellent review! I didn't think about bitshifts or pow()'s runtime. SDL_GetError was also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realize exit(0/1) could be removed as well. It was a pleasant surprise. I selected the matrix[][SIZE] format in the view that I would never fiddle with non-square boards. But, now that you mention it, it seems fun to implement. Thank you once again! – Gnik 16 hours ago
  • Not the OP, but can you explain: "Whether you use a single allocated malloc(sizeof(actual_board)*SIZESIZE) or SIZE times malloc(sizeof(*actual_board)*SIZE) is, at least for small sizes, not important. The former is easier to handle in terms of memory, the latter is easier in terms of access. ", specifically why the latter is easier in terms of access. – Mar Dev 9 hours ago
  • The SDLclose function definitely contains a bug. I had fixed that in the latest revision but changes were still left here. Maybe I will post the updated question after some implementation changes in a few weeks. It is also scary how well you understood the code. I really appreciate the time you game for this review. – Gnik 5 hours ago

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.