25
\$\begingroup\$

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;
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ 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. \$\endgroup\$
    – Summer
    Commented Dec 27, 2018 at 19:57
  • 1
    \$\begingroup\$ Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend? \$\endgroup\$
    – austingae
    Commented Dec 27, 2018 at 20:53
  • 2
    \$\begingroup\$ @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. \$\endgroup\$
    – Gnik
    Commented Dec 28, 2018 at 1:58

3 Answers 3

24
\$\begingroup\$

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 board_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.

In case you ever want to swap between those, a set of small inline function can come in handy:

unsigned char board_get(struct board_type *board, unsigned row, unsigned col) {
    assert(row < board->height);
    assert(col < board->width);
    return board->actual_board[row * board->width + col];
    // or, if actual_board is a `unsigned char**`
    return board->actual_board[row][col];
}

void board_set(struct board_type *board, unsigned row, unsigned col, unsigned char value) {
    assert(row < board->height);
    assert(col < board->width);
    board->actual_board[row * board->width + col] = value;
    // or, if actual_board is a `unsigned char**`
    board->actual_board[row][col] = value;
}

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 (1lu << 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();
}
\$\endgroup\$
4
  • \$\begingroup\$ 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! \$\endgroup\$
    – Gnik
    Commented Dec 27, 2018 at 15:15
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 27, 2018 at 22:00
  • \$\begingroup\$ 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. \$\endgroup\$
    – Gnik
    Commented Dec 28, 2018 at 2:19
  • \$\begingroup\$ @MarDev Sure. The first one will lead to a TYPE * matrix. The valid indices are now \$0,1,\ldots,\text{N}^2-1\$. This is very handy for resetting, as you only need memset, but now you need matrix[row + column * N] or similar. The latter leads to TYPE **matrix. We now have an indirection and must use matrix[i][j]. This is a lot easier if you want to use positions, as we don't have to transform \$\{0,\ldots\,N-1\}\times\{0,\ldots\,N-1\}\to\{0,\ldots,N^2-1\}\$ by hand with \$ind = xN+y\$. However, the is unlikely to be continuous anymore. I've added examples to the answer. \$\endgroup\$
    – Zeta
    Commented Dec 28, 2018 at 9:10
4
\$\begingroup\$

Since the other reviews have already hit most points, I'll just mention a few not already covered.

Avoid relative paths in #includes

Generally it's better to omit relative path names from #include files and instead point the compiler to the appropriate location. So instead of this:

#include "../include/styles.h"
#include "../include/game.h"

write this:

#include "styles.h"
#include "game.h"

This makes the code less dependent on the actual file structure, and leaving such details in a single location: a Makefile or compiler configuration file. With cmake, we can use include_directories. Since you've already got that in your toplevel CMakeLists.txt, just append the include directory in that CMake directive.

Understand how #include works

On most platforms, the difference between #include "math.h" and #include <math.h> is that the former looks first in the current directory. So for system files such as SDL2/SDL.h, you should really use #include <SDL2/SDL.h> instead. See this question for more details.

In many cases, it's likely that either will work, but to the human reader convention is that files in your project use "" while system includes (files not in your project) use <>. That's an imprecise differentiation, but a useful way to think about it.

Don't Repeat Yourself (DRY)

The merge_x and merge_y functions are almost identical. I think it would make sense to combine them into a single merge function that would take a direction as an additional parameter. The same approach can be taken with the shift and move functions.

For example, here's a combined shift() function that takes an extra parameter indicating ydir:

bool shift(Board board, bool opp, bool ydir)
{
    bool moved=false;
    int start=0,end=SIZE,increment=1;
    if (opp)
    {
        start=SIZE-1;
        end=-1;
        increment=-1;
    }
    for (int a=0;a<SIZE;a++)
    {
        int index=start;
        for(int b=start;b!=end;b+=increment)
        {
            int x = ydir ? b : a;
            int y = ydir ? a : b;
            if (board[x][y]!=0)
            {
                if (ydir) {
                    board[index][y]=board[x][y];
                } else {
                    board[x][index]=board[x][y];
                }
                if(index!=b) {
                    board[x][y]=0;
                    moved=true;
                }
                index+=increment;
            }
        }
    }
    return moved;
}

Use const where practical

The Board is not and should not be altered by the print_board function. For that reason, I would advise changing the signature of the function to this:

void print_board(const Board board, FILE* stream);

A similar change can be made to is_game_over and calculate_score

Don't leak memory

The SDL interface is hard to use correctly without leaking memory, because it isn't always readily apparent which functions allocate and which functions de-allocate. In this code, initSDL creates a renderer but never calls SDL_DestroyRenderer. I'd recommend adding a pointer to the renderer as a parameter to closeSDL and making sure it's non-NULL before calling SDL_DestroyRenderer.

Simplify code

The code currently contains this function:

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

It could be more clearly written as:

inline void move_x(Board board, bool opp)
{
    bool move_or_shift = shift_x(board,opp);
    move_or_shift |= merge_x(board,opp);
    if (move_or_shift) {
        add_random(board);
    }
}

Think of the user

There are a few small enhancements that would make the game better. First is to allow the user to see and savor the high score instead of immediately launching a new game. Second would be to detect whether any moves are possible rather than waiting for the user to attempt to move before evaluating this.

\$\endgroup\$
3
  • \$\begingroup\$ Great review! One thing I would like to be clarified is the DRY principle. I tried to make a single function for shift instead of shift_x and shift_y. Could you show me if that is possible? I thought about repeating code for clarity instead of writing a complex shift function. How would you go about this? Also for the Simplify Code section. I didn't get what you mean. You just provided the code and I think the function is simple enough. How could I make it simpler? Thank you once more! \$\endgroup\$
    – Gnik
    Commented Dec 29, 2018 at 3:17
  • \$\begingroup\$ About the includes. I am rather inconsistent with the style but isn't it almost always better to write " " instead of <>? Especially since #include "SDL2/SDL2.h" fallbacks to #include <SDL2/SDL2.h> if nothing is found in the source directory. Therefore, the user can provide his own implementation of "math" and "time" for example if need be. Maybe reduce file size by only providing certain functions of the library and so on. Is there any advantage to using < >? \$\endgroup\$
    – Gnik
    Commented Dec 29, 2018 at 3:39
  • 1
    \$\begingroup\$ I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again. \$\endgroup\$
    – Edward
    Commented Dec 29, 2018 at 4:04
3
\$\begingroup\$

Adding to @Zeta's excellent answer.


Whitespace

Your whitespace policy is pretty inconsistent. Sometimes you call functions like this:

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

with spaces just inside the (), while other times you call them like this:

TTF_SizeText(font, text, &message_rect.w, &message_rect.h);

with no spaces inside. Similarly, you sometimes put spaces after commas, as above, and other times you don't, like here:

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

On some lines you mix the two styles, like here:

struct COLOR g_COLORS[]={
    {230, 227, 232,255},
    {255, 127, 89,255},
    // etc.
};

Sometimes you put spaces around arithmetic operators, but you usually don't.

int squareSize=(SCREEN_WIDTH - 2*SCREEN_PAD)/SIZE-SCREEN_PAD;

You also sometimes associate pointers with the type, and other times with the variable:

bool initSDL(SDL_Window **gWindow,SDL_Renderer** gRenderer)

Lastly, in your header you sometimes have two blank lines separating docstring-forward-declaration pairs, and sometimes you have just one.

I would encourage you to use clang-format to keep your style consistent.

Correctness

Code like this always has bugs, even if they're just theoretical portability bugs. It's best to get out of the habit of writing it, even if it's working on your computer.

char score[15]; //15 chars is enough for score.
sprintf(score, "%d", calculate_score(matrix));
char scoreText[30]="Score:";
strncat(scoreText,score,15);

In particular, the C standard allows int to occupy more than 4 bytes. Instead, you should either ask snprintf for the length at runtime and allocate yourself or use asprintf (which is a GNU extension). To ask snprintf for the required buffer size, give it a null pointer and zero length, like so:

int score = calculate_score(matrix);
int score_bufsize = snprintf(NULL, 0, "Score: %d", score);
char* score_str = malloc(score_bufsize * sizeof(*score));
if (!score_str) {
    perror("malloc");
    exit(EXIT_FAILURE);
}

snprintf(score_str, score_bufsize, "Score: %d", score);

// ...

draw_text_white(gRenderer,font,score_str,fillRect);
free(score_str);

This pattern is made easier if you're willing to use GNU extensions, as in:

#define _GNU_SOURCE
#include <stdio.h>

// ...

int score = calculate_score(matrix);

char* score_str;
if (asprintf(&score_str, "Score: %d", score) < 0) {
    perror("asprintf");
    exit(EXIT_FAILURE);
}

// ...

draw_text_white(gRenderer,font,score_str,fillRect);
free(score_str);

Of course, you could always just use snprintf with a fixed size buffer and just truncate when things get too large.

\$\endgroup\$
1
  • \$\begingroup\$ I will take clang-format into consideration next time I write something. It seems most of the issues can be prevented just by using a better build system/IDE. Also, setting buffer size at runtime is definitely the way to go here. Thank you! \$\endgroup\$
    – Gnik
    Commented Dec 28, 2018 at 11:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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