Testing whether a chess move puts the king in check

Question

This is my first time back in C++ after a year of Java and essay-writing (and/or crying).

My current progress is, all by-definition moves implemented, including castling, en-passant, draw by threefold-repetition, and draw by 50 move rule (which I haven't tested yet). I am currently working on kings in check/next move gets king out of check.

I would like some feedback on whether this is good or not.

To test whether a king is in check, I would simulate the move and see if the opponent controls the square your king is on.

If that sounds like a sound idea, my current code (except output statements) is here:

bool simulate_move(std::vector<std::vector<Piece *> > board, int from_x, int from_y, int to_x, int to_y, bool turn) {
    std::vector<Point> white_control;
    std::vector<Point> black_control;
    King *white_king = NULL;
    King *black_king = NULL;

    move_piece(board, from_x, from_y, to_x, to_y);

    for (int i = 0; i < BOARD_SIZE; i++) {
        for (int j = 0; j < BOARD_SIZE; j++) {
            if (board[i][j] != NULL) {
                board[i][j]->get_controlled_squares(board[i][j]->isWhite() ? white_control : black_control);
                if (dynamic_cast<King *>(board[i][j]) != NULL) {
                    if (board[i][j]->isWhite()) {
                        white_king = dynamic_cast<King *>(board[i][j]);
                    }
                    else {
                        black_king = dynamic_cast<King *>(board[i][j]);
                    }
                }
            }
        }
    }

    if (turn) {
        if (vector_contains_point(black_control, white_king->get_x_position(), white_king->get_y_position())) {
            return false;
        }
    }
    else {
        if (vector_contains_point(white_control, black_king->get_x_position(), black_king->get_y_position())) {
            return false;
        }
    }

    return true;
}

I am testing with the moveset e2-e4 d7-d5 (B)f1-b5(+) c7-c6. According to the output, any move fails the simulation (king is in check).

Also, my bishop movement logic is:

void Bishop::get_controlled_squares(std::vector<Point>& point_list) {
    bool stop_up_left = false;
    bool stop_up_right = false;
    bool stop_down_left = false;
    bool stop_down_right = false;

    for (int i = 1; !stop_up_left || !stop_up_right || !stop_down_left || !stop_down_right; i++) {
        //upward left
        if (x_position - i < 0 || y_position - i < 0) {
            stop_up_left = true;
        }
        if (!stop_up_left) {
            if (board[x_position - i][y_position - i] != NULL) {
                stop_up_left = true;
            }
            point_list.push_back(Point(x_position - i, y_position - i));
        }

        //up right
        if (x_position + i > 7 || y_position - i < 0) {
            stop_up_right = true;
        }
        if (!stop_up_right) {
            if (board[x_position + i][y_position - i] != NULL) {
                stop_up_right = true;
            }
            point_list.push_back(Point(x_position + i, y_position - i));
        }

        //down left
        if (x_position - i < 0 || y_position + i > 7) {
            stop_down_left = true;
        }
        if (!stop_down_left) {
            if (board[x_position - i][y_position + i] != NULL) {
                stop_down_left = true;
            }
            point_list.push_back(Point(x_position - i, y_position + i));
        }

        //down right
        if (x_position + i > 7 || y_position + i > 7) {
            stop_down_right = true;
        }
        if (!stop_down_right) {
            if (board[x_position + i][y_position + i] != NULL) {
                stop_down_right = true;
            }
            point_list.push_back(Point(x_position + i, y_position + i));
        }
    }
}

Other thing: does anyone have a way to make the code look nicer?

Also, the board is as follows (left and bottom are chess-algebraic, top and right are array index):

  0 1 2 3 4 5 6 7
8 r n b q k b n r 0
7 p p p p p p p p 1
6                 2
5                 3
4                 4
3                 5
2 P P P P P P P P 6
1 R N B Q K B N R 7
  A B C D E F G H

GitHub

Answer 1

For the simulate move function: it is unclear if it operates on the real board, or on a copy of it. If it modifies the real board, it should undo its effects at the end.

About looking up the kings in that super-complicated for-if consturct: according to the other code snippet, a bishop knows its own location. I would assume this also applies to the kings, and if you could access them directly, their location would not be something to look up. I would suggest an array with dedicated location of the pieces, so you could both access the pieces directly, and also iterate over the array if necessary.

I think the controlled squares could be collected in an array mirroring the board itself: when "ranged" pieces are relatively free, those vectors will grow large (also because of repeats), while a board-sized array has fixed size and constant-time access. It could be boolean, storing control by one player, or with 4 possible values it could encode control by both players.

For simplifying a couple things, including the bishop code too: in chess-programming there is a common trick of storing a 12*12 (first iteration) board, having a border of 2 "occupied" squares in every direction (2 because of the knight). This way the abundant checks for coordinates running out of the board can be spared, instead one will inevitably encounter a border "piece" when trying to index outside the playfield. The second iteration is having this trick taken forward using linear addressing of the table (instead of having a 2D array and 2 coordinates), then a 10x12 array is enough, as a single 2-square wide border catches both horizontal cases (over and "underflowing" a row).

Answer 2

King *white_king = NULL;
King *black_king = NULL;

Don’t use the NULL macro. Forget it ever existed! In this case, your initializer is nullptr.

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

---

for (int i = 0; i < BOARD_SIZE; i++) {
    for (int j = 0; j < BOARD_SIZE; j++) {

⧺ES.9 Avoid ALL_CAPS names. Such names are commonly used for macros.

---

if (dynamic_cast<King *>(board[i][j]) != NULL) {

Don’t do explicit tests against nullptr. Use the contextual boolean value as a truth test. This is more important when you start using smart pointers — in general, the object itself of any type answers “are you OK to use”? in whatever means is appropriate.

---

I notice you have a lot of duplication in the function.

    if (board[i][j]->isWhite()) {
        white_king = dynamic_cast<King *>(board[i][j]);
    }
    else {
        black_king = dynamic_cast<King *>(board[i][j]);
    }

is identical except for one identifier name, and

if (turn) {
    if (vector_contains_point(black_control, white_king->get_x_position(), white_king->get_y_position())) {
        return false;
    }
}
else {
    if (vector_contains_point(white_control, black_king->get_x_position(), black_king->get_y_position())) {
        return false;
    }
}

is the same except for the variable names all changing between black and white.

Instead of separately named variables for black and white things, make an indexible collection.

E.g.

color= board[i][j]->get_color();
king[color] = dynamic_cast<King *>(board[i][j]);

and

color = turn ? black : white;
if (vector_contains_point(control[color], king[color]->get_x_position(), king[color]->get_y_position())) {
    return false;
}

---

The top part of the function uses board[i][j] a lot. In fact, that is the only value of the board that is used at all.

So you should write this as a range-for loop and forget about working with i, j, and loop bounds manually!

for (auto& row : board) {
    for (auto& square : row) {
        if (!square)  continue;  // nothing to do here         ※1
        const auto color = square->get_color();             // ※2
        control[color] = square->get_controlled_squares();  // ※3
        if (auto k = dynamic_cast<King*>(board))            // ※4
            king[color] = k;
    }
}

※1

Rather than nesting the entire body of code another level deep, treat this as a pre-condition and write it this way. It is clearer and easier to read because you know at this spot that this test controls the applicability of the entire procedure here; with the nested code, you don’t know until you analyze more where the brace ends, whether there is more stuff that is not under the condition, or an else branch.

※2

Instead of a bool is_white(), use an enum that states black or white. This enum is used to make arrays, as explained above, so you have a matching variable in each color. You refer to this result twice, so check once and save it!

※3

Don’t use “out” parameters. Return values (⧺F.20).

I do note, however, that you were keeping the difference to just the variable used, rather than duplicating the entire line. Think that way in general!

※4

You are dynamic casting, throwing away the result, and doing it again! The declare/assign/test form of the if is meant for exactly this purpose, and first explained with dynamic_cast in particular! Note that the variable k is only in scope where it is OK to use it.

Answer 3

Other thing: does anyone have a way to make the code look nicer?

1) Don’t write the same code 4 times.

2) Don’t keep repeating the x,y ± i (three times) in each block.

Here’s my take on it:

for (int i ⋯
    check (stop_up_left,    x_position-1, y_position-1, point_list);
    check (stop_up_right,   x_position+1, y_position-1, point_list);
    check (stop_down_left,  x_position-1, y_position+1, point_list);
    check (stop_down_right, x_position+1, y_position+1, point_list);
}

And you have a helper function:

void check (bool& stop, x,y, std::vector<Point>& point_list)
{
    if (stop) return;
    if (x<0 || x>7 || y<0 || y>7) {
        stop= true;
        return;
    }
    if (board[x][y]) stop= true;
    point_list.push_back({x,y});
}

Note that the check function knows nothing about the movement — it simply stops if it is blocked, recording the points up to then. It should work for all the pieces (for knight, pass a dummy stop and ignore it).

---

So, I sense a theme: Don’t Repeat Yourself (DRY). Keep this in mind at all levels of abstraction.

Think about a task in terms of essential steps on a high level, and repeat. That is opposed to immediately jumping to the fine minute details of the implementation. That will help you structure the code in a Top-Down Decomposition, and help you see abstractions at every level in which they exist.

How do you find the controlled squares? Answer 1 jumps right into minutia — look at the next square up-left, check it, etc.

Answer 2 takes it down one level of detail: Check in each direction of movement, stopping when it hits something.

Then, “check in some direction” is to write 4 calls to “check one” in a loop.

See? Understand the algorithm at steps of detail, and make the code follow that structure as well.