# Testing whether a chess move puts the king in check

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

• Ok, I found the solution to the simulation failing problem. I would still like a cleanup for the bishop thing (which can apply to the rook and don't get started on the queen (plz)). Oh and at least I get to sleep now even though it's 5:30 AM here. – sudomeacat Oct 25 '17 at 12:23

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).

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.

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.