Return to Answer

EDIT regarding a bug in the promotion logic.

Try a test case:

@Test
public void whitePawnPromotionCaptureBlocked() {
        state.set(5, 1, WHITE_PAWN);
        state.set(5, 0, BLACK_BISHOP);
        state.set(6, 0, BLACK_PAWN);

        // In this position there is one move,
        // which is Queen right
        // Unless I am blind, this move is not generated.
}

• The methods related to InitialDoubleMove are misnamed. It took me a while to understand what their purpose is. Consider renaming them to setEnPassant, clearEnPassant, etc.

  Speaking of en passant, I don't see the benefit of having an entire array of them. It may occur on at most a single square. In fact, it may occur at a single file (the rank must be 3). Something along the lines

    if (y == 3) {
        if (enPassantFile == x - 1 || enPassantFile == x + 1) {
            enPassantCapture(x, enPassantFile, children);
        }
    }
  

  Perhaps a single byte (with 8 bit flags, corresponding to 8 files) is sufficient.

• Promotion is buggy. A test for a promotion with capture is done in addWhitePromotion - and it is too late: addWhitePromotion is called _only_ if a regular move promotes. For example, in a position W: e7, B: Ke8, Rg8the moveegQ` will not be generated.

  I recommend to check for the promotion after the move has been generated. Then the check is as simple as y == 1.

  BTW, you only promote to the Queen. It is not right for a realistic chess engine.

• expandImplWhitePawn is very hard to follow. First, the test for y > 0 shall be done once, at the very beginning, and throw if fails. Second, I recommend restructuring it a bit differently:

    if (getCellColor(x, y - 1) == CELL_COLOR_NONE) {
        // generate a regular move forward
        if (y == 6 && getCellColor(x, y - 2) == CELL_COLOR_NONE) {
            // generate a double move forward
        }
    }
    // Now generate possible captures
    // Now check for promotion
  

• It feels very uncomfortable that some parts of code test for getCellColor() == CELL_COLOR_NONE, and some directly test for state[][] == EMPTY.

• Please no magic numbers. Use WHITE_INITIAL_RANK = 1 and WHITE_EN_PASSANT_RANK = 3.

  Also, just for a chess player's peace of mind use rank, file instead of x, y.

• The methods related to InitialDoubleMove are misnamed. It took me a while to understand what their purpose is. Consider renaming them to setEnPassant, clearEnPassant, etc.

  Speaking of en passant, I don't see the benefit of having an entire array of them. It may occur on at most a single square. In fact, it may occur at a single file (the rank must be 3). Something along the lines

    if (y == 3) {
        if (enPassantFile == x - 1 || enPassantFile == x + 1) {
            enPassantCapture(x, enPassantFile, children);
        }
    }
  

  Perhaps a single byte (with 8 bit flags, corresponding to 8 files) is sufficient.

• Promotion is buggy. A test for a promotion with capture is done in addWhitePromotion - and it is too late: addWhitePromotion is called only if a regular move promotes. For example, in a position W: e7, B: Ke8, Rg8 the move egQ will not be generated.

  I recommend to check for the promotion after the move has been generated. Then the check is as simple as y == 1.

  BTW, you only promote to the Queen. It is not right for a realistic chess engine.

• expandImplWhitePawn is very hard to follow. First, the test for y > 0 shall be done once, at the very beginning, and throw if fails. Second, I recommend restructuring it a bit differently:

    if (getCellColor(x, y - 1) == CELL_COLOR_NONE) {
        // generate a regular move forward
        if (y == 6 && getCellColor(x, y - 2) == CELL_COLOR_NONE) {
            // generate a double move forward
        }
    }
    // Now generate possible captures
    // Now check for promotion
  

• It feels very uncomfortable that some parts of code test for getCellColor() == CELL_COLOR_NONE, and some directly test for state[][] == EMPTY.

• Please no magic numbers. Use WHITE_INITIAL_RANK = 1 and WHITE_EN_PASSANT_RANK = 3.

  Also, just for a chess player's peace of mind use rank, file instead of x, y.

• The methods related to InitialDoubleMove are misnamed. It took me a while to understand what their purpose is. Consider renaming them to setEnPassant, clearEnPassant, etc.

  Speaking of en passant, I don't see the benefit of having an entire array of them. It may occur on at most a single square. In fact, it may occur at a single file (the rank must be 3). Something along the lines

    if (y == 3) {
        if (enPassantFile == x - 1 || enPassantFile == x + 1) {
            enPassantCapture(x, enPassantFile, children);
        }
    }
  

  Perhaps a single byte (with 8 bit flags, corresponding to 8 files) is sufficient.

• Promotion is buggy. A test for a promotion with capture is done in addWhitePromotion - and it is too late: addWhitePromotion is called _only_ if a regular move promotes. For example, in a position W: e7, B: Ke8, Rg8the moveegQ` will not be generated.

  I recommend to check for the promotion after the move has been generated. Then the check is as simple as y == 1.

  BTW, you only promote to the Queen. It is not right for a realistic chess engine.

• expandImplWhitePawn is very hard to follow. First, the test for y > 0 shall be done once, at the very beginning, and throw if fails. Second, I recommend handling a double move a bit differently:

    if (getCellColor(x, y - 1) == CELL_COLOR_NONE) {
        // generate a regular move forward
        if (y == 6 && getCellColor(x, y - 2) == CELL_COLOR_NONE) {
            // generate a double move forward
        }
    }
    // Now generate possible captures
    // Now check for promotion
  

• It feels very uncomfortable that some parts of code test for getCellColor() == CELL_COLOR_NONE, and some directly test for state[][] == EMPTY.

• The methods related to InitialDoubleMove are misnamed. It took me a while to understand what their purpose is. Consider renaming them to setEnPassant, clearEnPassant, etc.

  Speaking of en passant, I don't see the benefit of having an entire array of them. It may occur on at most a single square. In fact, it may occur at a single file (the rank must be 3). Something along the lines

    if (y == 3) {
        if (enPassantFile == x - 1 || enPassantFile == x + 1) {
            enPassantCapture(x, enPassantFile, children);
        }
    }
  

  Perhaps a single byte (with 8 bit flags, corresponding to 8 files) is sufficient.

• Promotion is buggy. A test for a promotion with capture is done in addWhitePromotion - and it is too late: addWhitePromotion is called _only_ if a regular move promotes. For example, in a position W: e7, B: Ke8, Rg8the moveegQ` will not be generated.

  I recommend to check for the promotion after the move has been generated. Then the check is as simple as y == 1.

  BTW, you only promote to the Queen. It is not right for a realistic chess engine.

• expandImplWhitePawn is very hard to follow. First, the test for y > 0 shall be done once, at the very beginning, and throw if fails. Second, I recommend restructuring it a bit differently:

    if (getCellColor(x, y - 1) == CELL_COLOR_NONE) {
        // generate a regular move forward
        if (y == 6 && getCellColor(x, y - 2) == CELL_COLOR_NONE) {
            // generate a double move forward
        }
    }
    // Now generate possible captures
    // Now check for promotion
  

• It feels very uncomfortable that some parts of code test for getCellColor() == CELL_COLOR_NONE, and some directly test for state[][] == EMPTY.

• Please no magic numbers. Use WHITE_INITIAL_RANK = 1 and WHITE_EN_PASSANT_RANK = 3.

  Also, just for a chess player's peace of mind use rank, file instead of x, y.