patterncMinor
Game of Fifteen - A Sliding Puzzle Game
Viewed 0 times
fifteenslidinggamepuzzle
Problem
Update: I addressed the changes and created an updated post.
I'm reviewing C by working through the edX CS50 problem sets. This task was to implement Game of Fifteen aka 15 Puzzle. Project details here.
Implemented as an interactive console game. The board is initialized with "tiles" as descending values with the objective being to reorder as ascending by swapping the empty tile (represented as
Note that there's a dependency on CS50 Library.
I'd really appreciate feedback; the good the bad and the ugly. I have a few things in mind:
I implemented
fifteen.c
```
/**
* Implements Game of Fifteen (generalized to dim x dim).
*
* Usage: fifteen d
*
* whereby the board's dimensions are to be d x d,
* where d must be in [DIM_MIN,DIM_MAX]
*/
#include
#include
#include
#include
#include "fifteen.h"
// hopefully shut up compiler about usleep()
#define _XOPEN_SOURCE (500)
#define _BSD_SOURCE
#define DIM_MIN (3)
#define DIM_MAX (9)
static int last_row;
static int last_col;
static int num_tiles;
static int
I'm reviewing C by working through the edX CS50 problem sets. This task was to implement Game of Fifteen aka 15 Puzzle. Project details here.
Implemented as an interactive console game. The board is initialized with "tiles" as descending values with the objective being to reorder as ascending by swapping the empty tile (represented as
_) with adjacent tiles. Valid movements are similar to a rook in chess (can't move diagonal) but can only swap tiles that are touching. Here's a demo:Note that there's a dependency on CS50 Library.
I'd really appreciate feedback; the good the bad and the ugly. I have a few things in mind:
- I'd like to add unit tests but I don't think it's possible without getting rid of all the global variables. Advice on how to make things more testable?
- I'm thinking the file is probably too big. How might you recommend splitting things up into other files? Part of the reluctance to do so was related to the confines of the CS50 project and getting their automated tests to pass. I think the global variables need refactored to do this as well.
- I was wondering about creating a Tile struct to store the index of each Tile rather than having to calculate index's so often. I'm unsure if that would be a good approach.
I implemented
get_index() as linear search rather than binary search because I figured that the tradeoff of having to sort the array nearly every time wouldn't be worth it. Does that sound reasonable?fifteen.c
```
/**
* Implements Game of Fifteen (generalized to dim x dim).
*
* Usage: fifteen d
*
* whereby the board's dimensions are to be d x d,
* where d must be in [DIM_MIN,DIM_MAX]
*/
#include
#include
#include
#include
#include "fifteen.h"
// hopefully shut up compiler about usleep()
#define _XOPEN_SOURCE (500)
#define _BSD_SOURCE
#define DIM_MIN (3)
#define DIM_MAX (9)
static int last_row;
static int last_col;
static int num_tiles;
static int
Solution
The GOOD, there are no MAGIC numbers, good job! The naming conventions of the constants and enums follow
the current coding standards. The code is quite readable which suggests it is maintainable.
Excellent use of comments! Most of the functions are good examples of coding.
Obsolete Functions
Prefer standard functions that are portable over obsolete functions.
StackOverflow Question.
At least the problem was documented:
Global Variabls
Avoid global variables whenever possible. The code already isolates them to the file using static, which is
good, but there should be other ways to implement this solution that avoid globals. A suggestion would be
to use a struct that defines the game and contains all the globals.
Pass the struct between the functions that need it, The struct should probably be defined in a header file
so that the display functions can use it as well (See section below).
Separate Display from Game Logic
Over time the Model View Controller (MVC) pattern has been developed. The Model in this case is the
game logic. The core game logic here could be reusable with multiple display mechanisms. In a proffesional
environment the functions for display such as
to allow for different display mechanisms (Windows, console, xbox, etc.). The use of multiple files
would call for a more complex build strategy such as Make, or CMake.
Function Complexity
The function
turned into functions/subroutines. The code to report the error usage can be moved into a function and
the code in the
This Programmers Question discusses when is it good to break up a function, and points to a good reference book.
the current coding standards. The code is quite readable which suggests it is maintainable.
Excellent use of comments! Most of the functions are good examples of coding.
Obsolete Functions
Prefer standard functions that are portable over obsolete functions.
usleep() is obsolete. See thisStackOverflow Question.
At least the problem was documented:
// hopefully shut up compiler about usleep()
#define _XOPEN_SOURCE (500)
#define _BSD_SOURCEGlobal Variabls
Avoid global variables whenever possible. The code already isolates them to the file using static, which is
good, but there should be other ways to implement this solution that avoid globals. A suggestion would be
to use a struct that defines the game and contains all the globals.
typedef struct
{
int last_row;
int last_col;
int num_tiles;
int empty_tile_index;
int dim;
int board[DIM_MAX][DIM_MAX];
} GameOf15_Data;Pass the struct between the functions that need it, The struct should probably be defined in a header file
so that the display functions can use it as well (See section below).
Separate Display from Game Logic
Over time the Model View Controller (MVC) pattern has been developed. The Model in this case is the
game logic. The core game logic here could be reusable with multiple display mechanisms. In a proffesional
environment the functions for display such as
clear(), greet() and draw() would be in other filesto allow for different display mechanisms (Windows, console, xbox, etc.). The use of multiple files
would call for a more complex build strategy such as Make, or CMake.
Function Complexity
The function
main() is too long and complex, there are at least 2 parts of this that could beturned into functions/subroutines. The code to report the error usage can be moved into a function and
the code in the
while (true) loop should be in a function possible called execute_game().This Programmers Question discusses when is it good to break up a function, and points to a good reference book.
Code Snippets
// hopefully shut up compiler about usleep()
#define _XOPEN_SOURCE (500)
#define _BSD_SOURCEtypedef struct
{
int last_row;
int last_col;
int num_tiles;
int empty_tile_index;
int dim;
int board[DIM_MAX][DIM_MAX];
} GameOf15_Data;Context
StackExchange Code Review Q#135593, answer score: 6
Revisions (0)
No revisions yet.