Return to Revisions

Here are some things that may help you improve your code.

Don't const qualify POD return values

The code includes this member function within Board:

const int GetPiece(int index) const;

The second const is useful and appropriate but the first one is not. It says that the return value may not be altered by the caller which is obviously not correct for plain old data (POD) such as int and is probably ignored by the compiler anyway.

Use C++ limits

The code includes INT_MIN which is defined in the old C <limits.h> file (which is not #included). You could add that file, but better would be to use std::numeric_limits<int>::min() from <limits>.

Beware of partially constructed objects

The Board constructor is currently this:

Board::Board(std::string& fenCode) {
    fen = std::make_unique<FEN>(board, fenCode);
}

However, the problem is that board, which is passed to the fen constructor, is uninitialized. A more subtle problem is this line:

int heldPiece, heldSquare = -1;

Due to C++ (and C) syntax, heldPiece does not get initialized. This is one of the major reasons that the guidelines say to always initalize every object (see ES.20).

Don't pass objects needlessly

The Evaluate() and Minimax() functions are both member functions of Board and so they already have access to data member board without explicitly passing it as an argument.

Prefer std::array to plain arrays

In pretty much every way, std::array is better than old-style plain C arrays. So I'd suggest Board::board should be std::array<int, 64>. See SL.con.1 for details.

Rethink the the use of pointers

The fen object is currently owned by the Board object as a std::unique_ptr, but this isn't really necessary. Instead, I'd suggest that if you follow the suggestions above, you can make fen just a plain FEN fen; and the Board constructor could look like this:

Board::Board(std::string& fenCode) 
    : board{}
    , fen(board, fenCode)
{}

Remove spurious semicolons

The semicolon at the end of both versions of split in fen.cpp can and should be omitted.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 55, 62, 64, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "62" and then trying to determine if this particular 62 means that White can castle on the King's side or some other constant that happens to have the same value.

Evaluate whether an enum class is better expressed as a class

There are many places in the code where some operation is applied to a piece, but what exactly happens depends on whether it's a knight, bishop, queen, etc. For that reason, I'd suggest that a better approach may be to create a base Piece class and derive Knight, Bishop, etc. This would allow you to keep thing better organized and reduce a great deal of repetition from the code. The board could contain pointers to Piece and then invoking actions on each would automatically route to the correct routine, whether for evaluating value or determining legal moves.

Use appropriate data structures

The heldLegalMoves is declared to be a std::list<int>* but I see no reason it couldn't instead be a much more efficient std::vector<int> without the pointer.

Understand the API

There is a fundamental problem in the way you're trying to draw the board. The problem is that you've already created a renderer, and then the code attempts to call SDL_GetWindowSurface(). Worse, the return value (which is nullptr on my machine) is never checked and it is later freed. You can either have a renderer or use SDL_GetWindowSurface() but not both. See https://wiki.libsdl.org/SDL_GetWindowSurface

If you do that, Graphics::InitBoard() gets simpler, and you can completely remove boardTexture and all references to it too:

void Graphics::InitBoard() {
    SDL_Rect r{SCREEN_X, SCREEN_Y, SQUARE_SIZE, SQUARE_SIZE};
    constexpr std::array<std::array<std::uint8_t, 4>, 2> color{{
        {0x61, 0x81, 0x87, 0xFF},   // dark color
        {0xAF, 0xC3, 0xC7, 0xFF}    // light color
    }};
    bool light{true};
    for (int i = 0; i < 8; i++) {
        for (int j = 0; j < 8; ++j) {
            SDL_SetRenderDrawColor(renderer.get(), 
                color[light][0], color[light][1], color[light][2], color[light][3]
            );
            SDL_RenderFillRect(renderer.get(), &r); 
            r.x += SQUARE_SIZE;
            light = !light;
        }
        light = !light;
        r.y += SQUARE_SIZE;
        r.x = SCREEN_X;
    }
    SDL_SetRenderDrawColor(renderer.get(), 0, 0, 0, 255);
}

Think of the user

I was surprised to find out that I could manually move either the white or the black pieces. Maybe that's intentional? Being able to reverse or alter a move that my opponent just made might come in handy, but it's unexpected. Also, I understand this is a work in progress, but I was disappointed that castling (which you did mention) and pawn promotion (which you didn't) aren't yet implemented. Still, it was kind of fun!