9
\$\begingroup\$

I have this method to check if a move is possible in Reversi. The method works as it should, but it doesn't feel right for some reason. It feels very repetitive. I have tried to make it less repetitive by splitting it up in other methods, but that didn't make it any better.

public bool MovePossible(int y, int x)
{
    //check if position is in field
    if (y > 7 || x > 7 || y < 0 || x < 0) 
    { 
        return false; 
    }

    //check if spot is not occupied
    if(Bord[y, x] != Color.None)
    {
        return false;
    }

    //get color of current turn
    Color color = InTurn;

    //set oppesite color
    Color oppositeColor = Color.Black;
    if (color == Color.Black) oppositeColor = Color.White;

    //check: X, Y + 1
    if (y + 1 <= 7)
    {
        if(Bord[y + 1, x] == oppositeColor)
        {
            for (int i = y + 2; i <= 7; i++)
            {
                if(Bord[i, x] == color)
                {
                    return true;
                }
            }
        }
    }

    //check: X, Y - 1
    if (y - 1 >= 0)
    {
        if (Bord[y - 1, x] == oppositeColor)
        {
            for (int i = y - 2; i >= 0; i--)
            {
                if (Bord[i, x] == color)
                {
                    return true;
                }
            }
        }
    }

    //check: X + 1, Y
    if (x + 1 <= 7)
    {
        if (Bord[y, x + 1] == oppositeColor)
        {
            for (int i = x + 2; i <= 7; i++)
            {
                if (Bord[y, i] == color)
                {
                    return true;
                }
            }
        }
    }

    //check: X - 1, Y
    if (x - 1 >= 0)
    {
        if (Bord[y, x - 1] == oppositeColor)
        {
            for (int i = x - 2; i >= 0; i--)
            {
                if (Bord[y, i] == color)
                {
                    return true;
                }
            }
        }
    }

    //check: X + 1, Y + 1
    if (y + 1 <= 7 && x + 1 <= 7)
    {
        if (Bord[y + 1, x + 1] == oppositeColor)
        {
            var i = y + 2;
            var j = x + 2;

            while (i <= 7 && j <= 7)
            {
                if (Bord[i, j] == color)
                {
                    return true;
                }

                i++;
                j++;
            }
        }
    }

    //check: X - 1, Y - 1
    if (y - 1 >= 0 && x - 1 >= 0)
    {
        if (Bord[y - 1, x - 1] == oppositeColor)
        {
            var i = y - 2;
            var j = x - 2;

            while (i >= 0 && j >= 0)
            {
                if (Bord[i, j] == color)
                {
                    return true;
                }

                i--;
                j--;
            }
        }
    }

    //check: X - 1, Y + 1
    if (y + 1 <= 7 && x - 1 >= 0)
    {
        if (Bord[y + 1, x - 1] == oppositeColor)
        {
            var i = y + 2;
            var j = x - 2;

            while(i <= 7 && j >= 0)
            {
                if (Bord[i, j] == color)
                {
                    return true;
                }

                i++;
                j--;
            }
        }
    }

    //check: X + 1, Y - 1
    if (x + 1 <= 7 && y - 1 >= 0)
    {
        if (Bord[y - 1, x + 1] == oppositeColor)
        {
            var i = y - 2;
            var j = x + 2;

            while (i >= 0 && j <= 7)
            {
                if (Bord[i, j] == color)
                {
                    return true;
                }

                i--;
                j++;
            }
        }
    }

    return false;
}

I would like to get some feedback on my code and how I can improve on it being less repetitive.

EDIT:

What is the code supposed to do?

The method has to check if a disk can be placed on a grid. It is part of the game Othello (or Reversi). The method has to return true when:

  • Y and x are within the field (so between 0 and 7)
  • The spot cannot be occupied
  • Y and x has a neighbor with the opposite color. For example: I want to place a black disk on 3, 3, either one or more of the following coordinates has to be white: 2, 2, 2, 3, 2, 4, 3, 2, 3, 4, 4, 2, 4, 3, 4, 4.
  • The disk from the opposite color has to be surrounded.

An example:

         0 1 2 3 4 5 6 7

     0   0 0 0 0 0 0 0 0  
     1   0 0 0 0 0 0 0 0
     2   0 0 0 0 0 0 0 0
     3   0 0 0 2 1 0 0 0
     4   0 0 0 1 2 0 0 0
     5   0 0 0 0 0 0 0 0
     6   0 0 0 0 0 0 0 0
     7   0 0 0 0 0 0 0 0

I want to place a white(1) disk at Y: 2, X: 3. That's valid because the spot is empty, it's next to a black(2) disk and the black disk is now surrounded. When the disk is placed the grid looks like this:

         0 1 2 3 4 5 6 7

     0   0 0 0 0 0 0 0 0  
     1   0 0 0 0 0 0 0 0
     2   0 0 0 1 0 0 0 0
     3   0 0 0 2 1 0 0 0
     4   0 0 0 1 2 0 0 0
     5   0 0 0 0 0 0 0 0
     6   0 0 0 0 0 0 0 0
     7   0 0 0 0 0 0 0 0

Because the black disk is now surrounded by the white disks the disk becomes a white disk (the one at 3, 3), but changing the disk happens in another function and is not important for the question.

Another example:

         0 1 2 3 4 5 6 7

     0   0 0 0 0 0 0 0 0  
     1   0 0 0 0 0 0 0 0
     2   0 0 0 0 0 0 0 0
     3   0 0 0 2 2 2 0 0
     4   0 0 0 1 1 0 0 0
     5   0 0 0 0 1 0 0 0
     6   0 0 0 0 0 0 0 0
     7   0 0 0 0 0 0 0 0

Placing a white(1) disk at Y: 4, X: 5 is not valid. It is next to a black(2) disk, but the black disk won't be surrounded, so it's not valid. Y: 2, X: 2 would be a valid move because the black disk at Y: 3, X: 3 is surrounded by Y: 2, X: 2 and Y 4 and X: 4.

\$\endgroup\$
0

1 Answer 1

6
\$\begingroup\$

I haven't checked whether your logic following the //check: X, Y + 1 comments is correct, but it can be refactored as follows. You're basically checking 8 directions (N, NE, E, SE, S, SW, W, NW), each uniquely identified by a dx and a dy between -1 and 1. You need to exclude the case where dx and dy are both 0.

dx    -1 0 +1
dy -1 NW N NE
    0  W .  E
   +1 SW S SE

That would give code like this:

for (int dx = -1; dx <= 1; dx++) {
    for (int dy = -1; dy <= 1; dy++) {
        if (dx == 0 && dy == 0)
            continue;

        if (x + dx < 0 || x + dx > 7 || y + dy < 0 || y + dy > 7)
            continue;

        if (Bord[y + dy, x + dx] != oppositeColor)
            continue;

        int i = 2;
        while (i <= 7)
        {
            if (x + i * dx < 0 || x + i * dx > 7 || y + i * dy < 0 || y + i * dy > 7)
                break;
            if (Bord[y + i * dy, x + i * dx] == Color.None)
                break;
            if (Bord[y + i * dy, x + i * dx] == color)
            {
                return true;            
            }
            i++;
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Awesome! this looks way better and seems totally logical! \$\endgroup\$ Feb 6, 2020 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.