Skip to main content
added 2138 characters in body
Source Link
Roland Illig
  • 21.9k
  • 2
  • 36
  • 83

Some more remarks, from top to bottom:

void Board::set(const size_t where, Square s) {
    if (where >= board_size)
        return;
    squares[where] = s;
}

Instead of returning, you should throw an exception since trying to modify the board outside of its borders is clearly a programming mistake on a 10x12 board. Same for Board::get.

I would inline the two constructors for Chess::Square.

Having const size_t parameters is not useful since size_t variables are always passed by value. At least in the declarations the const should be omitted. In the implementation it's acceptable, though.

In init_classic_board, the code for placing the pieces is so complicated (first place a queen, then replace that with a king) that you could also just have 16 equal lines, each for one piece. Or a little helper function taking a color and a list of 8 pieces, like this

place_pieces(91, Color::black,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);
fill_row(81, Color::black, Piece::pawn);
fill_row(71, Color::none, Piece::none);
fill_row(61, Color::none, Piece::none);
fill_row(51, Color::none, Piece::none);
fill_row(41, Color::none, Piece::none);
fill_row(31, Color::black, Piece::pawn);
place_pieces(21, Color::white,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);

Regarding the braces: whenever an inner for loop needs curly braces, I also put braces around the outer for loop. I only omit the braces for one-liners and block statements with a one-liner body statement.

Below the case Piece::rook_castle:, the if clause has wrong indentation. You should let your IDE format the code for you. This would also make the space between ){ consistent.

Before the case Piece::bishop:, there is a // fallthrough comment missing.

After the default:, the break has wrong indentation.

In read_move, the cin>>in must be enclosed in an if clause.

Some more remarks, from top to bottom:

void Board::set(const size_t where, Square s) {
    if (where >= board_size)
        return;
    squares[where] = s;
}

Instead of returning, you should throw an exception since trying to modify the board outside of its borders is clearly a programming mistake on a 10x12 board. Same for Board::get.

I would inline the two constructors for Chess::Square.

Having const size_t parameters is not useful since size_t variables are always passed by value. At least in the declarations the const should be omitted. In the implementation it's acceptable, though.

In init_classic_board, the code for placing the pieces is so complicated (first place a queen, then replace that with a king) that you could also just have 16 equal lines, each for one piece. Or a little helper function taking a color and a list of 8 pieces, like this

place_pieces(91, Color::black,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);
fill_row(81, Color::black, Piece::pawn);
fill_row(71, Color::none, Piece::none);
fill_row(61, Color::none, Piece::none);
fill_row(51, Color::none, Piece::none);
fill_row(41, Color::none, Piece::none);
fill_row(31, Color::black, Piece::pawn);
place_pieces(21, Color::white,
    Piece::rook, Piece::knight, Piece::bishop, Piece::queen,
    Piece::king, Piece::bishop, Piece::knight, Piece::rook);

Regarding the braces: whenever an inner for loop needs curly braces, I also put braces around the outer for loop. I only omit the braces for one-liners and block statements with a one-liner body statement.

Below the case Piece::rook_castle:, the if clause has wrong indentation. You should let your IDE format the code for you. This would also make the space between ){ consistent.

Before the case Piece::bishop:, there is a // fallthrough comment missing.

After the default:, the break has wrong indentation.

In read_move, the cin>>in must be enclosed in an if clause.

Source Link
Roland Illig
  • 21.9k
  • 2
  • 36
  • 83

Your code is easily understandable. There are several things that I like about it:

  • The use of demystified numbers to represent the board positions. I agree with you that there is nothing magical about them.
  • Your choice of the obvious, most simple names for all the things you operate upon, like king, pawn.

Things I would do different:

  • I would try to make the a1 square number 11 instead of 21, so that you can use more natural numbers (11 to 88) to refer to the squares on the board.
  • The board history should not be called ml; I dont understand that name.