1
\$\begingroup\$

I have this function that contains two similar if statements that is kind of hard to re-factor.

I am saying this because these two share a significant portion of duplicated code, but because of the part where it differs slightly, like board[startX + 1][startY - 1] and board[startX - 1][startY - 1], I find it difficult to re-factor it efficiently.

In other words, I can surely re-factor this by creating another function that contains the common parts, but I am afraid of creating a few more if blocks inside that function and making the code dirtier.

Any advice for refactoring this kind of code?

public boolean hasCapturableEnemy(Piece[][] board, int startX, int startY, int endX, int endY) {

    //If the Pawn belong to the Upper Team (Faces downward)
    if(board[startX][startY] != null && board[startX][startY].getTeam() == Player.UP) {
        //If a Piece exists on a diagonally adjacent tile, return true
        if(startX - endX == -1 && Math.abs(startY - endY) == 1) {
            if( (board[startX + 1][startY - 1] != null && board[startX + 1][startY - 1].getTeam() != Player.UP) || 
                    (board[startX + 1][startY + 1] != null && board[startX + 1][startY + 1].getTeam() != Player.UP)) {
                return true;
            }
        }
    } 

    if(board[startX][startY] != null && board[startX][startY].getTeam() == Player.DOWN) { 
        //If the Pawn belongs to the Down Team (Faces upward)

        if(startX - endX == 1 && Math.abs(startY - endY) == 1) {
            //If a Piece exists on a diagonally adjacent tile, return true
            if( (board[startX - 1][startY - 1] != null && board[startX - 1][startY - 1].getTeam() != Player.DOWN) || 
                    (board[startX - 1][startY + 1] != null && board[startX - 1][startY + 1].getTeam() != Player.DOWN)) {
                return true;
            }
        }
    }

    return false;

}
\$\endgroup\$
  • 1
    \$\begingroup\$ You would probably be better off doing a broader restructuring of your chess engine. I suggest that you post more of your code so that we may advise you better. \$\endgroup\$ – 200_success Sep 21 '17 at 23:40
  • \$\begingroup\$ First, you need to document what the function does and what the input values mean. For example, I am confused as to why you check two squares for enemy pieces and not just the one square denoted by (endX, endY). What does returning true from the function mean? \$\endgroup\$ – JS1 Sep 22 '17 at 2:39
  • 1
    \$\begingroup\$ I would add an integer direction which is -1 or +1 based on the player.. \$\endgroup\$ – RobAu Oct 24 '17 at 13:04
1
\$\begingroup\$

In other words, I can surely re-factor this by creating another function that contains the common parts, but I am afraid of creating a few more if blocks inside that function and making the code dirtier.

Au contraire. If your refactoring is increasing the code complexity, you're not doing it right.

The refactoring I did in this answer has reduced maximum nesting to a single level.

Maybe some quick-fire observations about why your code was so complex:

  • You were pushing all your calculations into your array index, which caused you to have to repeat the same value+1 and value-1 statements over and over.
  • As you already acknowledged, you had copy/pasted code and only made minor alterations (turning a +1 into a -1 due to the different player teams).
  • Your variables names weren't really all that clear, nor clearly used. Even after having refactored it, I still have no clue what endX and endY were used for.

There are two "types" of refactoring:

  • Simplifying code by changing it while not making any functional difference.

A simple example would be seeing if(myBool == true) and changing it to if(myBool). It makes no functional difference, and therefore can be safely refactored.

  • Revisiting the code and redesigning it, so you can optimize it with knowledge you have now (but didn't have at the time).

This often happens when a problem is complex at the beginning, but after developing a solution, it looks so much easier in hindsight.

We're going to be doing the second type of refactoring here. Which means we'll start from scratch and build the requirements as they come.


Assumptions:

  • Arrays are 0-indexed (it's been a while since I did Java, I think that's the case?)
  • We're using screen coordinates:

enter image description here


Method signature

We want:

  • A boolean that tells us whether the pawn can capture an enemy.

We need to supply:

  • The specific pawn
  • Access to the game board for retrieving information.

Your code:

public boolean hasCapturableEnemy(Piece[][] board, int startX, int startY, int endX, int endY)

I've been looking at it for a while, and I genuinely have no idea why you are supplying endX and endY. My suggestion:

public boolean hasCapturableEnemy(Piece[][] board, int pawnX, int pawnY)

1 - Input validation

Let's store the pawn in its own variable, for ease of use:

Piece pawn = board[pawnX][pawnY];

Does the referenced position actually contain a pawn? As it stands, you're only really checking for null, so I'll do the same:

if (pawn == null)
    return false;

Notice that I inverted the logic. If the pawn does not exist, then we immediately return false. This helps reduce nesting, and you won't need to indent the subsequent code.


2 - Defining the possible capture regions

To help you simplify this, let's look at the row calculation and the column calculation separately:

Columns are easy: given that a pawn is standing in column X, the capture regions can be found in columns X-1 and X+1.
Importantly, this is true for both the UP and DOWN players!

int captureColumnLeft = pawnX - 1;
int captureColumnRight = pawnX + 1;

When the pawn is on column 0 or 7, it will only have one capture region. But we'll get to that later.

Rows, however, are a bit more complex. This is the first time we need to observe the team that this pawn belongs to. Given that a pawn is standing on row Y, its capture regions are found in row Y+1 for the UP player, but in row Y-1 for the DOWN player.

int captureRow;

if(pawn.getTeam() == Player.UP)
    captureRow = pawnY + 1;
else
    captureRow = pawnY - 1;

We don't need to exclude fringe cases. A pawn can never be at the end of the board (for either player), since it would then turn into another piece.

Now we've very easily defined a given pawn's capture regions:

Piece captureRegionLeft = board[captureColumnLeft][captureRow];
Piece captureRegionRight = board[captureColumnRight][captureRow];

The next step is checking to see if there is a capturable piece on any of the capture regions. As the logic is exactly the same for both regions, we'll put this in a new method.

Before we get to that, let's close our current method:

return CanCaptureAt(captureRegionLeft, pawn) 
       || CanCaptureAt(captureRegionRight, pawn);

3 - Checking if a possible capture region contains a capturable piece

public boolean CanCaptureAt(Piece captureRegion, Piece pawn)
{

    // ...

}

The logic isn't all that hard:

  • If there is no piece in the capture region, we cannot capture anything in that region.

    if(captureRegion == null) return false;

  • If there is a piece in the capture region, it must be of the opposing team. In other words, a piece is capturable if the teams are different:

    return captureRegion.getTeam() != pawn.getTeam();


Combined:

public boolean hasCapturableEnemy(Piece[][] board, int pawnX, int pawnY)
{
    Piece pawn = board[pawnX][pawnY];

    if (pawn == null)
        return false;

    int captureColumnLeft = pawnX - 1;
    int captureColumnRight = pawnX + 1;

    int captureRow;

    if(pawn.getTeam() == Player.UP)
        captureRow = pawnY + 1;
    else
        captureRow = pawnY - 1;

    Piece captureRegionLeft = board[captureColumnLeft][captureRow];
    Piece captureRegionRight = board[captureColumnRight][captureRow];

    return CanCaptureAt(captureRegionLeft, pawn) 
        || CanCaptureAt(captureRegionRight, pawn);
}

public boolean CanCaptureAt(Piece captureRegion, Piece pawn)
{    
    if(captureRegion == null) 
        return false;

    return captureRegion.getTeam() != pawn.getTeam();    
}

Additionally

I said we still needed to exclude two fringe cases: if the pawn is on the leftmost column, it doesn't have a left capture region; and when it is on the rightmost column, it's doesn't have a right capture region.

This can be achieved by slightly reshuffling the code:

public boolean hasCapturableEnemy(Piece[][] board, int pawnX, int pawnY)
{
    Piece pawn = board[pawnX][pawnY];

    if (pawn == null)
        return false;

    int captureRow;

    if(pawn.getTeam() == Player.UP)
        captureRow = pawnY + 1;
    else
        captureRow = pawnY - 1;

    if(pawnX > 0) // == NOT in the leftmost column
    {
        int captureColumnLeft = pawnX - 1;
        Piece captureRegionLeft = board[captureColumnLeft][captureRow];

        if(CanCaptureAt(captureRegionLeft, pawn)) 
            return true;
    }

    if(pawnX < 7) // == NOT in the rightmost column
    {
        int captureColumnRight = pawnX + 1;
        Piece captureRegionRight = board[captureColumnRight][captureRow];

        if(CanCaptureAt(captureRegionRight, pawn)) 
            return true;
    }

    //Neither caption region registered a hit,
    //Therefore we cannot capture anything
    return false;
}

Footnotes

  • There is currently no check to see if the provided X/Y values are within the bounds of the board.
  • There is currently no check to see if the Piece at the provided X/Y coordinates is actually a pawn.
\$\endgroup\$
0
\$\begingroup\$

This is what I came up with:

I looked at the two innermost if statements and noted the differences:

  1. the operator in startX index (adding and substracting 1)
  2. the team compared against - it is the opposite team of board[startX][startY]

so I took these differences as arguments to the new method checkDiagonallyAdjacentTile() which contains the similar part of the if statement with the arguments specified in the correct places.

I also beautified the code a little: no need for parenthesis in && condition and broke down the two outer if statements, allowing for an else construct.

The end result is below

public boolean checkDiagonallyAdjacentTile(Piece[][] board, int addToStartX, Player playerTeam) {
    // if a Piece of opposite team exists on a diagonally adjacent tile, return true
    if (board[startX + addToStartX][startY - 1] != null && board[startX + addToStartX][startY - 1].getTeam() != playerTeam || 
        board[startX + addToStartX][startY + 1] != null && board[startX + addToStartX][startY + 1].getTeam() != playerTeam) {
           return true;
    }
    return false;
}


public boolean hasCapturableEnemy(Piece[][] board, int startX, int startY, int endX, int endY) {

    boolean hasCapturableEnemy = false;

    // if the Pawn belong to the Upper Team (Faces downward)
    if (board[startX][startY] != null) {
        if (board[startX][startY].getTeam() == Player.UP) {
            if (startX - endX == -1 && Math.abs(startY - endY) == 1) {
                hasCapturableEnemy = checkDiagonallyAdjacentTile(board, 1, board[startX][startY].getTeam());
            }
        } else {
            if (startX - endX == 1 && Math.abs(startY - endY) == 1) {
                hasCapturableEnemy = checkDiagonallyAdjacentTile(board, -1, board[startX][startY].getTeam());
            }
        }
    }

    return hasCapturableEnemy;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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