2048 with GUI in C
0x2048
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]='';
}
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;
}
c game gui sdl 2048
asked Dec 27 '18 at 8:14
Gnik
39016
add a comment |
0x2048
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]='';
}
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;
}
c game gui sdl 2048
asked Dec 27 '18 at 8:14
Gnik
39016
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
Dec 27 '18 at 19:57
1
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
2
@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
Dec 28 '18 at 1:58
add a comment |
0x2048
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]='';
}
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;
}
c game gui sdl 2048
asked Dec 27 '18 at 8:14
Gnik
39016
0x2048
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]='';
}
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;
}
c game gui sdl 2048
c game gui sdl 2048
asked Dec 27 '18 at 8:14
Gnik
39016
asked Dec 27 '18 at 8:14
Gnik
39016
edited Dec 27 '18 at 16:37
asked Dec 27 '18 at 8:14
Gnik
39016
asked Dec 27 '18 at 8:14
Gnik
39016
asked Dec 27 '18 at 8:14
Gnik
39016
39016
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
Dec 27 '18 at 19:57
1
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
2
@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
Dec 28 '18 at 1:58
add a comment |
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
Dec 27 '18 at 19:57
1
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
2
@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
Dec 28 '18 at 1:58
3
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
Dec 27 '18 at 19:57
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
Dec 27 '18 at 19:57
1
1
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
2
2
@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
Dec 28 '18 at 1:58
@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
Dec 28 '18 at 1:58
add a comment |
3 Answers
3
active
oldest
votes
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: %sn", 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: %sn", SDL_GetError());
return false;
}
*gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
if( gRenderer == NULL )
{
fprintf(stderr, "Renderer could not be created: %sn", 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();
}
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
Thank you for the excellent review! I didn't think about bitshifts orpow()'s runtime.SDL_GetErrorwas also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realizeexit(0/1)could be removed as well. It was a pleasant surprise. I selected thematrix[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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
@MarDev Sure. The first one will lead to aTYPE * matrix. The valid indices are now $0,1,ldots,text{N}^2-1$. This is very handy for resetting, as you only needmemset, but now you needmatrix[row + column * N]or similar. The latter leads toTYPE **matrix. We now have an indirection and must usematrix[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.
– Zeta
Dec 28 '18 at 9:10
add a comment |
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.
answered Dec 28 '18 at 16:31
Edward
46.3k377209
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?
– Gnik
Dec 29 '18 at 3:39
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
add a comment |
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.
answered Dec 28 '18 at 8:28
Alex Reinking
21829
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!
– Gnik
Dec 28 '18 at 11:09
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210408%2f2048-with-gui-in-c%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
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: %sn", 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: %sn", SDL_GetError());
return false;
}
*gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
if( gRenderer == NULL )
{
fprintf(stderr, "Renderer could not be created: %sn", 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();
}
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
Thank you for the excellent review! I didn't think about bitshifts orpow()'s runtime.SDL_GetErrorwas also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realizeexit(0/1)could be removed as well. It was a pleasant surprise. I selected thematrix[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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
@MarDev Sure. The first one will lead to aTYPE * matrix. The valid indices are now $0,1,ldots,text{N}^2-1$. This is very handy for resetting, as you only needmemset, but now you needmatrix[row + column * N]or similar. The latter leads toTYPE **matrix. We now have an indirection and must usematrix[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.
– Zeta
Dec 28 '18 at 9:10
add a comment |
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: %sn", 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: %sn", SDL_GetError());
return false;
}
*gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
if( gRenderer == NULL )
{
fprintf(stderr, "Renderer could not be created: %sn", 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();
}
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
Thank you for the excellent review! I didn't think about bitshifts orpow()'s runtime.SDL_GetErrorwas also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realizeexit(0/1)could be removed as well. It was a pleasant surprise. I selected thematrix[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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
@MarDev Sure. The first one will lead to aTYPE * matrix. The valid indices are now $0,1,ldots,text{N}^2-1$. This is very handy for resetting, as you only needmemset, but now you needmatrix[row + column * N]or similar. The latter leads toTYPE **matrix. We now have an indirection and must usematrix[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.
– Zeta
Dec 28 '18 at 9:10
add a comment |
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: %sn", 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: %sn", SDL_GetError());
return false;
}
*gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
if( gRenderer == NULL )
{
fprintf(stderr, "Renderer could not be created: %sn", 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();
}
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
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: %sn", 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: %sn", SDL_GetError());
return false;
}
*gRenderer = SDL_CreateRenderer( *gWindow, -1, SDL_RENDERER_ACCELERATED );
if( gRenderer == NULL )
{
fprintf(stderr, "Renderer could not be created: %sn", 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();
}
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
edited Dec 28 '18 at 13:59
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
answered Dec 27 '18 at 12:26
Zeta
15.1k23475
15.1k23475
Thank you for the excellent review! I didn't think about bitshifts orpow()'s runtime.SDL_GetErrorwas also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realizeexit(0/1)could be removed as well. It was a pleasant surprise. I selected thematrix[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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
@MarDev Sure. The first one will lead to aTYPE * matrix. The valid indices are now $0,1,ldots,text{N}^2-1$. This is very handy for resetting, as you only needmemset, but now you needmatrix[row + column * N]or similar. The latter leads toTYPE **matrix. We now have an indirection and must usematrix[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.
– Zeta
Dec 28 '18 at 9:10
add a comment |
Thank you for the excellent review! I didn't think about bitshifts orpow()'s runtime.SDL_GetErrorwas also a very helpful and specific comment. I was pretty sure I had removed all magic numbers. I didn't realizeexit(0/1)could be removed as well. It was a pleasant surprise. I selected thematrix[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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
@MarDev Sure. The first one will lead to aTYPE * matrix. The valid indices are now $0,1,ldots,text{N}^2-1$. This is very handy for resetting, as you only needmemset, but now you needmatrix[row + column * N]or similar. The latter leads toTYPE **matrix. We now have an indirection and must usematrix[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.
– Zeta
Dec 28 '18 at 9:10
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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 15:15
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
Dec 27 '18 at 22:00
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
Dec 27 '18 at 22:00
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
Dec 28 '18 at 2:19
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
Dec 28 '18 at 2:19
@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.– Zeta
Dec 28 '18 at 9:10
@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.– Zeta
Dec 28 '18 at 9:10
add a comment |
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.
answered Dec 28 '18 at 16:31
Edward
46.3k377209
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?
– Gnik
Dec 29 '18 at 3:39
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
add a comment |
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.
answered Dec 28 '18 at 16:31
Edward
46.3k377209
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?
– Gnik
Dec 29 '18 at 3:39
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
add a comment |
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.
answered Dec 28 '18 at 16:31
Edward
46.3k377209
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.
answered Dec 28 '18 at 16:31
Edward
46.3k377209
edited Dec 29 '18 at 4:04
answered Dec 28 '18 at 16:31
Edward
46.3k377209
answered Dec 28 '18 at 16:31
Edward
46.3k377209
answered Dec 28 '18 at 16:31
Edward
46.3k377209
46.3k377209
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?
– Gnik
Dec 29 '18 at 3:39
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
add a comment |
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?
– Gnik
Dec 29 '18 at 3:39
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
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!
– Gnik
Dec 29 '18 at 3:17
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!
– Gnik
Dec 29 '18 at 3:17
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 < >?– Gnik
Dec 29 '18 at 3:39
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 < >?– Gnik
Dec 29 '18 at 3:39
1
1
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
I've added to my answer to try to answer all of your questions. If anything's still not clear, please ask again.
– Edward
Dec 29 '18 at 4:04
add a comment |
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.
answered Dec 28 '18 at 8:28
Alex Reinking
21829
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!
– Gnik
Dec 28 '18 at 11:09
add a comment |
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.
answered Dec 28 '18 at 8:28
Alex Reinking
21829
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!
– Gnik
Dec 28 '18 at 11:09
add a comment |
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.
answered Dec 28 '18 at 8:28
Alex Reinking
21829
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.
answered Dec 28 '18 at 8:28
Alex Reinking
21829
answered Dec 28 '18 at 8:28
Alex Reinking
21829
answered Dec 28 '18 at 8:28
Alex Reinking
21829
answered Dec 28 '18 at 8:28
Alex Reinking
21829
21829
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!
– Gnik
Dec 28 '18 at 11:09
add a comment |
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!
– Gnik
Dec 28 '18 at 11:09
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!
– Gnik
Dec 28 '18 at 11:09
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!
– Gnik
Dec 28 '18 at 11:09
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210408%2f2048-with-gui-in-c%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
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
Dec 27 '18 at 19:57
1
Maybe irrelevant to your need, but can I make 2048 with javascript as frontend and python as backend?
– austingae
Dec 27 '18 at 20:53
2
@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
Dec 28 '18 at 1:58