HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Game of Fifteen - A Sliding Puzzle Game

Submitted by: @import:stackexchange-codereview··
0
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 _) 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. usleep() is obsolete. See this
StackOverflow Question.

At least the problem was documented:

// hopefully shut up compiler about usleep()
#define _XOPEN_SOURCE (500)
#define _BSD_SOURCE


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.

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 files
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 main() is too long and complex, there are at least 2 parts of this that could be
turned 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_SOURCE
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;

Context

StackExchange Code Review Q#135593, answer score: 6

Revisions (0)

No revisions yet.