patterncMinor
A function that moves a tile in the game of fifteen
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?
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
It will read the value of
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):
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
1will now have the value0for 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 fromboard[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.