Sliding tile puzzle

Question

I'm making a sliding tile puzzle using C++, and have code that exchanges the blank tile for one adjacent to it.

Using user input, the program takes the input and puts it in the function which uses a switch statement to determine which direction the user wants to switch, then preforms the switch if applicable.

I was simply wondering if it's better coding to have 4 separate functions, one for each direction like this:

void puzzle::swapLeft()
{
                if (X == 0)
                {
                    return;
                }
                temp = grid[X - 1][Y]; //swapping positions
                grid[X - 1][Y] = grid[X][Y];
                grid[X][Y] = temp;
                X -= 1;
                return
} 

Or is my current code with its switch statement?

Note: X is where the empty tile's X position is and Y is empty tile's Y position.

void puzzle::swap(int d) //d where 1 = up, 2 = left, 3 = down, 4 = right
{
    int temp = 0;
    switch (d) //determines direction
    {
    case 1://swaps empty with upward tile
        if (Y == 0) //checks if null is on upper border, if true returns to prevent error
        {
            return;
        }
        temp = grid[X][Y - 1]; //swapping positions
        grid[X][Y - 1] = grid[X][Y];
        grid[X][Y] = temp;
        Y -= 1;//null tile's new position
        break;

    case 2://swaps empty with left tile
        if (X == 0)//checks if null is on left border, if true returns to prevent error
        {
            return;
        }
        temp = grid[X - 1][Y]; //swapping positions
        grid[X - 1][Y] = grid[X][Y];
        grid[X][Y] = temp;
        X -= 1;
        break;
    case 3: //swaps empty with downward tile
        if (Y == 3) //checks if null is on bottom border, if true returns to prevent error
        {
            return;
        }
        temp = grid[X][Y + 1]; //swapping positions
        grid[X][Y + 1] = grid[X][Y];
        grid[X][Y] = temp;
        Y += 1; //null tile's new position
        break;
    case 4: //swaps null with rightward tile
        if (X == 3) //checks if null is on right border, if true returns to prevent error
        {
            return;
        }
        temp = grid[X + 1][Y]; //swapping positions
        grid[X + 1][Y] = grid[X][Y];
        grid[X][Y] = temp;
        X += 1; //empty tile's new position
        break;
    }
return; 
}

Answer 1

self documenting code:

switch (d) //determines direction
{
    case 1: swapUp();    break;
    case 2: swapLeft();  break;
    case 3: swapDown();  break;
    case 4: swapRight(); break;
    default:
      throw 1;
}

void swapUp()     {swap(0, 1, -1, 0);}
void swapLeft()   {swap(1, 0, 0, -1);}
void swapDown()   {swap(0, -1, -1, 3);}
void swapRight()  {swap(-1, 0, 3, -1);}

void swap(int xDelta, int yDelta, int xCheck, int yCheck)
{
    //swaps empty with upward tile
    if ((Y == yCheck) || (X == xCheck)) //checks if null is on upper border, if true returns to prevent error
    {
        return;
    }
    temp = grid[X - xDelta][Y - yDelta]; //swapping positions
    grid[X - xDelta][Y - yDelta] = grid[X][Y];
    grid[X][Y] = temp;
    Y = Y - yDelta;//null tile's new position
    X = X - xDelta;
}

Identifier Names

It is traditional that Identifier names that are all caps (i.e. X and Y) are macros. So best to use mixed case or lowercase only names.

Answer 2

Additionally to the other comments I would recommend to use std::swap for better readability. You could replace the whole block

temp = grid[X][Y - 1]; //swapping positions
grid[X][Y - 1] = grid[X][Y];
grid[X][Y] = temp;

with a single line statement

std::swap(grid[X][Y], grid[X][Y - 1]);

Also you could spare your temp variable.

Answer 3

It is ABSOLUTELY better to use different functions! switch statements are powerful, but not to be used for this. They allow you to skip straight to the correct case without checking each one, but this is not the case in your current situation.

Its going to be more readable, more resuable, more expandable. Imagine if you had to edit the code to make one more change? You'd have to change it all over the place, without functions. And right now, you only have 4 cases, but imagine if later you had have 30 million cases... That's a lot of code.

So you might have to pull some "magic" to get that to work. Since the only different line is X += 1;, you have a few options. The easiest is probably

void puzzle::swapLeft(int exitNumber, int numCase) 
{
    if (X == exitNumber) return;

    temp = grid[X - 1][Y]; //swapping positions
    grid[X - 1][Y] = grid[X][Y];
    grid[X][Y] = temp;

    switch(numCase)
    {
    case 0: Y -= 1; break;
    case 1: Y += 1; break;
    case 2: X -= 1; break;
    case 3: X += 1; break;
    }
} 

Because the only part that needs switching is the numCase! Also, you don't need to end functions with return, it makes no difference (;