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

A function that moves a tile in the game of fifteen

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fifteenthemovesfunctiongamethattile

Problem

I've created a a function that will move a tile that a user specifies within the game board.

The game board can be from 33 up to 99 and is filled with values 1 to d - 1.
This function is called within another function that keeps track of "space" array and the 2d "board"array.

This function works however, I feel that I might be using too many constants? How can I make this better?

/**
* If tile borders empty space, moves tile and returns true, else
* returns false. 
* 
* @param   int     tile    The tile the user wants to move
* @param   int[]   space   Space keeps track of where the space is on the board
* @return  bool    
*/
bool move(int tile, int space[])
{
  //check the tile to the left of the space tile so we can swap the space tile with the tile the user wanted
  if ( board[ space[0] ][ space[1] + 1 ] == tile ) {

    board[ space[0] ][ space[1] + 1 ] = 0;

    board[ space[0] ][ space[1] ] = tile;
    space[1]++;

    return true;

  //check the tile to the right of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] ][ space[1] - 1] == tile ) {

    board[ space[0] ][ space[1] - 1 ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[1]--;

    return true;

  //check the tile to the top of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] + 1 ][ space[1] ] == tile ) {

    board[ space[0] + 1 ][ space[1] ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[0]++;

    return true;

  //check the tile to the bottom of the space tile so we can swap the space tile with the tile the user selected
  }else if ( board[ space[0] - 1 ][ space[1] ] == tile ) {

    board[ space[0] - 1 ][ space[1] ] = 0;

    board[ space[0] ][space[1] ] = tile;
    space[0]--;

    return true;

  } else {
    //the tile does not border the space tile
    return false;
  }
}

Solution

Reading past array bounds

Each of your four checks has the possibility of reading past array bounds. Very bad things can happen when that happens. I will illustrate with an example:

Let board be defined as int board[3][3], and lettile = 1, space[0] = 2, space[1] = 2. When the code reaches this first check:

if ( board[ space[0] ][ space[1] + 1 ] == tile ) {

    board[ space[0] ][ space[1] + 1 ] = 0;

    board[ space[0] ][ space[1] ] = tile;
    space[1]++;

    return true;
  }


It will read the value of board[2][3], which is past the end of the board array. The memory location corresponding to board[2][3] is 4 bytes past the end of board, and will probably be occupied by some other global variable in your program. Suppose this memory location contains the value 1. In this case, you will swap the 1 with the 0 in board[2][2], and then you will set space[1] to 3 which is outside of your board dimensions. So now:

  • Whatever global variable held the value 1 will now have the value 0 for no reason.



  • The next time you call into this function, you will be reading even further out of bounds because space[1] is 3, which will cause you to read from board[2][4].



Simplify with loop

Instead of writing each of your four direction checks separately, you can combine them into a single loop. Here is how I would do that (with additional bounds checking as well to fix the first problem):

bool move(int tile, int space[])
{
    int dx[4] = {  0,  0,  1, -1 };
    int dy[4] = {  1, -1,  0,  0 };

    // Check in each of the four directions.
    for (int i = 0; i = BOARD_SIZE || y = BOARD_SIZE)
            continue;

        // If we find the tile, swap them with the empty space.
        if (board[x][y] == tile) {
            board[x][y] = 0;
            board[space[0]][space[1]] = tile;
            space[0] = x;
            space[1] = y;
            return true;
        }
    }
    return false;
}

Code Snippets

if ( board[ space[0] ][ space[1] + 1 ] == tile ) {

    board[ space[0] ][ space[1] + 1 ] = 0;

    board[ space[0] ][ space[1] ] = tile;
    space[1]++;

    return true;
  }
bool move(int tile, int space[])
{
    int dx[4] = {  0,  0,  1, -1 };
    int dy[4] = {  1, -1,  0,  0 };

    // Check in each of the four directions.
    for (int i = 0; i < 4; i++) {
        int x = space[0] + dx[i];
        int y = space[1] + dy[i];

        // If out of bounds, skip this direction.
        if (x < 0 || x >= BOARD_SIZE || y < 0 || y >= BOARD_SIZE)
            continue;

        // If we find the tile, swap them with the empty space.
        if (board[x][y] == tile) {
            board[x][y] = 0;
            board[space[0]][space[1]] = tile;
            space[0] = x;
            space[1] = y;
            return true;
        }
    }
    return false;
}

Context

StackExchange Code Review Q#156739, answer score: 4

Revisions (0)

No revisions yet.