Skip to main content
added 37 characters in body
Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

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

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

Since there's only a single case in this switch, we can replace with

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

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

Source Link
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

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

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

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;
    }