12
\$\begingroup\$

I'd like to preface this by saying firstly, my code is absolutely nowhere near complete, and secondly that I am essentially brand new to coding and this is my first real project.

I like to be thrown in the deep end and so have set myself the challenge of making a chess AI using machine learning. I'm still in the logic stage of the chess game itself and have had to rewrite much of it more times than I can count.

Before I get too deep into this, does anyone have any advice how I can improve and ensure the game I make now will be easily usable when I eventually get to the machine learning phase? Mainly I'm looking for techniques I could use to stop having so many nested loops and repetitive code etc. Hopefully you can see where I'm going with the provided code and any advice is helpful advice.


#include <iostream>
#include <map>

enum class PieceType {
  KING,
  QUEEN,
  PAWN,
  PAWN_PASSANT,
  BISHOP,
  ROOK,
  HORSE,
  EMPTY,
};

enum class PieceColour {
  WHITE,
  BLACK,
  NONE,
};

struct Piece {
  PieceType type;
  PieceColour colour;
  bool EnPassant = false;
  bool ExposedKing = false;
};

struct Move {
  int x1, x2, y1, y2;
};

class ChessBoard {

private:
  Piece board[8][8];

  std::map<PieceType, std::string> PieceTypeToString = {
      {PieceType::KING, "K"},   {PieceType::QUEEN, "Q"},
      {PieceType::BISHOP, "B"}, {PieceType::HORSE, "H"},
      {PieceType::PAWN, "P"},   {PieceType::ROOK, "R"},
      {PieceType::EMPTY, "X"},
  };

  std::map<PieceColour, std::string> PieceColourToString = {
      {PieceColour::WHITE, "W"},
      {PieceColour::BLACK, "B"},
      {PieceColour::NONE, "X"},
  };

public:
  ChessBoard() {
    for (int x = 0; x < 8; x++) {
      for (int y = 0; y < 8; y++) {
        PieceType type;
        PieceColour colour = (x < 2) ? PieceColour::WHITE : PieceColour::BLACK;

        switch (x) {
        case 0:
        case 7:
          if (y == 0 || y == 7) {
            type = PieceType::ROOK;
          } else if (y == 1 || y == 6) {
            type = PieceType::HORSE;
          } else if (y == 2 || y == 5) {
            type = PieceType::BISHOP;
          } else if (y == 3) {
            type = PieceType::QUEEN;
          } else if (y == 4) {
            type = PieceType::KING;
          }
          break;
        case 1:
        case 6:
          type = PieceType::PAWN;
          break;
        default:
          type = PieceType::EMPTY;
          colour = PieceColour::NONE;
        }

        board[x][y] = {type, colour};
      }
    }
  }

  void printboard() {
    for (int x = 0; x < 8; x++) {
      for (int y = 0; y < 8; y++) {

        std::cout << PieceTypeToString[board[x][y].type];
        std::cout << PieceColourToString[board[x][y].colour] << " ";
      }
      std::cout << std::endl;
    }
  }

  Piece getPieceAt(int x, int y) const { return board[x][y]; }

  void setPieceAt(int x, int y, const Piece &piece) { board[x][y] = piece; }

  void Promotion(int x, int y, PieceType PromoPiece) {
    board[x][y].type = PromoPiece;
  }

  Move LastMove;
};

bool Obstructions(int x1, int x2, int y1, int y2, ChessBoard chessBoard) {
  int x, y;
  Piece CheckSpace = chessBoard.getPieceAt(x, y);
  Piece source = chessBoard.getPieceAt(x1, y1);
  Piece destination = chessBoard.getPieceAt(x2, y2);
  int xSteps = x2 - x1;
  int ySteps = y2 - y1;
  bool Obstructed;
  // check for x obs
  // check for y obs
  switch (source.type, source.colour) {
  case (PieceType::PAWN, PieceColour::WHITE):
    for (int i = 0; i <= abs(xSteps); i++) {
      CheckSpace = chessBoard.getPieceAt(x1 + i, y1);
      if (CheckSpace.type != PieceType::EMPTY) {
        Obstructed = false;
      } else {
        Obstructed = true;
      }
    }
  }

  return Obstructed;
}

bool MoveCheck(int x1, int y1, int x2, int y2, ChessBoard &chessBoard) {
  Piece CheckSpace;
  Piece source = chessBoard.getPieceAt(x1, y1);
  Piece destination = chessBoard.getPieceAt(x2, y2);
  int xSteps = x2 - x1;
  int ySteps = y2 - y1;
  int xStepsABS = abs(xSteps);
  int yStepsABS = abs(ySteps);
  int xDirection;
  int yDirection;

  if (x2 < 0 || x2 > 7 || y2 < 0 || y2 > 7) {
    return false;
  }
  if (destination.colour == source.colour) {
    return false;
  }
  if (x1 < 0 || x2 > 7 || y1 < 0 || y1 > 7) {
    return false;
  }

  switch (source.type) {

  case PieceType::PAWN:
    if (!Obstructions(x1, x2, y1, y2, chessBoard)) {
      return false;
    }
    if (Obstructions(x1, x2, y1, y2, chessBoard)) {
      return true;
    }
    if ((source.colour == PieceColour::BLACK && x2 > x1) ||
        (source.colour == PieceColour::WHITE && x2 < x1)) {
      return false;
    }

    if ((destination.type == PieceType::EMPTY) &&
        (ySteps == 0)) { // PAWN STANDARD MOVES
      if (xStepsABS == 1) {
        return true; // PAWN MOVE 1 FORWARD
      }
      if (((xSteps == 2) && (source.colour == PieceColour::WHITE) &&
           (x1 == 1)) ||
          ((xSteps == -2) && (source.colour == PieceColour::BLACK) &&
           (x1 == 6))) {
        return true; // PAWN MOVE 2 FORWARD
      }
    }
    if (((xStepsABS == 1) && ((ySteps == 1) || (ySteps == -1))) &&
        (destination.colour != PieceColour::NONE)) {
      if (((source.colour == PieceColour::WHITE) &&
           (destination.colour != source.colour) && (x2 > x1)) ||
          ((source.colour == PieceColour::BLACK) &&
           (destination.type != PieceType::EMPTY) && (x2 < x1))) {
        return true; // PAWN TAKE MOVE
      }
    }
    if (((x2 == 7) && (source.colour == PieceColour::WHITE)) ||
        ((x2 == 0) && (source.colour == PieceColour::BLACK))) {
      std::cout << "promotion!!! Choose your upgrade (NOT FUNCTIONAL ATM)"
                << std::endl;
    }
    return false; // END OF PAWN LOGIC

    // case PieceType::ROOK:

  } // END OF SWITCH !!!!!!!

  return false; // END OF MOVE CHECK FUNCTION!!!!!!!!!!!!!!!!
}
void movePiece(ChessBoard &chessBoard) {

  // Get player move

  int x1, y1, x2, y2;
  Piece emptypiece;
  emptypiece.type = PieceType::EMPTY;
  emptypiece.colour = PieceColour::NONE;
  std::cout << "Enter your move (x,y)(x,y): " << std::endl;
  std::cin >> x1 >> y1 >> x2 >> y2;
  std::cout << "You move the piece at(" << x1 << "," << y1 << ") to (" << x2
            << "," << y2 << ")" << std::endl;

  if (!MoveCheck(x1, y1, x2, y2, chessBoard)) {
    std::cout << "INVALID MOVE" << std::endl;
    return;
  }

  else if (MoveCheck(x1, y1, x2, y2, chessBoard)) {
    Piece piece = chessBoard.getPieceAt(x1, y1);
    chessBoard.setPieceAt(x2, y2, piece);
    chessBoard.setPieceAt(x1, y1, emptypiece);
    return;
  }
};

int main() {

  ChessBoard chessBoard;
  chessBoard.printboard();
  while (true) {
    movePiece(chessBoard);
    chessBoard.printboard();
  }

  return 0;
};
\$\endgroup\$
1

3 Answers 3

11
\$\begingroup\$

What you call "horse" is normally known as a Knight in English.


Enable more compiler warnings. These will help you improve your code before you ask another human to read it:

g++-13 -std=c++23 -fconcepts -fPIC -gdwarf-4 -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wmissing-braces -Wconversion  -Wuseless-cast -Weffc++       288978.cpp    -o 288978
288978.cpp: In constructor ‘constexpr Piece::Piece()’:
288978.cpp:22:8: warning: ‘Piece::type’ should be initialized in the member initialization list [-Weffc++]
   22 | struct Piece {
      |        ^~~~~
288978.cpp:22:8: warning: ‘Piece::colour’ should be initialized in the member initialization list [-Weffc++]
288978.cpp: In constructor ‘ChessBoard::ChessBoard()’:
288978.cpp:52:16: note: synthesized method ‘constexpr Piece::Piece()’ first required here
   52 |   ChessBoard() {
      |                ^
288978.cpp:52:3: warning: ‘ChessBoard::LastMove’ should be initialized in the member initialization list [-Weffc++]
   52 |   ChessBoard() {
      |   ^~~~~~~~~~
288978.cpp: In function ‘bool Obstructions(int, int, int, int, ChessBoard)’:
288978.cpp:119:18: warning: left operand of comma operator has no effect [-Wunused-value]
  119 |   switch (source.type, source.colour) {
      |           ~~~~~~~^~~~
288978.cpp:120:20: warning: left operand of comma operator has no effect [-Wunused-value]
  120 |   case (PieceType::PAWN, PieceColour::WHITE):
      |         ~~~~~~~~~~~^~~~
288978.cpp:119:10: warning: enumeration value ‘BLACK’ not handled in switch [-Wswitch]
  119 |   switch (source.type, source.colour) {
      |          ^
288978.cpp:119:10: warning: enumeration value ‘NONE’ not handled in switch [-Wswitch]
288978.cpp:113:9: warning: variable ‘destination’ set but not used [-Wunused-but-set-variable]
  113 |   Piece destination = chessBoard.getPieceAt(x2, y2);
      |         ^~~~~~~~~~~
288978.cpp:115:7: warning: unused variable ‘ySteps’ [-Wunused-variable]
  115 |   int ySteps = y2 - y1;
      |       ^~~~~~
288978.cpp: In function ‘bool MoveCheck(int, int, int, int, ChessBoard&)’:
288978.cpp:155:10: warning: enumeration value ‘KING’ not handled in switch [-Wswitch]
  155 |   switch (source.type) {
      |          ^
288978.cpp:155:10: warning: enumeration value ‘QUEEN’ not handled in switch [-Wswitch]
288978.cpp:155:10: warning: enumeration value ‘PAWN_PASSANT’ not handled in switch [-Wswitch]
288978.cpp:155:10: warning: enumeration value ‘BISHOP’ not handled in switch [-Wswitch]
288978.cpp:155:10: warning: enumeration value ‘ROOK’ not handled in switch [-Wswitch]
288978.cpp:155:10: warning: enumeration value ‘HORSE’ not handled in switch [-Wswitch]
288978.cpp:155:10: warning: enumeration value ‘EMPTY’ not handled in switch [-Wswitch]
288978.cpp:141:7: warning: unused variable ‘yStepsABS’ [-Wunused-variable]
  141 |   int yStepsABS = abs(ySteps);
      |       ^~~~~~~~~
288978.cpp:142:7: warning: unused variable ‘xDirection’ [-Wunused-variable]
  142 |   int xDirection;
      |       ^~~~~~~~~~
288978.cpp:143:7: warning: unused variable ‘yDirection’ [-Wunused-variable]
  143 |   int yDirection;
      |       ^~~~~~~~~~
288978.cpp: In function ‘bool Obstructions(int, int, int, int, ChessBoard)’:
288978.cpp:111:43: warning: ‘x’ is used uninitialized [-Wuninitialized]
  111 |   Piece CheckSpace = chessBoard.getPieceAt(x, y);
      |                      ~~~~~~~~~~~~~~~~~~~~~^~~~~~
288978.cpp:110:7: note: ‘x’ was declared here
  110 |   int x, y;
      |       ^
288978.cpp:111:43: warning: ‘y’ is used uninitialized [-Wuninitialized]
  111 |   Piece CheckSpace = chessBoard.getPieceAt(x, y);
      |                      ~~~~~~~~~~~~~~~~~~~~~^~~~~~
288978.cpp:110:10: note: ‘y’ was declared here
  110 |   int x, y;
      |          ^

In particular, this highlights something that doesn't do what you seem to expect:

  switch (source.type, source.colour) {
  case (PieceType::PAWN, PieceColour::WHITE):

That's exactly equivalent to:

  switch (source.colour) {
  case (PieceColour::WHITE):

Since there's only a single case in this switch, we can replace with a simple if, and test both values:

  if (source.type == PieceType::PAWN &&
      source.colour == PieceColour::WHITE) {

It's not obvious why there's no similar logic for the black pawns.


Input handling is very fragile:

  std::cout << "Enter your move (x,y)(x,y): " << std::endl;
  std::cin >> x1 >> y1 >> x2 >> y2;
  std::cout << "You move the piece at(" << x1 << "," << y1 << ") to (" << x2
            << "," << y2 << ")" << std::endl;

If there's any error in input (either a stream error such as end of stream, or a format error such as entering a piece name instead of a number), then we need to either ignore until the next newline or give up entirely (as appropriate).

Because movePiece() doesn't return a value, we have no way to know whether a move was actually made, and the game will continue regardless. We'll want to fix that.


Initialising the board with the loops in the constructor works okay, but I think it would be simpler and clearer if we arrange for a default-constructed Piece to be empty (thus initialising the board all-empty) then use std::fill() (from <algorithm>) to populate the pawns; the remaining rows are probably better written as a plain assignment:

#include <algorithm>

struct Piece {
    PieceType type = PieceType::EMPTY;
    PieceColour colour = PieceColour::NONE;
    bool EnPassant = false;
    bool ExposedKing = false;
};

    ChessBoard()
    {
        static constexpr Piece homerow[8] = {
            { PieceType::ROOK, PieceColour::WHITE }, { PieceType::HORSE, PieceColour::WHITE },
            { PieceType::BISHOP, PieceColour::WHITE }, { PieceType::QUEEN, PieceColour::WHITE },
            { PieceType::KING, PieceColour::WHITE }, { PieceType::BISHOP, PieceColour::WHITE },
            { PieceType::HORSE, PieceColour::WHITE }, { PieceType::ROOK, PieceColour::WHITE }
        };
        std::ranges::copy(homerow, board[0]);
        std::ranges::transform(homerow, board[7],
                               [](Piece p){ p.colour = PieceColour::BLACK; return p; });

        std::ranges::fill(board[1], Piece{ PieceType::PAWN, PieceColour::WHITE });
        std::ranges::fill(board[6], Piece{ PieceType::PAWN, PieceColour::BLACK });
        return;
    }
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Top man thank you, more compiler warnings and constexpr are two things i had no clue existed. Definitely showed me some useful tips and broader ways of approaching problems. Looks like i also need to delve deeper into various c++ commands etc to improve my program efficiency and readability. \$\endgroup\$
    – Liam V
    Jan 18 at 11:45
  • \$\begingroup\$ No problem. I hope I've given you enough pointers to continue your learning. It's worth acquainting yourself with the standard algorithm library, as that can often save you from having to re-invent lots of functionality. \$\endgroup\$ Jan 18 at 14:14
5
\$\begingroup\$

On top of what others already mentioned...

Mixing Functionality & I/O

There's an organization issue: you are mixing functionality & I/O, making your code untestable. Since you mention that you are in the process of writing, I won't fault you for the lack of tests... but when you get to them, you'll realize it's going to be painful to make any automated test because of this mix.

That is:

  • You cannot create a board (in arbitrary position) then check whether a move is accepted or rejected.
  • You cannot check that the board display is correctly formatted.

You don't need much change for this to happen:

int main() {
    ChessBoard chessBoard;
    chessBoard.printboard(std::cout);

    while (true) {
        auto move = askForMove(std::cin);

        auto error = movePiece(move, chessBoard);

        if (error.has_value()) {
            std::cout << *error << '\n';
        }

        chessBoard.printboard(std::cout);
    }

    return 0;
}

Now, you can also create automated tests calling movePiece with hardcoded sets of moves, and checking whether they're allowed, as well as calling printboard (just print would be enough) with a std::stringstream to verify the board is correctly displayed.

I want out!

There's no termination condition for your program!

There should be multiple termination conditions:

  1. askForMove should not only ask for a move, but also ask whether the user wants to quit. It should thus become askForInput.
  2. movePiece should check for check mate situation.

Whose turn is it already?

You should, somehow, keep a counter of whose turn it is:

  • You should display it in askForMove.
  • You should forbid a player to move two pieces in succession.

Although... do be wary that castling requires moving two pieces. Ideally, you'd simply recognize castling based on the king's move, and move the rook appropriately.

Since a player may be wondering how to do so, though, you should make it explicit. Maybe in a Help menu?

\$\endgroup\$
5
  • \$\begingroup\$ thanks for the reply, main reason for UX being so crappy is because once i have the logic 100% sorted, im going straight into learning machine learning neural networks all that jazz to attempt to train an AI to play, by my reasoning, computers dont need good UX just plain old absolute instructions. No need for an ai to quit or know whos turn it is so long as it knows one move per colour. As for your explanation about mixing functionalities would you mind explaining a little more, i dont quite understand what you mean but could just be because i havent slept. \$\endgroup\$
    – Liam V
    Jan 20 at 14:53
  • 1
    \$\begingroup\$ @LiamV: Imagine you want to test that a sequence of moves leads to a check-mate. Ideally, you'd write a unit-test in which you create the board, then call movePiece(move, board) for each move, checking that all but the final moves do not terminate the game, and that the last move in does, in check-mate. You can't do that easily if movePiece asks for the move on stdin: to test with stdin/stdout you'd have to use a wrapper script, etc... messy. So you need to separate I/O from the rest. It'll help with AI integration too. \$\endgroup\$ Jan 20 at 15:12
  • \$\begingroup\$ @LiamV: Also, you'll need to know which player it is to play -- otherwise the AI may play 3 white moves in a row -- and you'll need to know whether the game is a draw, or a check-mate, etc... and tell the AI. The AI won't know by itself, it can only learn about it if you tell it. \$\endgroup\$ Jan 20 at 15:13
  • \$\begingroup\$ thanks for clarifying i see what you mean now and yes thinking back i definietly need to implement the rules you stated. I think first and foremost i need to grab myself a c++ book so i can understand the language, its functions and intricacies more deeply than i do right now before i jump into the more advanced stuff. Thanks for taking the time to reply. \$\endgroup\$
    – Liam V
    Jan 20 at 15:38
  • \$\begingroup\$ @LiamV: If you want a book, have a look at The Definitive C++ Book Guide & List answer on SO. Good recommendations for all levels. \$\endgroup\$ Jan 21 at 13:04
5
\$\begingroup\$

Toby already got most, especially the important stuff, but here are some more considerations (and a bit repeated):

  1. I suggest changing your coding so zero / all-bits-zero is an an unclaimed field. Zero/Value-initialization is more conveniently accessible and is expected to mean default/nothing where it makes sense. In addition, it is cheaper than any other. Pushing it off to -1 (or all negative) or in extreme cases the maximum is generally avoided, and there are reasons for choosing those over some random value somewhere in the middle.

  2. I'm not sure how you want to use the two bools, thus I cannot even say whether the first overlaps duplicates PAWN_PASSANT, though the naming is suggestive.

  3. It might be worthwhile to compress a Piece down to a single byte, or it might be a waste. Anyway, that decision should be confined to the class.

  4. Move is certainly not designed for storage, but either for simplicity, or maybe for plentiful memory possibly compute-bound operation.

  5. Using a std::map for a consecutive range of never-changed elements starting at zero is always a waste. Making them member-variables compounds it.

  6. A ChessBoard is big enough it shouldn't be copied without cause. A Piece on the other hand is a tiny trivial type, and a Move could be.

  7. I wonder why you don't use your Move in your interfaces.

  8. Avoid std::endl in favor of '\n'. Use std::flush if you actually need an explicit manual flush.

  9. Consider storing an initialized board as a static constant once it is a literal (has only literaltype members, and no special member function), and getting new ones by simple copy-initialization.

    consteval ChessBoard CreateInitialBoard() {
        ChessBoard result = {};
        // Init
        return result;
    }
    static const ChessBoard InitialPosition = CreateInitialBoard();
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.