4
\$\begingroup\$

I just finished implementing a Java 2 player CLI chess game, it involves player putting in commands (i.e "move e2 e4" to move or "attack d1 d5" to capture.). I am trying to follow OOP, SOLID and clean code principles as much as possible.

Initially I designed this with the idea of MVC with the current implementation trying to achieve that, although I am a bit unsure about it.

A lot of code ahead, I hope this is okay.

Here is a class diagram, I have excluded setters and getters: Class diagram

Piece class:

    public abstract class Piece {
      private final Color color;
      private final Set<MovementType> movementTypes = EnumSet.noneOf(MovementType.class);
      private Square position;
    
      protected Piece(Color color, Square position) {
        this.color = color;
        this.position = position;
      }
    
      public Color getColor() {
        return color;
      }
    
      public Square getPosition() {
        return position;
      }
    
      public void setPosition(Square position) {
        this.position = position;
      }
    
      public boolean canCapture(Piece capturedPiece) {
        if (capturedPiece.getPosition().equals(this.getPosition())) {
          return false;
        }
        if (!this.isMoveValid(capturedPiece.getPosition())) {
          return false;
        }
    
        if (capturedPiece.getColor() == this.getColor()) {
          throw new IllegalArgumentException("You cannot capture your own piece.");
        }
        return true;
      }
    
      public abstract boolean isMoveValid(Square futurePosition);
    
      public Set<MovementType> getMovementTypes() {
        return movementTypes;
      }
    
      public void addMovementType(MovementType type) {
        movementTypes.add(type);
      }
    
      @Override
      public String toString() {
        return "Piece{" + "color=" + color + ", getPosition()=" + getPosition() + '}';
      }
    }

Bishop class:

public class Bishop extends Piece {
  public Bishop(Color color, Square position) {
    super(color, position);
    addMovementType(MovementType.DIAGONAL);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    return this.getPosition().getXDelta(futurePosition)
        == this.getPosition().getYDelta(futurePosition);
  }

  @Override
  public String toString() {
    return "B";
  }
}

King class:

public class King extends Piece {
  private boolean hasBeenChecked;
  private boolean hasMoved;

  public King(Color color, Square position) {
    super(color, position);
    addMovementType(MovementType.DIAGONAL);
    addMovementType(MovementType.AXIS);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    int yPosDifference = this.getPosition().getYDelta(futurePosition);
    int xPosDifference = this.getPosition().getXDelta(futurePosition);
    if (yPosDifference > 1 || xPosDifference > 1) {
      return false;
    }

    if (!hasMoved) {
      hasMoved = true;
    }
    return true;
  }

  public boolean hasBeenChecked() {
    return hasBeenChecked;
  }

  public boolean hasMoved() {
    return hasMoved;
  }

  public void setHasBeenChecked(boolean hasBeenChecked) {
    this.hasBeenChecked = hasBeenChecked;
  }

  @Override
  public String toString() {
    return "K";
  }
}

Knight class:

public class Knight extends Piece {
  public Knight(Color color, Square position) {
    super(color, position);
    getMovementTypes().add(MovementType.L_SHAPE);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    int val =
        this.getPosition().getXDelta(futurePosition) * this.getPosition().getYDelta(futurePosition);
    return val == 2;
  }

  @Override
  public String toString() {
    return "N";
  }
}

Pawn class:

public class Pawn extends Piece {
  private boolean isPromoted = false;
  private boolean isEnPassant = false;

  public Pawn(Color color, Square position) {
    super(color, position);
    addMovementType(MovementType.AXIS);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    boolean isMovingBackwards =
        this.getColor() == Color.WHITE
            ? futurePosition.getY() < this.getPosition().getY()
            : futurePosition.getY() > this.getPosition().getY();
    if (isMovingBackwards) {
      return false;
    }

    int yPosDifference = this.getPosition().getYDelta(futurePosition);
    boolean isMoveValid;

    // Starting position, allow moving 2 squares unless the square in front of the pawn is occupied.
    boolean isOnStartingSquare =
        (this.getColor() == Color.WHITE && getPosition().getY() == 2)
            || (this.getColor() == Color.BLACK && getPosition().getY() == 7);

    if (isOnStartingSquare) {
      isMoveValid =
          futurePosition.getX() == getPosition().getX()
              && yPosDifference <= 2
              && yPosDifference > 0;
      isEnPassant = true;
    } else {
      isMoveValid = futurePosition.getX() == getPosition().getX() && yPosDifference == 1;
      isEnPassant = false;
    }

    boolean isOnLastSquare =
        ((this.getColor() == Color.WHITE && futurePosition.getY() == Board.COLUMNS)
            || (this.getColor() == Color.BLACK && futurePosition.getY() == 1));

    isPromoted = isMoveValid && isOnLastSquare;
    return isMoveValid;
  }

  @Override
  public boolean canCapture(Piece capturedPiece) {
    if (capturedPiece.getPosition().equals(this.getPosition())) {
      return false;
    }

    if (capturedPiece.getColor() == this.getColor()) {
      throw new IllegalArgumentException("You cannot capture your own piece.");
    }

    boolean isMovingBackwards =
        this.getColor() == Color.WHITE
            ? capturedPiece.getPosition().getY() < this.getPosition().getY()
            : capturedPiece.getPosition().getY() > this.getPosition().getY();
    if (isMovingBackwards) {
      return false;
    }

    int yPosDifference = this.getPosition().getYDelta(capturedPiece.getPosition());
    int xPosDifference = this.getPosition().getXDelta(capturedPiece.getPosition());

    return xPosDifference == 1 && yPosDifference == 1;
  }

  public boolean isPromoted() {
    return isPromoted;
  }

  public boolean isEnPassant() {
    return isEnPassant;
  }

  @Override
  public String toString() {
    return "P";
  }
}

The Pawn class overrides canCapture() since the pawn is the only piece that can capture differently than the others.

Queen class:

public class Queen extends Piece {
  public Queen(Color color, Square position) {
    super(color, position);
    addMovementType(MovementType.DIAGONAL);
    addMovementType(MovementType.AXIS);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    boolean isDiagonal =
        this.getPosition().getXDelta(futurePosition)
            == this.getPosition().getYDelta(futurePosition);
    boolean isHorizontal =
        this.getPosition().getY() == futurePosition.getY()
            && this.getPosition().getX() != futurePosition.getX();
    boolean isVertical =
        this.getPosition().getX() == futurePosition.getX()
            && this.getPosition().getY() != futurePosition.getY();
    return isVertical || isHorizontal || isDiagonal;
  }

  @Override
  public String toString() {
    return "Q";
  }
}

Rook class:

public class Rook extends Piece {
  public Rook(Color color, Square position) {
    super(color, position);
    addMovementType(MovementType.AXIS);
  }

  @Override
  public boolean isMoveValid(Square futurePosition) {
    boolean isXAxis =
        this.getPosition().getY() == futurePosition.getY()
            && this.getPosition().getX() != futurePosition.getX();
    boolean isYAxis =
        this.getPosition().getX() == futurePosition.getX()
            && this.getPosition().getY() != futurePosition.getY();
    return isXAxis || isYAxis;
  }

  @Override
  public String toString() {
    return "R";
  }
}

Square class:

public class Square {
  private final int x;
  private final int y;

  public Square(int x, int y) {
    this.x = x;
    this.y = y;
  }

  public Square(String position) {
    this.x = position.charAt(0) - 96;
    this.y = Character.getNumericValue(position.charAt(1));
  }

  public int getX() {
    return x;
  }

  public int getY() {
    return y;
  }

  public int getXDelta(Square other) {
    return Math.abs(this.getX() - other.getX());
  }

  public int getYDelta(Square other) {
    return Math.abs(this.getY() - other.getY());
  }

  @Override
  public boolean equals(Object obj) {
    if (this == obj) {
      return true;
    }
    if (obj == null || !(obj instanceof Square)) {
      return false;
    }
    Square square = (Square) obj;
    return x == square.getX() && y == square.getY();
  }

  @Override
  public int hashCode() {
    return Objects.hash(x, y);
  }

  @Override
  public String toString() {
    return "Square{" + "x=" + x + ", y=" + y + '}';
  }
}

For the board class my idea was that it is responsible for the the state of the pieces (knowing where their locations are) and handle any querying for pieces on the board.

Board class:

  public class Board {
  public static final int ROWS = 8;
  public static final int COLUMNS = 8;
  private final List<Piece> pieces = new ArrayList<>();

  public List<Piece> getPieces() {
    return pieces;
  }

  public Piece getPieceFromSquare(Square square) {
    return pieces.stream()
        .filter(s -> s.getPosition().equals(square))
        .findFirst()
        .orElseThrow(
            () -> {
              throw new IllegalArgumentException("Cannot find piece.");
            });
  }

  public void addPieceToBoard(Piece piece) {
    if (!isSquareOccupied(piece.getPosition())) pieces.add(piece);
    else {
      throw new IllegalArgumentException("Square is already occupied!");
    }
  }

  public void removePieceFromBoard(Piece piece) {
    pieces.remove(piece);
  }

  public King getKing(Color color) {
    return (King)
        pieces.stream()
            .filter(piece -> piece.getColor() == color && piece instanceof King)
            .findFirst()
            .orElseThrow(() -> new NoSuchElementException("Cannot find king."));
  }

  public boolean isInCheck(King king) {
    boolean inCheck =
        pieces.stream()
            .filter(piece -> piece.getColor() != king.getColor())
            .anyMatch(piece -> piece.canCapture(king) && isPathFree(piece, king.getPosition()));
    // If it becomes true, it should stay true.
    if (!king.hasBeenChecked() && inCheck) {
      king.setHasBeenChecked(true);
    }
    return inCheck;
  }

  public boolean areThereAnyValidMoves(Color color) {
    List<Piece> friendlyPieces =
        pieces.stream().filter(piece -> piece.getColor() == color).toList();

    // TODO: ugly and slow, find a better way (list of moves?).
    // Loop through all the squares, if the move is valid and the king isn't left in check after,
    // return true
    // if the square is occupied and the piece can capture it without leaving the king in check
    for (Piece piece : friendlyPieces) {
      for (int i = 1; i <= ROWS; i++) {
        for (int j = 1; j <= COLUMNS; j++) {
          Square currentSquare = new Square(i, j);
          try {
            boolean isACapture =
                isSquareOccupied(currentSquare)
                    && piece.canCapture(getPieceFromSquare(currentSquare))
                    && isPathFree(piece, currentSquare);
            Piece capturedPiece = null;
            if (isACapture) {
              capturedPiece = getPieceFromSquare(currentSquare);
              removePieceFromBoard(capturedPiece);
            } else {
              validateMove(piece, currentSquare);
            }
            boolean isInCheck = wouldMoveResultInCheck(piece, currentSquare);
            if (isACapture) {
              addPieceToBoard(capturedPiece);
            }
            if (!isInCheck) {
              return true;
            }
          } catch (Exception ignored) {
            /*Move is invalid, move to next square.*/
          }
        }
      }
    }
    return false;
  }

  private boolean wouldMoveResultInCheck(Piece piece, Square destinationSquare) {
    Square oldPosition = piece.getPosition();
    piece.setPosition(destinationSquare);
    boolean isInCheck = isInCheck(getKing(piece.getColor()));
    piece.setPosition(oldPosition);
    return isInCheck;
  }

  public void validateMove(Piece piece, Square end) {
    Square start = piece.getPosition();
    if (!isSquareInbounds(end)) {
      throw new IllegalArgumentException("Specified square is out of bounds.");
    }

    if (start.equals(end)) {
      throw new IllegalArgumentException("You cannot move to the same position.");
    }

    if (isSquareOccupied(end)) {
      Piece pieceOnEndSquare = getPieceFromSquare(end);
      if (pieceOnEndSquare.getColor() == piece.getColor()) {
        throw new IllegalArgumentException("There is a friendly piece on that square.");
      }
      throw new IllegalArgumentException(
          "A piece exists on that position (use the attack command to capture).");
    }

    if (piece instanceof Pawn) {
      int yPosDifference = piece.getPosition().getYDelta(end);
      int xPosDifference = piece.getPosition().getXDelta(end);
      if (xPosDifference == 1 && yPosDifference == 1) {
        throw new IllegalArgumentException("Use the attack command to capture using the pawn.");
      }
    }

    if (piece.isMoveValid(end)) {
      boolean canMove = isPathFree(piece, end);
      if (!canMove) {
        throw new IllegalArgumentException("There are pieces on the way.");
      }
    } else {
      throw new IllegalArgumentException(piece + " Illegal move.");
    }
  }


  public boolean isPathFree(Piece piece, Square destination) {
    boolean isThereAPiece = false;
    Square start = piece.getPosition();
    if (piece.getMovementTypes().contains(MovementType.DIAGONAL)) {
      try {
        isThereAPiece = isThereAPieceOnDiagonal(start, destination);
      } catch (Exception ignored) {
        // Only throws when a piece can move diagonally and on axis, the move isn't diagonal,
        // ignore.
      }
    }

    if (piece.getMovementTypes().contains(MovementType.AXIS)) {
      try {
        isThereAPiece = isThereAPieceOnAxis(start, destination);
      } catch (Exception ignored) {
        // Only throws when a piece can move diagonally and on axis, the move isn't axis, ignore.
      }
    }

    if (piece.getMovementTypes().contains(MovementType.L_SHAPE)) {
      isThereAPiece = false;
    }

    return !isThereAPiece;
  }

  private boolean isThereAPieceOnAxis(Square start, Square destination) {
    boolean isXAxis = start.getY() == destination.getY() && start.getX() != destination.getX();
    boolean isYAxis = start.getX() == destination.getX() && start.getY() != destination.getY();
    if (!isXAxis && !isYAxis) {
      throw new IllegalArgumentException("Positions are not on the same axis.");
    }

    int offset;
    int startingValue;
    int limit;
    if (isXAxis) {
      offset = destination.getX() < start.getX() ? -1 : 1;
      startingValue = start.getX();
      limit = destination.getX();
    } else {
      offset = destination.getY() < start.getY() ? -1 : 1;
      startingValue = start.getY();
      limit = destination.getY();
    }

    for (int position = startingValue + offset; position != limit; position += offset) {
      Square currentSquare =
          isXAxis ? new Square(position, start.getY()) : new Square(start.getX(), position);
      if (isSquareOccupied(currentSquare)) {
        return true;
      }
    }
    return false;
  }

  private boolean isThereAPieceOnDiagonal(Square start, Square destination) {
    int xPositionDifference = start.getXDelta(destination);
    int yPositionDifference = start.getYDelta(destination);
    // Check if diagonal
    if (xPositionDifference != yPositionDifference) {
      throw new IllegalArgumentException("Positions are not diagonal");
    }

    // Trying to move to the same square the piece is at.
    // No need to check for the x position, the two variables are equal.
    if (yPositionDifference == 0) {
      return false;
    }

    // Check for the direction of the move and decide offset
    int xOffset = destination.getX() < start.getX() ? -1 : 1;
    int yOffset = destination.getY() < start.getY() ? -1 : 1;

    int y = start.getY();
    for (int x = start.getX() + xOffset; x != destination.getX(); x += xOffset) {
      y += yOffset;
      if (this.isSquareOccupied(new Square(x, y))) {
        return true;
      }
    }
    return false;
  }

  public boolean isSquareOccupied(Square square) {
    if (square == null) {
      throw new NullPointerException("Square argument is null.");
    }
    return pieces.stream().anyMatch(piece -> piece.getPosition().equals(square));
  }

  // Used for testing.
  public void clear() {
    pieces.clear();
  }

  public boolean isSquareInbounds(Square square) {
    return (square.getX() <= ROWS && square.getY() <= COLUMNS)
        && (square.getX() > 0 && square.getY() > 0);
  }

  public void initializeBoard() {
    // Black pieces
    for (int i = 1; i <= COLUMNS; i++) {
      addPieceToBoard(new Pawn(Color.BLACK, new Square(i, 7)));
    }
    addPieceToBoard(new Rook(Color.BLACK, new Square("a8")));
    addPieceToBoard(new Knight(Color.BLACK, new Square("b8")));
    addPieceToBoard(new Bishop(Color.BLACK, new Square("c8")));
    addPieceToBoard(new Queen(Color.BLACK, new Square("d8")));
    addPieceToBoard(new King(Color.BLACK, new Square("e8")));
    addPieceToBoard(new Bishop(Color.BLACK, new Square("f8")));
    addPieceToBoard(new Knight(Color.BLACK, new Square("g8")));
    addPieceToBoard(new Rook(Color.BLACK, new Square("h8")));

    // White pieces
    for (int i = 1; i <= COLUMNS; i++) {
      addPieceToBoard(new Pawn(Color.WHITE, new Square(i, 2)));
    }
    addPieceToBoard(new Rook(Color.WHITE, new Square("a1")));
    addPieceToBoard(new Knight(Color.WHITE, new Square("b1")));
    addPieceToBoard(new Bishop(Color.WHITE, new Square("c1")));
    addPieceToBoard(new Queen(Color.WHITE, new Square("d1")));
    addPieceToBoard(new King(Color.WHITE, new Square("e1")));
    addPieceToBoard(new Bishop(Color.WHITE, new Square("f1")));
    addPieceToBoard(new Knight(Color.WHITE, new Square("g1")));
    addPieceToBoard(new Rook(Color.WHITE, new Square("h1")));
  }
}

As you can tell by the TODO I really dislike the areThereAnyValidMoves() method, it has nested loops, the logic is confusing, etc. at the same time I couldn't think of a better implementation.

Player class:

public class Player {
  private final Color color;
  private final List<Piece> capturedPieces = new ArrayList<>();
  private String name;
  private boolean isTurn;

  public Player(String name, Color color, boolean isTurn) {
    this.name = name;
    this.color = color;
    this.isTurn = isTurn;
  }

  public String getName() {
    return name;
  }

  public void setName(String name) {
    this.name = name;
  }

  public Color getColor() {
    return color;
  }

  public List<Piece> getCapturedPieces() {
    return capturedPieces;
  }

  public void addCapturedPiece(Piece piece) {
    capturedPieces.add(piece);
  }

  public boolean isTurn() {
    return isTurn;
  }

  public void setTurn(boolean turn) {
    isTurn = turn;
  }

  public void move(Piece piece, Square destinationSquare) {
    piece.setPosition(destinationSquare);
  }
}

I implemented the command pattern to execute commands that the user enters and a command factory to give some sort of encapsulation to the command classes.

ICommand interface:

public interface ICommand {
  void execute();

  void undo();
}

BoardCommand class:

public abstract class BoardCommand implements ICommand {
  protected final Piece piece;
  protected final Square startingSquare;
  protected final Square destinationSquare;
  protected final Board board;
  protected final Player player;

  protected BoardCommand(String command, Board board, Player player) {
    String[] commandArgs = command.split(" ");
    this.board = board;
    this.player = player;
    startingSquare = new Square(commandArgs[1]);
    destinationSquare = new Square(commandArgs[2]);
    piece = board.getPieceFromSquare(startingSquare);
  }

  // For commands that parse the command themselves.
  protected BoardCommand(Board board, Player player) {
    this.board = board;
    this.player = player;
    startingSquare = null;
    destinationSquare = null;
    piece = null;
  }
}

All other commands extend from this class, this is because all the three subclasses require these properties and adding them to the interface and switching it into an abstract class doesn't make sense since if we add new commands that do not need those properties they would violate the ISP.

MoveCommand class:

public class MoveCommand extends BoardCommand {

  protected MoveCommand(String command, Board board, Player player) {
    super(command, board, player);
  }

  @Override
  public void execute() {
    if (piece.getColor() != this.player.getColor()) {
      throw new IllegalArgumentException("The piece chosen was not your piece.");
    }

    board.validateMove(piece, destinationSquare);
    player.move(piece, destinationSquare);
  }

  @Override
  public void undo() {
    this.piece.setPosition(this.startingSquare);
  }
}

CastlingCommand class:

public class CastleCommand extends BoardCommand {

  private final int commandArgsLength;

  protected CastleCommand(String command, Board board, Player player) {
    super(board, player);
    commandArgsLength = command.split("-").length;
  }

  @Override
  public void execute() {
    if (commandArgsLength < 2) {
      throw new IllegalArgumentException(
          "Illegal castling. (use 'o-o' for king side, 'o-o-o' for queen side.");
    }
    King king = this.board.getKing(this.player.getColor());
    boolean isWhite = this.player.getColor() == Color.WHITE;

    // King side
    if (commandArgsLength == 2) {
      Square rookPosition = isWhite ? new Square("h1") : new Square("h8");
      Piece rook = board.getPieceFromSquare(rookPosition);
      if (canCastle(king, rook)) {
        Square kingNewPosition = isWhite ? new Square("g1") : new Square("g8");
        king.setPosition(kingNewPosition);

        Square rookNewPosition = isWhite ? new Square("f1") : new Square("f8");
        rook.setPosition(rookNewPosition);
      } else {
        throw new IllegalArgumentException("Cannot castle on king side.");
      }
    }

    // Queen side
    if (commandArgsLength == 3) {
      Square rookPosition = isWhite ? new Square("a1") : new Square("a8");
      Piece rook = board.getPieceFromSquare(rookPosition);
      if (canCastle(king, rook)) {
        Square kingNewPosition = isWhite ? new Square("c1") : new Square("c8");
        king.setPosition(kingNewPosition);

        Square rookNewPosition = isWhite ? new Square("d1") : new Square("d8");
        rook.setPosition(rookNewPosition);
      } else {
        throw new IllegalArgumentException("Cannot castle on queens side.");
      }
    }
  }

  private boolean canCastle(King king, Piece rook) {
    if (!(rook instanceof Rook)) {
      return false;
    }
    return !king.hasBeenChecked() && !king.hasMoved() && board.isPathFree(king, rook.getPosition());
  }

  @Override
  public void undo() {
    King king = this.board.getKing(this.player.getColor());
    boolean isWhite = this.player.getColor() == Color.WHITE;
    Square oldKingPosition = isWhite ? new Square("e1") : new Square("e8");
    if (commandArgsLength == 2) {
      Square oldRookPosition = isWhite ? new Square("h1") : new Square("h8");
      Square currentRookPosition = isWhite ? new Square("f1") : new Square("f8");

      board.getPieceFromSquare(currentRookPosition).setPosition(oldRookPosition);
      king.setPosition(oldKingPosition);
    }

    if (commandArgsLength == 3) {
      Square oldRookPosition = isWhite ? new Square("a1") : new Square("a8");
      Square currentRookPosition = isWhite ? new Square("d1") : new Square("d8");

      board.getPieceFromSquare(currentRookPosition).setPosition(oldRookPosition);
      king.setPosition(oldKingPosition);
    }
  }
}

The obvious issue with this is the too many ternary operators (isWhite ? ...), is there a way to make this code more flexible? Creating a method that generates those are not possible since there are different values for the squares.

AttackCommand class:

public class AttackCommand extends BoardCommand{
    protected AttackCommand(String command, Board board, Player player) {
        super(command, board, player);
    }
    @Override
    public void execute() {
        if (!this.board.isSquareInbounds(this.destinationSquare)) {
            throw new IllegalArgumentException("Specified square is out of bounds.");
        }
        //En passant edge case
        if (piece instanceof Pawn && !board.isSquareOccupied(destinationSquare)) {
            //Check the right and left side of the piece, if a pawn exists and is vulnerable to en passant, capture it.
            Square squareOnTheRightOfPiece = new Square(piece.getPosition().getX() + 1, piece.getPosition().getY());
            Square squareOnTheLeftOfPiece = new Square(piece.getPosition().getX() - 1, piece.getPosition().getY());
            Piece capturedPiece = null;
            if (board.isSquareOccupied(squareOnTheRightOfPiece)) {
                capturedPiece = board.getPieceFromSquare(squareOnTheRightOfPiece);
            } else if (board.isSquareOccupied(squareOnTheLeftOfPiece)) {
                capturedPiece = board.getPieceFromSquare(squareOnTheLeftOfPiece);
            } else {
                throw new IllegalArgumentException("Destination square is empty.");
            }
            if (capturedPiece != null && capturedPiece.getColor() == piece.getColor()) {
                throw new IllegalArgumentException("You cannot capture your own piece.");
            }
            if (capturedPiece instanceof Pawn capturedPawn && capturedPawn.isEnPassant()) {
                capture(capturedPiece);
            } else {
                throw new IllegalArgumentException("En passant is not applicable.");
            }
            return;
        }
        if (board.isSquareOccupied(destinationSquare)) {
            Piece capturedPiece = board.getPieceFromSquare(destinationSquare);
            if (piece.canCapture(capturedPiece) && board.isPathFree(piece, destinationSquare)) {
                capture(capturedPiece);
            } else {
                throw new IllegalArgumentException("Illegal capture.");
            }
        } else {
            throw new IllegalArgumentException("Destination square is empty.");
        }
    }
    private void capture(Piece capturedPiece) {
        board.removePieceFromBoard(capturedPiece);
        this.piece.setPosition(this.destinationSquare);
        this.player.addCapturedPiece(capturedPiece);
    }
    @Override
    public void undo() {
        List<Piece> playerCapturedPieces = this.player.getCapturedPieces();
        Piece revivedPiece = playerCapturedPieces.get(playerCapturedPieces.size() - 1);
        revivedPiece.setPosition(this.destinationSquare);
        piece.setPosition(this.startingSquare);
        this.board.addPieceToBoard(revivedPiece);
        this.player.getCapturedPieces().remove(revivedPiece);
    }
}

CommandFactory class:

public class CommandFactory {
  private final Board board;

  public CommandFactory(Board board) {
    this.board = board;
  }

  public ICommand createCommand(String command, Player player) {
    if (command.isBlank()) {
      throw new IllegalArgumentException("Command is blank.");
    }
    String[] commandArgs = command.split(" ");
    if (commandArgs.length < 2) {
      throw new IllegalArgumentException("Command arguments are missing.");
    }
    String commandType = commandArgs[0];
    if (commandType.equalsIgnoreCase("move")) {
      return new MoveCommand(command, board, player);
    } else if (commandType.equalsIgnoreCase("attack")) {
      return new AttackCommand(command, board, player);
    } else if (commandType.equalsIgnoreCase("castle")) {
      return new CastleCommand(command, board, player);
    } else {
      throw new IllegalArgumentException(commandType + " command not found.");
    }
  }
}

EngineFacade class:

public class EngineFacade {
    private final Board board;
    private final Player whitePlayer;
    private final Player blackPlayer;
    private final CommandFactory commandFactory;
    private ICommand lastCommand;
    public EngineFacade() {
        this.board = new Board();
        board.initializeBoard();
        this.whitePlayer = new Player("", Color.WHITE, true);
        this.blackPlayer = new Player("", Color.BLACK, false);
        this.commandFactory = new CommandFactory(board);
    }
    public void executeCommand(String commandString) {
        ICommand command = commandFactory.createCommand(commandString, getCurrentPlayerObject());
        command.execute();
        lastCommand = command;
    }
    public void undoCommand() {
        lastCommand.undo();
    }
    public boolean didAPawnPromote() {
        return board.getPieces().stream().anyMatch(piece -> piece instanceof Pawn pawn && pawn.isPromoted());
    }
    private Player getCurrentPlayerObject() {
        return whitePlayer.isTurn() ? whitePlayer : blackPlayer;
    }
    public String getCurrentPlayer() {
        return whitePlayer.isTurn() ? "White" : "Black";
    }

    public boolean isCurrentPlayerInCheck() {
        return board.isInCheck(board.getKing(getCurrentPlayerObject().getColor()));
    }

    public boolean isCheckmate() {
        Color currentPlayerColor = getCurrentPlayerObject().getColor();
        King king = board.getKing(currentPlayerColor);
        return board.isInCheck(king) && !board.areThereAnyValidMoves(currentPlayerColor);
    }

    public boolean isStalemate() {
        Color currentPlayerColor = getCurrentPlayerObject().getColor();
        King king = board.getKing(currentPlayerColor);
        return !board.isInCheck(king) && !board.areThereAnyValidMoves(currentPlayerColor);
    }

    public void setBlackPlayerName(String name) {
        this.blackPlayer.setName(name);
    }
    public void setWhitePlayerName(String name) {
        this.whitePlayer.setName(name);
    }

    public boolean isWhitePlayerTurn() {
        return whitePlayer.isTurn();
    }

    public void setWhitePlayerTurn(boolean turn) {
        this.whitePlayer.setTurn(turn);
    }

    public boolean isBlackPlayerTurn() {
        return blackPlayer.isTurn();
    }

    public void setBlackPlayerTurn(boolean turn) {
        this.blackPlayer.setTurn(turn);
    }

    public Board getBoard() {
        return board;
    }

    private Pawn getPromotedPawn() {
        return (Pawn) board.getPieces().stream().filter(piece -> piece instanceof Pawn pawn && pawn.isPromoted())
                .findFirst()
                .orElseThrow(() -> new NoSuchElementException("Cannot find promoted pawn."));
    }
    private void promotePawn(Pawn pawn, Piece desiredPiece)
    {
        board.removePieceFromBoard(pawn);
        board.addPieceToBoard(desiredPiece);
    }

    public void promote(int choice) {
        Pawn pawn = getPromotedPawn();
        switch (choice) {
            case 1 ->  promotePawn(pawn, new Queen(pawn.getColor(), pawn.getPosition()));
            case 2 ->  promotePawn(pawn, new Rook(pawn.getColor(), pawn.getPosition()));
            case 3 ->  promotePawn(pawn, new Bishop(pawn.getColor(), pawn.getPosition()));
            case 4 ->  promotePawn(pawn, new Knight(pawn.getColor(), pawn.getPosition()));
            default -> throw new IllegalArgumentException("Choice is not valid.");
        }
    }
}

I believe the getPromotedPawn(), didAPawnPromote() should be in the board class, not in EngineFacade class, but at the same time I feel like the board shouldn't know if a pawn promoted or not that's the responsibility of the "Game manager", i.e EngineFacade.

ChessGame class:

public class ChessGame {
  private final EngineFacade engineFacade;
  private final View view;

  public ChessGame() {
    this.engineFacade = new EngineFacade();
    this.view = new View();
  }

  public void start() {
    Scanner sc = new Scanner(System.in);
    initializePlayers(sc);
    while (!engineFacade.isCheckmate() && !engineFacade.isStalemate()) {
      try {
        view.draw(engineFacade.getBoard());
        System.out.println(engineFacade.getCurrentPlayer() + " turn!");
        if (engineFacade.isCurrentPlayerInCheck()) {
          System.out.println("Check! Protect the king.");
        }
        System.out.println("Enter a command:");
        engineFacade.executeCommand(sc.nextLine());
        if (engineFacade.didAPawnPromote()) {
          handlePromotion(sc);
        }
        // If player is still in check/made the king in check after executing the move, undo the
        // move.
        if (engineFacade.isCurrentPlayerInCheck()) {
          engineFacade.undoCommand();
          throw new IllegalArgumentException("King is in check, undoing last move...");
        }
      } catch (Exception e) {
        System.out.println(e.getMessage());
        System.out.println("Try again.");
        continue;
      }
      engineFacade.setWhitePlayerTurn(!engineFacade.isWhitePlayerTurn());
      engineFacade.setBlackPlayerTurn(!engineFacade.isBlackPlayerTurn());
    }
    // Switch the current player to the winner.
    engineFacade.setWhitePlayerTurn(!engineFacade.isWhitePlayerTurn());
    engineFacade.setBlackPlayerTurn(!engineFacade.isBlackPlayerTurn());
    System.out.println(engineFacade.getCurrentPlayer() + " Wins!!");
  }

  public void handlePromotion(Scanner sc) {
    System.out.println("PROMOTION!");
    System.out.println("Choose a piece to promote to:");
    System.out.println("1)Queen\n2)Rook\n3)Bishop\n4)Knight");
    int choice = sc.nextInt();
    try {
      engineFacade.promote(choice);
    } catch (Exception e) {
      System.out.println(e.getMessage());
      System.out.println("Please enter a valid number.");
      handlePromotion(sc);
    }
  }

  private void initializePlayers(Scanner sc) {
    System.out.println("Enter white player name: ");
    String whitePlayerName = sc.nextLine();
    System.out.println("Enter black player name: ");
    String blackPlayerName = sc.nextLine();
    engineFacade.setWhitePlayerName(whitePlayerName);
    engineFacade.setBlackPlayerName(blackPlayerName);
  }
}

This acts as a middle man between the user, EngineFacade and the View (code below) which draws the board.

View class:

  public void draw(Board board) {
    int rows = Board.ROWS;
    int columns = Board.COLUMNS;
    String borderColor = ANSI_RED;
    System.out.println(borderColor + "   ===================" + ANSI_RESET);
    for (int i = 1; i <= columns; i++) {
      System.out.print(((columns + 1) - i) + borderColor + " || " + ANSI_RESET);
      for (int j = 1; j <= rows; j++) {
        Square currentSquare = new Square(j, (columns + 1) - i);
        if (board.isSquareOccupied(currentSquare)) {
          printPiece(board.getPieceFromSquare(currentSquare));
        } else {
          System.out.print("- " + ANSI_RESET);
        }
      }
      System.out.println(borderColor + "||" + ANSI_RESET);
    }
    System.out.println(borderColor + "   ===================" + ANSI_RESET);
    System.out.println("\t\t a b c d e f g h");
  }

  private void printPiece(Piece piece) {
    if (piece.getColor() == Color.WHITE) {
      System.out.print(ANSI_CYAN + piece + ANSI_RESET);
    } else {
      System.out.print(ANSI_PURPLE + piece + ANSI_RESET);
    }
    System.out.print(" ");
  }
}

ANSI_* are constant variables that I excluded the definition of, they are used to give color to the console output.

I know this was an extremely long post, thanks in advance for any replies.

\$\endgroup\$
2
  • \$\begingroup\$ For efficiency, moves are usually generated through bit boards. Also some engines generate pseudolegal moves and then check legality during evaluation. Also if you want to support it, UCI protocol is designed to be MVC \$\endgroup\$
    – qwr
    Commented Aug 26, 2022 at 12:42
  • \$\begingroup\$ I am not trying to make an effective engine per se, it's more about learning OO design and principles, I chose not to go with the bitboards since they're not very readable for someone who does not know the concept, imo. \$\endgroup\$
    – Yoh
    Commented Aug 26, 2022 at 13:09

1 Answer 1

2
\$\begingroup\$

Pretty good code and good try to decompose. From my first glance, I want to point out those:

  • Enforce immutability more. Don't create setters for everything just so that it's "standard" in java (setters themselves are quite controversal pattern and imho even more often bad than not). For example your Piece has movement type, which I see as something immutable. For me the correct approach is definitely to pass this Set in constructor and remove any modificator method (ex. addMovementType) to make it more clear, this is something immutable and set on creation of the class. I would go even further and ensure, this set is truly immutable by some kind of mechanism like Collections.unmodifiableSet or using guava ImmutableSet to make this immutability even more obvious in the code.
  • Another example would be player and it's name. If you set it once in the constructor, you are possibly saving yourself a lot of headaches and trouble (but of course losing the possibility to rename player in the middle).
  • Naming is hard and your names could be a lot better. Especially methods.
  • Use setters less for changing state. Instead try to think about the flow and what you are trying to do. Instead of "setPosition" you can use "moveToPosition". Then it's more clear, what is going on. SetPosition just sounds like random data setting. Same for mentioned player. Use rename rather than vague setName, be more specific about your actions.
  • It looks like you are using exceptions for branching and sometimes you are swallowing them. Not sure, because it's confusing. Is it really necessary to do it that way? This smells bad.
  • Feels like the movement type should be regular class, that can handle movement itself rather than being just enum with bunch of conditions. I suggest you create different movement types, use common interface and then use those classes to handle the movement with cooperation of the Board class.
  • Square class represents a Position to me, not a square. You are even calling the property in the Piece class position. Renaming will make easier naming other methods too. For example getPieceFromSquare can be getPieceAt or getPieceAtPosition, which sound more natural and is less confusing.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the answer, regarding the setters, I believe setPosition fits better only because moveToPosition indicates that the piece moves itself on the board, rather the player through the move method actually moves it by updating its position, I hope I'm making sense. Regarding the exceptions, I swallow the exceptions twice, the movement/capturing methods throw an exception when the move is invalid and in those cases we're trying if a certain move works or not and if it doesn't work (i.e we caught an exception) we just test the next one. \$\endgroup\$
    – Yoh
    Commented Aug 25, 2022 at 15:46
  • \$\begingroup\$ Regarding the movement type being a regular class, what exactly do you imagine its responsibility be? I believe checking if a move is valid for a certain piece should be the piece's responsibility and checking if a path is free between one point and the other is the board's responsibility, what would be the responsibility of this new class that doesn't violate the other classes responsibilities? I agree with everything else though, thank you so much for the feedback. \$\endgroup\$
    – Yoh
    Commented Aug 25, 2022 at 15:49
  • \$\begingroup\$ If that makes more sense to do that kind of naming, by all means do it. Still cannot agree with using exceptions for branching and swallowing. That's just very unusual and confusing. I suggest use some regular return value (boolean or enum) to signal, if the movement is valid. I agree with movement responsibilities you mentioned. That doesn't mean those classes (board/piece) cannot delegate it to some more specific classes. In fact they should delegate as much as possible if you want to follow SRP. \$\endgroup\$
    – K.H.
    Commented Aug 25, 2022 at 20:29
  • \$\begingroup\$ Exceptions are very expensive for the runtime environment to generate and work with. They should not be used for normal flow of control. \$\endgroup\$
    – Eric Stein
    Commented Aug 26, 2022 at 1:59

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