13
\$\begingroup\$

This is my very first project in java. I would love some feedback on my design. How the same implementation can be better written. Idioms, conventions, anything that comes to your mind. I hope I have not included a lot of code here. I thought it would be hard to review my code without the basic classes.

The GUI is not included here but if you would like to look at it then here is my repository.

I have got some specific questions about the implementation that I would love to have answered

1) The BoardManager.java class seems to be pretty large. Is there any way I can clean it up (divide it up maybe)? Something that bugs me is the implementation of isValidCastling() function and move() function. Is there any way to improve on these?

2) I have learned a convention of assigning a null value to all objects if they are empty/not present (If a square does not contain a piece, then null is assigned). Is there a better option? Due to this, I get a lot of NullPointerExceptions while debugging? Is this a design flaw or is this something I have to deal with?

PieceType.java

package pieces;

/**
 * @author gnik
 *
 */
public enum PieceType {
    KING("k"), KNIGHT("n"), ROOK("r"), QUEEN("q"), BISHOP("b"), PAWN("p");

    private String value;

    PieceType(String value) {
        this.value = value;

    }

    @Override
    public String toString() {
        return this.value;
    }
    public static PieceType fromString(String value){
        for (PieceType piece :PieceType.values()) {
            if (piece.value.equalsIgnoreCase(value)) {
              return piece;
            }
          }
          return null;
    }

}

PlayerType.java

package player;

/**
 * @author gnik
 *
 */

public enum PlayerType {
    WHITE("W"), BLACK("B");

    private String value;

    PlayerType(String value) {
        this.value = value;
    }

    @Override
    public String toString() {
        return value;
    }
}

Piece.java

package pieces;

import player.PlayerType;
import game.Coordinate;


/**
 * @author gnik
 *
 */
public abstract class Piece {
    private PieceType type;
    private PlayerType player;

    /**
     * Initialize a piece with a playerType.
     * @param player The player the piece belongs to 
     * @param type The piece type
     */
    public Piece(PlayerType player,PieceType type){
        this.type=type;
        this.player=player;
    }

    /* (non-Javadoc)
     * @see java.lang.Object#toString()
     */
    public String toString(){
        return player.toString()+type.toString();
    }    

    /**
     * Returns the playerType.
     * @return PlayerType player
     */
    public PlayerType getPlayer(){return player;} 

    /**
     * Returns the type of piece.
     * @return PieceType Piece
     */
    public PieceType getType(){return type;}



    /**
     * Checks if the move is a valid move by the piece.
     * @param initPos Initial Coordinate 
     * @param finalPos Final Coordinate
     * @return boolean If the move is Valid
     */
    public abstract boolean isValidMove(Coordinate initPos,Coordinate finalPos);


    /**
     * Return the path for movement.
     * @param initPos The initial Coordinate
     * @param finalPos The final Coordinate
     * @return Coordinate[] The Path for the movement 
     */
    public abstract Coordinate[] getPath(Coordinate initPos,Coordinate finalPos);


}

Pawn.java

package pieces;

import player.PlayerType;
import game.Coordinate;

/**
 * @author gnik
 * 
 */
public class Pawn extends Piece {

    /**
     * Create a pawn
     * 
     * @param player
     *            The player the piece belongs to
     */
    public Pawn(PlayerType player) {
        super(player, PieceType.PAWN);
    }

    @Override
    public boolean isValidMove(Coordinate initPos, Coordinate finalPos) {
        if (initPos.equals(finalPos)) {
            return false;
        }

        // This is for normal pawn moves.
        if (Math.abs(initPos.getY() - finalPos.getY()) == 1
                && Math.abs(initPos.getX() - finalPos.getX()) == 0) {
            // White can only move forward
            if (this.getPlayer() == PlayerType.WHITE) {
                if (initPos.getY() < finalPos.getY()) {
                    return true;
                }
            }
            // Black can only move backward in a sense.
            if (this.getPlayer() == PlayerType.BLACK) {
                if (initPos.getY() > finalPos.getY()) {
                    return true;
                }
            }

        }

        // This is for first pawn move.
        if (Math.abs(initPos.getY() - finalPos.getY()) == 2
                && Math.abs(initPos.getX() - finalPos.getX()) == 0
                && (initPos.getY() == 1 || initPos.getY() == 6)) {

            // White can only move forward
            if (this.getPlayer() == PlayerType.WHITE) {
                if (initPos.getY() < finalPos.getY()) {
                    return true;
                }
            }
            // Black can only move backward in a sense.
            if (this.getPlayer() == PlayerType.BLACK) {
                if (initPos.getY() > finalPos.getY()) {
                    return true;
                }
            }

        }

        // This if for normal pawn captures.
        // this is for Enpassant.

        return false;
    }

    @Override
    public Coordinate[] getPath(Coordinate initPos, Coordinate finalPos) {
        //This is for pawn captures
        if (initPos.getX()!=finalPos.getX()){return new Coordinate[]{initPos,finalPos};}
        //This is for normal pawn moves and first pawn moves.
        int pathLength = Math.abs(initPos.getY() - finalPos.getY()) + 1;
        Coordinate[] path = new Coordinate[pathLength];

        for (int cnt = 0; cnt < pathLength; cnt++) {
            path[cnt] = new Coordinate(initPos.getX(), Math.min(initPos.getY(),
                    finalPos.getY()) + cnt);
        }

        return path;
    }
}

Queen.java

package pieces;

import player.PlayerType;
import game.Coordinate;

/**
 * @author gnik
 *
 */
public class Queen extends Piece{

    /**
     * Create a Queen
     * @param player The player the queen belongs to
     */
    public Queen(PlayerType player){
        super(player,PieceType.QUEEN);
    }


    @Override
    public boolean isValidMove(Coordinate initPos,Coordinate finalPos) {
        if (initPos.equals(finalPos)){return false;}

        //This is the bishop move.
        int diffX=Math.abs(initPos.getX()-finalPos.getX());
        int diffY=Math.abs(initPos.getY()-finalPos.getY());
        if (diffX==diffY) return true;

        //This is the rook move.
        if (initPos.getX()==finalPos.getX() ||
                initPos.getY()==finalPos.getY())
            {return true;}

        return false;
    }

    @Override
    public Coordinate[] getPath(Coordinate initPos,Coordinate finalPos) {
        Coordinate[] path;
        //If it is a rook move
        if (initPos.getX()==finalPos.getX() ||
                initPos.getY()==finalPos.getY())
        {
            int pathLength=Math.abs(initPos.getX()-finalPos.getX())
                    +Math.abs(initPos.getY()-finalPos.getY())+1;
            path=new Coordinate[pathLength];
            for (int cnt=0;cnt<pathLength;cnt++)
            {
                if ((initPos.getX()==finalPos.getX())){
                    path[cnt]=new Coordinate(initPos.getX(),Math.min(initPos.getY(),finalPos.getY())+cnt);
                }
                else{
                    path[cnt]=new Coordinate(Math.min(initPos.getX(),finalPos.getX())+cnt,initPos.getY());
                }
            }

        }
        else
        {
            //If it a bishop move.
            int pathLength=( Math.abs(initPos.getX()-finalPos.getX())+
                    Math.abs(initPos.getY()-finalPos.getY()) )/2+1;
            path=new Coordinate[pathLength];

            //Integer.signum(a) provides the sign of a number 1 if positive and -1 if negative.
            //In this case i am considering initPos as the first point and finalPos as second
            int i_X=Integer.signum(finalPos.getX()-initPos.getX());
            int i_Y=Integer.signum(finalPos.getY()-initPos.getY());

            for (int cnt=0;cnt<pathLength;cnt++)
            {
                path[cnt]=new Coordinate(initPos.getX()+i_X*cnt,initPos.getY()+i_Y*cnt);
            }
        }





        return path;
    }

}

Knight.java

package pieces;

import player.PlayerType;
import game.Coordinate;

/**
 * @author gnik
 *
 */
public class Knight extends Piece {

    /**
     * Create a Knight
     * @param player The player the Knight belongs to
     */
    public Knight(PlayerType player){
        super(player,PieceType.KNIGHT);
    }


    @Override
    public boolean isValidMove(Coordinate initPos,Coordinate finalPos) {
        if (initPos.equals(finalPos)){return false;}

        int diffX=Math.abs(initPos.getX()-finalPos.getX());
        int diffY=Math.abs(initPos.getY()-finalPos.getY());
        if ((diffX+diffY)==3 && diffX!=0 && diffY!=0)
        {return true;}

        return false;
    }

    @Override
    public Coordinate[] getPath(Coordinate initPos,Coordinate finalPos) {
        //The knight can jump over pieces.
        return new Coordinate[]{initPos,finalPos};
    }

}

I think the I don't need place all the code of all the pieces for a full code review. Therefore I am excluding some of the other pieces. If you feel like you need to look at it, I have provided a link to my repository.

Coordinate.java

package game;

/**
 * @author gnik
 * 
 */
public class Coordinate {
    int positionX;
    int positionY;

    /**
     * Initializes a new coordinate with the x and y values.
     * 
     * @param x
     *            The x coordinate
     * @param y
     *            The y coordinate
     */
    public Coordinate(int x, int y) {
        positionX = x;
        positionY = y;

    }

    /**
     * Creates a coordinate from a UCI move string
     * @param coordinate The coordinate in string format (UCI)
     */
    public Coordinate(String coordinate){
        positionX=(char)coordinate.toCharArray()[0]-97;
        positionY=Integer.parseInt(coordinate.substring(1,2))-1;

    }
    /**
     * Checks if a Coordinate is valid or not
     * 
     * @return boolean If coordinate is valid
     */
    public boolean isValid() {
        if ((positionX >= 0 && positionX < 8)
                && (positionY >= 0 && positionY < 8)) {
            return true;
        }
        return false;
    }

    /**
     * Gets the x coordinate
     * 
     * @return int The x coordinate
     */
    public int getX() {
        return positionX;
    }

    /**
     * Gets the y coordinate
     * 
     * @return int The y coordinate.
     */
    public int getY() {
        return positionY;
    }

    /**
     * Sets the x coordinate
     * 
     * @param x
     *            The x coordinate
     */
    public void setX(int x) {
        positionX = x;
    }

    /**
     * Sets the y coordinate
     * 
     * @param y
     *            The y coordinate
     */
    public void setY(int y) {
        positionY = y;
    }

    /*
     * (non-Javadoc)
     * 
     * @see java.lang.Object#toString()
     */
    public String toString() {
        return Integer.toString(positionX) + "," + Integer.toString(positionY);
    }

    /**
     * Checks if two coordinates are equal. The x and y coordinates should be
     * equal.
     * 
     * @param coordinate
     *            A coordinate
     * @return boolean If the coordinates have equal value
     */
    public boolean equals(Coordinate coordinate) {
        if ((positionX == coordinate.getX())
                && (positionY == coordinate.getY())) {
            return true;
        }
        return false;
    }

    /**
     * This converts the coordinate to UCI chess notation
     * @return String The string representation of the square in UCI form
     */
    public String getParsedCoordinate() {
        String parsedString = "";
        parsedString = (char) (positionX + 97)
                + Integer.toString(positionY + 1);
        return parsedString;
    }
}

Square.java

package game;

import pieces.Piece;

/**
 * @author gnik
 *
 */
public class Square {
    /**
     * The coordinate of the square
     */
    private Coordinate coordinate;
    /**
     * The piece object
     */
    private Piece piece = null;

    /**
     * Creates a new square.
     * @param coordinate The coordinate of the square
     * @param piece The piece if the square contains any.
     */
    public Square(Coordinate coordinate, Piece piece) {
        this.coordinate = coordinate;
        this.piece = piece;
    }

    /**
     * This is the alternative way to construct a square. Use if no piece is present
     * This calls Square(Coordinate,null)
     * @param coordinate Coordinate of the square
     */
    public Square(Coordinate coordinate) {
        this(coordinate, null);
    }

    /**
     * This returns the coordinate of the square.
     * @return coordinate Coordinate of square
     */
    public Coordinate getCoordinate() {
        return coordinate;
    }

    /**
     * This gets the piece the square is in. 
     * @return piece Piece
     */
    public Piece getPiece() {
        return piece;
    }

    /**
     * Checks if two square have the same coordinate
     * @param square Square
     * @return boolean If the squares are equal.
     */
    public boolean equals(Square square) {
        if (square.getCoordinate().equals(coordinate))
            return true;
        return false;
    }

    /**
     * Checks if the square contains a piece
     * @return boolean If square contains a piece 
     */
    public boolean isOccupied() {
        if (piece == null) {
            return false;
        }
        return true;
    }

    /**
     * Gets a string representation of the square
     * @return string String representation of the square
     */
    public String getPieceString() {
        if (piece == null) {
            return "  ";
        }
        return piece.toString();
    }

    /**
     * Removes any piece that is currently in the square
     */
    public void releasePiece() {
        piece = null;
    }

    /**
     * Sets a piece to the square
     * @param piece The piece object
     */
    public void setPiece(Piece piece) {
        this.piece = piece;
    }

}

Board.java

package game;

import pieces.*;
import player.PlayerType;



/**
 * @author gnik
 *
 */
public class Board {

    private Square[][] squares=new Square[8][8];


    /**
     * This initialized the board to it's original position.
     */

    public Board()
    {
        setSquares();
        setWhitePieces();
        setBlackPieces();

    }

    /**
     * Resets the board to it's original position
     */
    public void resetBoard(){
        setSquares();
        setWhitePieces();
        setBlackPieces();
    }

    /**
     * Initializes all the squares
     */
    private void setSquares(){
        for (int x=0;x<8;x++)
        {
            for (int y=0;y<8;y++)
            {
                squares[x][y]=new Square(new Coordinate(x,y));
            }
        }

    }

    /**
     * Initializes and assigns all white Pieces.
     */
    private void setWhitePieces(){
        squares[2][0].setPiece(new Bishop(PlayerType.WHITE));
        squares[5][0].setPiece(new Bishop(PlayerType.WHITE));
        squares[1][0].setPiece(new Knight(PlayerType.WHITE));
        squares[6][0].setPiece(new Knight(PlayerType.WHITE));
        squares[0][0].setPiece(new Rook(PlayerType.WHITE));
        squares[7][0].setPiece(new Rook(PlayerType.WHITE));
        squares[3][0].setPiece(new Queen(PlayerType.WHITE));
        squares[4][0].setPiece(new King(PlayerType.WHITE));
        squares[0][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[1][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[2][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[3][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[4][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[5][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[6][1].setPiece(new Pawn(PlayerType.WHITE));
        squares[7][1].setPiece(new Pawn(PlayerType.WHITE));

    }

    /**
     * Initializes and sets all Black Pieces.
     */
    private void setBlackPieces(){
        squares[2][7].setPiece(new Bishop(PlayerType.BLACK));
        squares[5][7].setPiece(new Bishop(PlayerType.BLACK));
        squares[1][7].setPiece(new Knight(PlayerType.BLACK));
        squares[6][7].setPiece(new Knight(PlayerType.BLACK));
        squares[0][7].setPiece(new Rook(PlayerType.BLACK));
        squares[7][7].setPiece(new Rook(PlayerType.BLACK));
        squares[3][7].setPiece(new Queen(PlayerType.BLACK));
        squares[4][7].setPiece(new King(PlayerType.BLACK));
        squares[0][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[1][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[2][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[3][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[4][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[5][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[6][6].setPiece(new Pawn(PlayerType.BLACK));
        squares[7][6].setPiece(new Pawn(PlayerType.BLACK));

    }


    /**
     * Returns all the squares on the board. 
     * @return Square[][] A dimensional array of all the Squares
     */
    public Square[][] getSquares(){
        return squares;
    }


    /**
     * This gets the square with a specific coordinate.
     * @param coordinate Coordinate of the square
     * @return Square The Square object
     */
    public Square getSquare(Coordinate coordinate){
        Square result=null;
        for (int x=0;x<8;x++)
        {
            for (int y=0;y<8;y++){
                if (squares[x][y].getCoordinate().equals(coordinate))
                {
                    result=squares[x][y];
                }
            }
        }
        return result;
    }

    /**
     * Makes a Move from initial Coordinate to final coordinate
     * @param initCoordinate The initial Coordinate.
     * @param finalCoordinate The final Coordinate.
     */
    public void makeMove(Coordinate initCoordinate,Coordinate finalCoordinate)
    {
        makeMove(getSquare(initCoordinate),getSquare(finalCoordinate));
    }

    /**
     * This set the piece in the specified coordinate
     * @param coordinate Coordinate of the piece
     * @param piece The piece object to be set.
     */
    public void setPiece(Coordinate coordinate,Piece piece){
        getSquare(coordinate).setPiece(piece);
    }
    /**
     * Captures the piece that is present in the square.
     * @param square The square of the piece
     */
    public void capturePiece(Square square){
        if(square.isOccupied())
        {
            square.releasePiece();
        }
    }

    /**
     * This makes a move from a square to another. **Move may be invalid**
     * @param initSquare The initial Square.
     * @param finalSquare The final Square
     */
    public void makeMove(Square initSquare,Square finalSquare){
        //Has a piece been captured;
        if(finalSquare.isOccupied())
        {
            capturePiece(finalSquare);
        }
        //Make the move
        finalSquare.setPiece(initSquare.getPiece());
        initSquare.releasePiece();
    }

    /**
     * This prints the board in the command line.
     */
    public void printBoard(){
        for (int y=7;y>=0;y--){
            for(int x=0;x<8;x++)
            {
                System.out.print(squares[x][y].getPieceString()+" ");
            }
            System.out.print('\n');
        }
    }



}

BoardManager.java

package game;

import java.util.ArrayList;
import java.util.List;

import pieces.Bishop;
import pieces.Knight;
import pieces.Pawn;
import pieces.Piece;
import pieces.PieceType;
import pieces.Queen;
import pieces.Rook;
import player.PlayerType;

/**
 * @author gnik
 * 
 */
public class BoardManager {
    /**
     * The board object
     */
    private Board board;

    /**
     * Current Player which is to move. Default is PlayerType.WHITE
     */
    private PlayerType currentPlayer = PlayerType.WHITE;

    /**
     * This is the list that holds all the moves made by the user.
     */
    private List<Move> moveList = new ArrayList<Move>();

    /**
     * Constructs a new BoardManager object
     */
    public BoardManager() {
        this.board = new Board();
    }

    /**
     * Resets the board to it's initial state
     */
    public void resetBoard() {
        moveList = new ArrayList<Move>();
        board.resetBoard();
        currentPlayer = PlayerType.WHITE;
    }

    /**
     * Switches the player currently to move.
     */
    private void switchCurrentPlayer() {
        if (currentPlayer == PlayerType.WHITE) {
            currentPlayer = PlayerType.BLACK;
        } else {
            currentPlayer = PlayerType.WHITE;
        }

    }

    /**
     * Return the player who is to move
     * 
     * @return PlayerType The player
     */
    public PlayerType getCurrentPlayer() {
        return currentPlayer;
    }

    /**
     * Returns a list of moves that the player has made.
     * 
     * @return List The list of moves
     */
    public List<Move> getMoveList() {
        return moveList;
    }

    /**
     * Returns the board object
     * 
     * @return board The board object
     */
    public Board getBoard() {
        return board;
    }

    /**
     * Promotes a pawn to a newer piece. Calls isValidPromotion function first
     * 
     * @param square
     *            Promotion Square
     * @param pieceType
     *            The type of piece to promote to. If none is provided it
     *            defaults to Queen.
     * @return boolean If the promotion was made
     */
    public boolean promote(Square square, PieceType pieceType) {
        if (isValidPromotion(square)) {
            Piece piece;
            if (pieceType == PieceType.BISHOP) {
                piece = new Bishop(square.getPiece().getPlayer());
            } else if (pieceType == PieceType.KNIGHT) {
                piece = new Knight(square.getPiece().getPlayer());
            } else if (pieceType == PieceType.ROOK) {
                piece = new Rook(square.getPiece().getPlayer());
            } else {
                piece = new Queen(square.getPiece().getPlayer());
            }
            moveList.add(new Move(square.getCoordinate(), square
                    .getCoordinate(), piece, square));
            square.setPiece(piece);
            return true;
        }
        return false;
    }

    /**
     * Checks if the square contains a pawn that can be promoted.
     * 
     * @param square
     *            Square of the pawn
     * @return boolean If the pawn can be promoted
     */
    public boolean isValidPromotion(Square square) {
        if (!square.isOccupied() == true) {
            return false;
        }
        if (square.getPiece().getType() == PieceType.PAWN) {
            int col = 7;
            if (square.getPiece().getPlayer() == PlayerType.BLACK) {
                col = 0;
            }
            if (square.getCoordinate().equals(
                    new Coordinate(square.getCoordinate().getX(), col))) {
                return true;
            }

        }
        return false;
    }

    /**
     * Returns if either of the players are checkmated.
     * 
     * @return boolean
     */
    public boolean isGameOver() {
        if (isCheckmate(PlayerType.WHITE) || isCheckmate(PlayerType.BLACK)) {
            return true;
        }
        return false;
    }

    /**
     * Returns if the player is checkmated or not.
     * 
     * @param player
     *            The player to check checkmate for
     * @return boolean
     */
    public boolean isCheckmate(PlayerType player) {
        Square[] attackers = getAttackingPieces(player);
        // If there are no attackers
        if (attackers.length == 0) {
            return false;
        }

        // If there is more than one attacker then there are many options check all.
        boolean checkmate = true;
        Square attackerSquare = attackers[0];
        Square kingSquare = squareOfKing(player);
        Coordinate[] attackPath = attackerSquare.getPiece().getPath(
                attackerSquare.getCoordinate(), kingSquare.getCoordinate());
        Square[][] allSquares = board.getSquares();
        for (int x = 0; x < 8; x++) {
            for (int y = 0; y < 8; y++) {

                // If the king can move to a different square.
                if (isValidMove(squareOfKing(player), board.getSquares()[x][y])
                        && squareOfKing(player) != board.getSquares()[x][y]) {
                    return false;
                }
                for (Coordinate coordinate : attackPath) {
                    Square tmpSquare = allSquares[x][y];
                    // The square must be occupied
                    if (tmpSquare.isOccupied()) {

                        // The player must move his own piece between the paths
                        // of the attacker and the King.
                        // If it can do so then there is no checkmate
                        if (tmpSquare.getPiece().getPlayer() == kingSquare
                                .getPiece().getPlayer()
                                && isValidMove(tmpSquare,
                                        board.getSquare(coordinate))) {
                            checkmate = false;
                        }
                    }

                }
            }

        }
        return checkmate;

    }

    /**
     * This undoes the previous move.
     */
    public void undoMove() {
        if (moveList.isEmpty()) {
            return;
        }
        Move lastMove = moveList.get(moveList.size() - 1);
        if (lastMove.getFinalCoordinate() != lastMove.getInitCoordinate()) {
            board.makeMove(lastMove.getFinalCoordinate(),
                    lastMove.getInitCoordinate());

            if (lastMove.isCapture()) {
                board.setPiece(lastMove.getCaptureCoordinate(),
                        lastMove.getCapturedPiece());
            }
        } else {
            // If the move was a promotion.
            moveList.remove(moveList.size() - 1);
            lastMove = moveList.get(moveList.size() - 1);
            board.setPiece(lastMove.getFinalCoordinate(), new Pawn(lastMove
                    .getPiece().getPlayer()));
        }
        // Flush the lastmove.
        moveList.remove(moveList.size() - 1);
        // Switch the current players.
        switchCurrentPlayer();
    }

    /**
     * This function returns all the valid moves a square/piece can make
     * 
     * @param coordinate
     *            The coordinate of the piece/square.
     * @return Square[] The array of possible squares.
     */
    public Square[] getValidMoves(Coordinate coordinate) {
        List<Square> moves = new ArrayList<Square>();
        for (int x = 0; x < 8; x++) {
            for (int y = 0; y < 8; y++) {
                if (isValidMove(board.getSquare(coordinate),
                        board.getSquares()[x][y])) {
                    moves.add(board.getSquares()[x][y]);
                }
            }
        }
        return moves.toArray(new Square[0]);
    }

    /**
     * Returns the array of squares of the pieces that are attacking the King If
     * no piece is attacking it then empty array is returned.
     * 
     * @param player
     *            The player whose king is under attack
     * @return Squares[] The array of squares of pieces that are attacking the
     *         King. Max Size of array is 2
     */
    public Square[] getAttackingPieces(PlayerType player) {
        List<Square> squares = new ArrayList<Square>();
        Square[][] allSquares = board.getSquares();
        Square kingSquare = squareOfKing(player);
        for (int x = 0; x < 8; x++) {
            for (int y = 0; y < 8; y++) {
                Square tmpSquare = allSquares[x][y];
                if (tmpSquare.isOccupied()) {
                    if (isValidMovement(tmpSquare, kingSquare)
                            && kingSquare.getPiece().getPlayer() != tmpSquare
                                    .getPiece().getPlayer()) {
                        squares.add(tmpSquare);
                    }
                }

            }
        }
        return squares.toArray(new Square[0]);
    }

    /**
     * Makes a move from initial coordinate to final one. It calls
     * isValidMove(),isValidCastling() and isValidEnpassant()
     * 
     * @param initCoordinate
     *            Initial Coordinate
     * @param finalCoordinate
     *            Final Coordinate
     * @return boolean If the move was made
     */
    public boolean move(Coordinate initCoordinate, Coordinate finalCoordinate) {
        if(initCoordinate==null || finalCoordinate==null){return false;}
        // Only valid coordinates are allowed.
        if (!(initCoordinate.isValid() && finalCoordinate.isValid())) {
            return false;
        }
        Square s1 = board.getSquare(initCoordinate);
        Square s2 = board.getSquare(finalCoordinate);
        //Checks for sane moves.
        if(!isSaneMove(s1,s2)){return false;}
        // Only the current player can move the piece.
        if (currentPlayer == s1.getPiece().getPlayer()) {
            if (isValidCastling(s1, s2)) {
                Piece tmp = s1.getPiece();
                castle(s1, s2);
                switchCurrentPlayer();
                moveList.add(new Move(s1.getCoordinate(), s2.getCoordinate(),
                        tmp));
                return true;
            } else if (isValidEnpassant(s1, s2)) {
                Piece tmp = s1.getPiece();
                Square capture = board
                        .getSquare((moveList.get(moveList.size() - 1)
                                .getFinalCoordinate()));
                enpassant(s1, s2);
                switchCurrentPlayer();
                moveList.add(new Move(s1.getCoordinate(), s2.getCoordinate(),
                        tmp, capture));
                return true;
            } else if (isValidMove(s1, s2)) {
                switchCurrentPlayer();
                moveList.add(new Move(s1.getCoordinate(), s2.getCoordinate(),
                        s1.getPiece(), s1));
                board.makeMove(s1, s2);
                return true;
            }
        }
        return false;
    }

    /**
     * Checks if the move is valid enpassant move.
     * 
     * @param s1
     *            Initial Square
     * @param s2
     *            Final Square
     * @return boolean If enpassant valid
     */
    private boolean isValidEnpassant(Square s1, Square s2) {
        // The final square should be empty
        if (s2.isOccupied()) {
            return false;
        }

        // The first piece should be a pawn.
        if (s1.getPiece().getType() != PieceType.PAWN) {
            return false;
        }
        // Move type is different according to player color
        if (s1.getPiece().getPlayer() == PlayerType.WHITE) {
            if (s1.getCoordinate().getY() > s2.getCoordinate().getY()) {
                // White can only move forward
                return false;
            }
        } else {
            if (s1.getCoordinate().getY() < s2.getCoordinate().getY()) {
                // Black can only move backward
                return false;
            }
        }
        // The move should be like a bishop move to a single square.
        if (Math.abs(s1.getCoordinate().getX() - s2.getCoordinate().getX()) == 1
                && Math.abs(s1.getCoordinate().getY()
                        - s2.getCoordinate().getY()) == 1) {
            // There should be a pawn move before enpassant.
            if (moveList.isEmpty()) {
                return false;
            }
            Move lastMove = moveList.get(moveList.size() - 1);
            if (lastMove.getPiece() == null) {
                return false;
            }
            if (board.getSquare(lastMove.getFinalCoordinate()).getPiece()
                    .getType() == PieceType.PAWN) {
                // The pawn should be moving two steps forward/backward.
                // And our pawn should be moving to the same file as the last
                // pawn
                if (Math.abs(lastMove.getFinalCoordinate().getY()
                        - lastMove.getInitCoordinate().getY()) == 2
                        && lastMove.getFinalCoordinate().getX() == s2
                                .getCoordinate().getX()) {
                    return true;
                }
            }
        }
        return false;
    }

    /**
     * Makes a Enpassant move
     * 
     * @param initSquare
     *            Initial Square
     * @param finalSquare
     *            Final Square
     */
    private void enpassant(Square initSquare, Square finalSquare) {
        Move lastMove = moveList.get(moveList.size() - 1);
        board.capturePiece(board.getSquare(lastMove.getFinalCoordinate()));
        board.makeMove(initSquare, finalSquare);

    }

    /**
     * Checks if the given move makes check for the moving player
     * 
     * @param initSquare
     *            Initial Square
     * @param finalSquare
     *            Final Square
     * @return boolean If the move makes check.
     */
    private boolean moveMakesCheck(Square initSquare, Square finalSquare) {
        Piece temporaryPiece = finalSquare.getPiece();
        finalSquare.setPiece(initSquare.getPiece());
        initSquare.releasePiece();
        boolean enpassant = false;
        Piece tmp = null;
        Square lastMove = null;
        // if it is a enpassant move then you must also remove a piece from the
        // board temporarily.
        if (isValidEnpassant(initSquare, finalSquare)) {
            enpassant = true;
            lastMove = board.getSquare(moveList.get(moveList.size() - 1)
                    .getFinalCoordinate());
            tmp = lastMove.getPiece();
            lastMove.releasePiece();
        }

        if (isCheck(finalSquare.getPiece().getPlayer())) {
            initSquare.setPiece(finalSquare.getPiece());
            finalSquare.setPiece(temporaryPiece);
            if (enpassant) {
                lastMove.setPiece(tmp);
            }
            return true;
        } else {
            initSquare.setPiece(finalSquare.getPiece());
            finalSquare.setPiece(temporaryPiece);
            if (enpassant) {
                lastMove.setPiece(tmp);
            }
        }
        return false;
    }

    /**
     * Gets the square of the King
     * 
     * @param player
     *            The player whose king it is
     * @return Square The square of the king
     */
    private Square squareOfKing(PlayerType player) {
        Square[][] squares = board.getSquares();
        Square squareOfKing = null;
        for (int x = 0; x < 8; x++) {
            for (int y = 0; y < 8; y++) {
                Square square = squares[x][y];
                if (square.isOccupied()) {
                    if (square.getPiece().getType() == PieceType.KING
                            && square.getPiece().getPlayer() == player) {
                        squareOfKing = square;
                    }
                }
            }
        }
        return squareOfKing;
    }

    /**
     * Checks if there is check for the player
     * 
     * @param player
     *            Is this player in check
     * @return boolean If the player is in check
     */
    public boolean isCheck(PlayerType player) {
        if (getAttackingPieces(player).length > 0) {
            return true;
        } else {
            return false;
        }
    }

    /**
     * Checks if the move is valid pawn capture move
     * 
     * @param initSquare
     *            Initial Square
     * @param finalSquare
     *            Final Square
     * @return boolean If the pawn capture is valid
     */
    private boolean isValidPawnCapture(Square initSquare, Square finalSquare) {
        // If the piece is not a pawn OR this is not a capture.
        if (!finalSquare.isOccupied()
                || initSquare.getPiece().getType() != PieceType.PAWN) {
            return false;
        }
        Coordinate initPos = initSquare.getCoordinate();
        Coordinate finalPos = finalSquare.getCoordinate();
        PlayerType player = initSquare.getPiece().getPlayer();

        // This is for normal pawn capture moves.
        if (Math.abs(initPos.getY() - finalPos.getY()) == 1
                && Math.abs(initPos.getX() - finalPos.getX()) == 1) {
            // White can only move forward
            if (player == PlayerType.WHITE) {
                if (initPos.getY() < finalPos.getY()) {
                    return true;
                }
            }
            // Black can only move backward in a sense.
            if (player == PlayerType.BLACK) {
                if (initPos.getY() > finalPos.getY()) {
                    return true;
                }
            }

        }
        return false;
    }

    /**
     * @param square
     *            The square of the piece
     * @return boolean If this piece has been moved or captured.
     */
    private boolean hasPieceMoved(Square square) {
        for (Move move : moveList) {
            if (move.getInitCoordinate() == square.getCoordinate()
                    || move.getFinalCoordinate() == square.getCoordinate()) {
                return true;
            }
        }
        return false;
    }

    /**
     * Checks if it is valid Castling move
     * 
     * @param kingSquare
     *            The square of the king
     * @param rookSquare
     *            The square of the rook
     * @return boolean If this is valid Castling
     */
    private boolean isValidCastling(Square kingSquare, Square rookSquare) {
        // Check if the squares are occupied.
        if (!(kingSquare.isOccupied() && rookSquare.isOccupied())) {
            return false;
        }
        // Check if the pieces have been moved or not.
        if (hasPieceMoved(kingSquare) || hasPieceMoved(rookSquare)) {
            return false;
        }

        // First check if the move is valid.
        if (!rookSquare.getPiece().isValidMove(kingSquare.getCoordinate(),
                rookSquare.getCoordinate())) {
            return false;
        }
        // Check if the path is clear
        if (!isPathClear(
                rookSquare.getPiece().getPath(rookSquare.getCoordinate(),
                        kingSquare.getCoordinate()),
                rookSquare.getCoordinate(), kingSquare.getCoordinate())) {
            return false;
        }
        // Now check if the movement of the castling is fine
        // First check if the piece is king and rook
        if (kingSquare.getPiece().getType() == PieceType.KING
                && rookSquare.getPiece().getType() == PieceType.ROOK) {

            int col = 0;
            if (kingSquare.getPiece().getPlayer() == PlayerType.BLACK) {
                col = 7;
            }
            // The peices are in correct position for castling.

            if (kingSquare.getCoordinate().equals(new Coordinate(4, col))
                    && (rookSquare.getCoordinate().equals(
                            new Coordinate(0, col)) || rookSquare
                            .getCoordinate().equals(new Coordinate(7, col)))) {

                // Check if there is check in any way between the king and final
                // king square
                int offset;
                if (Math.signum(rookSquare.getCoordinate().getX()
                        - kingSquare.getCoordinate().getX()) == 1) {
                    offset = 2;
                } else {
                    offset = -2;
                }
                // Calculates final kings X coordinate
                int kingX = kingSquare.getCoordinate().getX() + offset;
                for (Coordinate coordinate : rookSquare.getPiece()
                        .getPath(
                                kingSquare.getCoordinate(),
                                new Coordinate(kingX, kingSquare
                                        .getCoordinate().getY()))) {
                    if (kingSquare.equals(board.getSquare(coordinate))) {
                        // This removes a nasty null pointer exception
                        continue;
                    }
                    if (moveMakesCheck(kingSquare, board.getSquare(coordinate))) {
                        return false;
                    }
                }

                return true;
            }
        }
        return false;
    }

    /**
     * Makes a castle move.
     * <p>
     * It calls the isValidCastling() first.
     * 
     * @param kingSquare
     *            The square of the King
     * @param rookSquare
     *            The square of the Rook
     */
    private void castle(Square kingSquare, Square rookSquare) {
        int offset;
        if (Math.signum(rookSquare.getCoordinate().getX()
                - kingSquare.getCoordinate().getX()) == 1) {
            offset = 2;
        } else {
            offset = -2;
        }
        int kingX = kingSquare.getCoordinate().getX() + offset;
        int rookX = kingX - offset / 2;
        board.makeMove(kingSquare.getCoordinate(), new Coordinate(kingX,
                kingSquare.getCoordinate().getY()));
        board.makeMove(rookSquare.getCoordinate(), new Coordinate(rookX,
                rookSquare.getCoordinate().getY()));
    }

    /**
     * Checks if there are any obstacles between the pieces.
     * 
     * @param path
     *            The path between the pieces
     * @param initCoordinate
     *            Initial Coordinate to ignore
     * @param finalCoordinate
     *            Final Coordinate to ignore
     * @return boolean Is path clear
     */
    private boolean isPathClear(Coordinate[] path, Coordinate initCoordinate,
            Coordinate finalCoordinate) {
        Square[][] squares = board.getSquares();
        for (Coordinate coordinate : path) {
            if ((squares[coordinate.getX()][coordinate.getY()].isOccupied())
                    && (!coordinate.equals(initCoordinate))
                    && (!coordinate.equals(finalCoordinate))) {
                return false;
            }
        }
        return true;
    }

    /**
     * Checks trivial movement. If a sane move is being made it returns true.
     * @param initSquare The initial square
     * @param finalSquare The final square
     * @return boolean If a move is sane.
     */
    private boolean isSaneMove(Square initSquare, Square finalSquare) {
        //Check if the coordinates are valid
        if(!initSquare.getCoordinate().isValid() || !initSquare.getCoordinate().isValid() )
        {
            return false;
        }
        // If the player tries to move a empty square.
        if (!initSquare.isOccupied()) {
            return false;
        }
        // If it is moving to the same square.
        // This is also checked by every piece but still for safety
        if (initSquare.equals(finalSquare)) {
            return false;
        }

        return true;
    }

    /**
     * Checks if the piece can make a valid movement to the square.
     * 
     * @param initSquare
     *            Initial Square
     * @param finalSquare
     *            Final Square
     * @return boolean If movement is valid
     */
    private boolean isValidMovement(Square initSquare, Square finalSquare) {
        if(!isSaneMove(initSquare,finalSquare)){
            return false;
        }
        // If the player tries to take his own piece.
        if (finalSquare.isOccupied()) {
            if (initSquare.getPiece().getPlayer() == finalSquare.getPiece()
                    .getPlayer())
                return false;
        }
        // Check all movements here. Normal Moves, Pawn Captures and Enpassant.
        // Castling are handled by the move function itself.
        // If the piece cannot move to the square. No such movement.
        if (!initSquare.getPiece().isValidMove(initSquare.getCoordinate(),
                finalSquare.getCoordinate())
                && !isValidPawnCapture(initSquare, finalSquare)
                && !isValidEnpassant(initSquare, finalSquare)) {
            return false;
        }
        // Pawns cannot capture forward.
        if (initSquare.getPiece().getType() == PieceType.PAWN
                && finalSquare.isOccupied()
                && !isValidPawnCapture(initSquare, finalSquare)) {
            return false;
        }

        // If piece is blocked by other pieces
        Coordinate[] path = initSquare.getPiece().getPath(
                initSquare.getCoordinate(), finalSquare.getCoordinate());
        if (!isPathClear(path, initSquare.getCoordinate(),
                finalSquare.getCoordinate())) {
            return false;
        }
        return true;
    }

    /**
     * Checks if the given move is valid and safe. Calls the isValidMovement()
     * and moveMakesCheck().
     * 
     * @param initSquare
     *            The initial Square
     * @param finalSquare
     *            The final Square
     * @return boolean Whether move is valid
     */
    public boolean isValidMove(Square initSquare, Square finalSquare) {
        if (isValidCastling(initSquare, finalSquare)) {
            return true;
        }
        if (!isValidMovement(initSquare, finalSquare)) {
            return false;
        }
        if (moveMakesCheck(initSquare, finalSquare)) {
            return false;
        }
        return true;
    }

}

Move.java

package game;

import pieces.Piece;

/**
 * @author gnik
 * 
 */
public class Move {
    /**
     * This is the initial coordinate of the move
     */
    private Coordinate initCoordinate;

    /**
     * This is the final coordinate of the move
     */
    private Coordinate finalCoordinate;

    /**
     * This is the type of piece
     */
    private Piece piece;

    /**
     * This is the piece that was captured.
     */
    private Piece capturedPiece=null;

    /**
     * This is the coordinate of the captured piece 
     */
    private Coordinate captureCoordinate=null;

    /**
     * Creates a move object. Promotions are represented as movement to the same
     * square and the piece represented is the piece to be promoted to.
     * 
     * @param initCoordinate
     *            The initial move coordinate.
     * @param finalCoordinate
     *            The coordinate of the final move.
     * @param piece
     *            The piece that was moved
     */
    public Move(Coordinate initCoordinate, Coordinate finalCoordinate,
            Piece piece) {
        this(initCoordinate,finalCoordinate,piece,null);
    }

    /**
     * Creates a move object. Promotions are represented as movement to the same
     * square and the piece represented is the piece to be promoted to.
     * 
     * @param initCoordinate
     *            The initial move coordinate.
     * @param finalCoordinate
     *            The coordinate of the final move.
     * @param piece
     *            The piece that was moved
     * @param captureSquare The square of the piece that was captured.
     */
    public Move(Coordinate initCoordinate, Coordinate finalCoordinate,
            Piece piece,Square captureSquare) {
        this.initCoordinate = initCoordinate;
        this.finalCoordinate = finalCoordinate;
        this.piece = piece;
        if(captureSquare!=null){
        this.capturedPiece=captureSquare.getPiece();
        this.captureCoordinate=captureSquare.getCoordinate();
        }
    }

    /**
     * It returns the initial move coordinate
     * 
     * @return Coordinate The initial Coordinate
     */
    public Coordinate getInitCoordinate() {
        return initCoordinate;
    }

    /**
     * It returns the final move coordinate
     * 
     * @return Coordinate The final Coordinate
     */
    public Coordinate getFinalCoordinate() {
        return finalCoordinate;
    }

    /**
     * It returns the piece that was moved.
     * 
     * @return piece The piece that was moved.
     */
    public Piece getPiece() {
        return piece;
    }

    /**
     * Checks if the move was a capture
     * @return boolean If the move was a capture move.
     */
    public boolean isCapture(){
        if (capturedPiece==null){return false;}
        return true;
    }


    /**
     * Returns the piece that was captured when the move was made.
     * @return Returns the captured piece
     */
    public Piece getCapturedPiece(){
        return capturedPiece;
    }
    /**
     * Returns the coordinate of the capture.
     * @return The coordinate were the capture occured.
     */
    public Coordinate getCaptureCoordinate(){
        return captureCoordinate;
    }
}

MoveParser.java

package game;

import java.util.List;

/**
 * @author gnik
 * 
 */
public class MoveParser {

    /**
     * @param moveList
     *            The list of moves of the game
     * @return The parsed string of the moves
     */
    public static String parse(List<Move> moveList) {
        String parsedMoves = "";
        for (Move move : moveList) {
            //This if else is for promotion
            if (parseMove(move).length() != 1) {
                parsedMoves += " " + parseMove(move);
            } else {
                parsedMoves += parseMove(move);
            }

        }
        return parsedMoves;
    }

    /**
     * @param move
     *            The move object
     * @return The parsed string of one move
     */
    private static String parseMove(Move move) {
        if (move.getInitCoordinate().equals(move.getFinalCoordinate())) {
            return move.getPiece().getType().toString();
        }
        return move.getInitCoordinate().getParsedCoordinate()
                + move.getFinalCoordinate().getParsedCoordinate();
    }

}

Stockfish.java

package game;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;


/**
 * @author gnik
 *
 */
public class Stockfish {
    /**
     * This is the Stockfish process that runs in the background.
     */
    private Process stockfish;

    /**
     * This is the input BufferedWriter object to give commands to Engine.
     */
    private BufferedWriter stockfishInput;

    /**
     * This is the output object to recive output from Engine.
     */
    private BufferedReader stockfishOutput;


    private int stockfishLevel;

    /**
     * This starts a new process for the stockfish engine(if available only).
     * Also sets it in UCI_Chess960 mode with default options. Else it prints a stacktrace.
     * @param level The difficulty level from 0-20.
     */
    public Stockfish(int level) {
        stockfishLevel=level;
        try {
            stockfish = Runtime.getRuntime().exec("stockfish");
        } catch (IOException e) {
            e.printStackTrace();
        }
        stockfishInput = new BufferedWriter(new OutputStreamWriter(
                stockfish.getOutputStream()));
        stockfishOutput = new BufferedReader(new InputStreamReader(
                stockfish.getInputStream()));
        try {
            stockfishInput.write("setoption name UCI_Chess960 value true\n");
            stockfishInput.write("setoption name Skill Level value "+Integer.toString(stockfishLevel)+"\n");
            stockfishInput.flush();
            stockfishOutput.readLine();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    /**
     * Starts a new engine at max difficulty. Same as Engine(20)
     */
    public Stockfish()
    {
        this(20);
    }


    /**
     * Sets the level of difficulty of the engine
     * @param level The level of difficulty (0-20)
     */
    public void setLevel(int level ){
        stockfishLevel=level;
        try {
            stockfishInput.write("setoption name Skill Level value "+Integer.toString(stockfishLevel)+"\n");
            stockfishInput.flush();
        } catch (IOException e) {
            e.printStackTrace();
        }

    }
    /**
     * This returns the best possible move 
     * If there is no possible move(Checkmate) then it send a blank string.(BUGGY)
     * @param position The current board position in UCI format
     * @return String The best possible move in UCI format
     */
    public String getBestMove(String position) {
        String output = "Stockfish api";
        try {
            stockfishInput.write("position startpos moves " + position + "\n");
            stockfishInput.write("go" + "\n");
            stockfishInput.flush();
            while (!output.substring(0, 8).equals("bestmove")) {
                output = stockfishOutput.readLine();
            }
            stockfishInput.write("stop" + "\n");
            if (output.equals("bestmove (none)")){return "";}
            output = output.split(" ")[1];

        } catch (IOException e) {
            e.printStackTrace();
        }

        return output;
    }


    /**
     * Closes the to the Stockfish engine process gracefully. 
     */
    public void quit(){
        try {
            stockfishInput.write("quit\n");
        } catch (IOException e) {
            e.printStackTrace();
        }
        stockfish.destroy();
    }

}
\$\endgroup\$
  • \$\begingroup\$ Very impressive first project and first Code Review question! \$\endgroup\$ – 200_success May 19 '18 at 6:13
  • \$\begingroup\$ @200_success Thank you! Really glad the work paid off. \$\endgroup\$ – Gnik May 19 '18 at 6:58
7
\$\begingroup\$

First of all I got to say that I'm impressed with the overal quality of your code. So everything that follows is mostly minor nitpicking on things you can improve on in no specific order.

comments are for why, not what

I don't like comments like these:

/**
 * Returns the playerType.
 * @return PlayerType player
 */
public PlayerType getPlayer(){return player;} 

The method signature already clearly states what it returns. A PlayerType. There's no reason to specify this 3 times.

The only reason to ever do this is when your employer tells you to.

        //Integer.signum(a) provides the sign of a number 1 if positive and -1 if negative.
        //In this case i am considering initPos as the first point and finalPos as second
        int i_X=Integer.signum(finalPos.getX()-initPos.getX());
        int i_Y=Integer.signum(finalPos.getY()-initPos.getY());

This comment was a lot more useful. Although it's already present in the javadoc of the signum method. Most java IDE's have a key shortcut to see that documentation if you want.
That said I was missing a part of the documentation. What is the result of signum(0)?

queen move

After looking up that signum(0) returns 0 you actually don't need the "rooke move" case handled separately. Use that signum to decide each step in both X and Y direction. And move that way in each step in the for loop. If it's a rook move you'll just add 0 to the current position in the other direction so it works anyway.

braces on same line

The java convention is to put the opening brace { on the same line as the if/for/..., not on the following line. You seem to be rather inconsistent with this.

It's also a good idea to put the else on the same line as the } from the if to show they belong together.

if(...) {
    ...
} else {
    ...
}

PieceType ?

I don't see the point of this class. As far as I could tell it's only ever used when constructing a certain piece. If you remove this entirely does your code still work the same?

EDIT: You use this in BoardManager for the promotion checks so it actually does have a certain use. I can't tell if it's good or bad to have this at the moment so don't just delete it except if you can handle those cases entirely based on the Piece classes as well.

if(true) return true

This is the kind of construct I see a lot in beginners code:

public boolean isValid() {
    if ((positionX >= 0 && positionX < 8)
            && (positionY >= 0 && positionY < 8)) {
        return true;
    }
    return false;
}

The condition in the if is already a boolean. Why not return that directly instead?

public boolean isValid() {
    return (positionX >= 0 && positionX < 8)
            && (positionY >= 0 && positionY < 8);
}

PlayerType nitpicking

public enum PlayerType {
    WHITE("W"), BLACK("B");

    private String value;

    PlayerType(String value) {
        this.value = value;
    }

    @Override
    public String toString() {
        return value;
    }
}

Why the value at all? Note that it's not wrong to do it this way. I personally would just simplify this entire class to:

public enum PlayerType  {
    W,B;
}

char 97?

This line (char) (positionX + 97) contains a magic 97. I know from experience that this will work but I'm not sure whether this will start at a or A. It would be better if you write it explicitly as (char) (positionX + 'a').


EDIT extra questions.

1) BoardManager does too much.

I believe the most logical refactoring is to make the pieces themselves responsible for checking the full validity of a move. You already have the method

public abstract boolean isValidMove(Coordinate initPos,Coordinate finalPos);

and each of the pieces know which squares to check (for example, a knight ignores any in between square and a queen needs to check each square in the line between initial and final coordinates). What you do need is a way for the piece to request information on those in between squares. I'd say this can be achieved by passing in the BoardManager as a parameter to that method:

public abstract boolean isValidMove(Coordinate initPos,Coordinate finalPos, BoardManager boardManager);

That way the BoardManager can delegate the actual move checking to each of the pieces and only needs to provide the needed methods like isEmpty(Coordinate square) or isEnPassantSquare(Coordinate square).

A good exercise to check if a class does too much is to try to describe it's main responsibility in 1 to 3 sentences. For example the java.util.Math class has this as it's first line in the javadoc:

The class Math contains methods for performing basic numeric operations such as the elementary exponential, logarithm, square root, and trigonometric functions.

This one line tells exactly what to expect of each of the methods in the class. Each of those represents a numeric operation and it knows just enough to calculate those.

In my eyes your BoardManager class has as main responsibility: Keep track of the state of the board.

That way it's expected to find a field with the current state of the board. It's also fine to see the history stored here (although this might deserve a specialised class with an undoMove(Board board) for example that restores the given board to what it was before the last move).

What I don't expect to find is actual logic on the movement of pieces in this class. You already solved this partially by getting the path from the Piece but you can push the responsibility a bit futher into the Piece class.

To do the same exercise on the Piece class I would describe it as:

A class that represents a single Piece on the board. It handles the specific moves/attack moves of that piece.

That way it's not surprising to find methods on how a piece can move and even very specialised things like castling moves in a King piece. To make the castling checks easier I would also include a boolean field to tell if that piece had moved already or not. Might be tricky to undo correctly though.

2) how to deal with empty values.

Using null is a valid solution. But like you noticed yourself, if you don't constantly check for null before actually using the piece you'll get a NullPointerException. Using a good IDE would mitigate this problem a bit since it can warn you at compile time that this field might be null if you didn't check on it yet. (I know IntelliJ has this option and expect others like Eclipse or Netbeans to have the same functionality but don't know sure about those).

Alternatively you can create a special class to represent a non-Piece. For example

public class NoPiece extends Piece {

    ...

    public abstract boolean isValidMove(Coordinate initPos,Coordinate finalPos, BoardManager boardManager) {
        return false;
    }

    ....
}

You can also let certain methods throw specific errors like InvalidMoveException depending on how you'd handle those otherwise in your class.

On the plus side you'll get a lot less NullPointerExceptions and you might simplify certain methods.

The downside is that you still need to do the actual check for an "emtpy square" anyway so it can take more code to achieve the same functionality.

So to recap on your question: Yes there's an alternative but it's not always better. Look at it case by case. In certain situations it can greatly simplify the code, in others you don't gain anything from the extra Null Object class.

\$\endgroup\$
  • \$\begingroup\$ The comment, char 97 and if(true) return true are all valid points. But in the case of the PlayerType and PieceType, I thought It would make the debugging and printing the board easier as I could just print out the enums. Is there a better way to do this? The review seems spot on though. I still have some questions that I have just included in the question. If you could comment on these additional questions, I would accept this as the best answer. Thank you again for this. \$\endgroup\$ – Gnik May 19 '18 at 11:10
  • \$\begingroup\$ Enums have an implicit toString that prints out the "name" of the enum object. So in my simplified PlayerType the printout will be exactly the same as in yours. I also just noticed that you use the type in the toString method. An alternative would be to implement the tostring in each of the specific Piece classes to get the same effect. \$\endgroup\$ – Imus May 19 '18 at 18:50
  • \$\begingroup\$ Didn't know about the implicit toString(). Then the code is redundant. I will keep that in mind now. \$\endgroup\$ – Gnik May 19 '18 at 19:03
  • \$\begingroup\$ Added 2 edits. 1 for the PieceType and a major one for your extra questions :) \$\endgroup\$ – Imus May 19 '18 at 19:46
  • \$\begingroup\$ This sums everything up perfectly! Seems like I have a lot to do. :) \$\endgroup\$ – Gnik May 19 '18 at 19:53
1
\$\begingroup\$

Magic Numbers

There are a lot of raw integer literals in this code; integers are domain agnostic, and don't have any semantic context. For example, the digit 8 appears 30 times in your source code. Do all of those 8s mean the same thing? What about the 25 times 7 appears?

What's really going on here is that you've got some chess ideas (the black pawn rank, the king rook file), and you are using integers as in memory representations of those ideas -- but you've written your code in the language of the in memory representation, rather than in the language of your domain. And since there are a lot of different ideas that accidentally share the same in memory representation, it's hard to tell them apart.

Use the domain language where available

Chess boards don't have rows and cols; they have ranks and files.

Duplication of idea

When the same idea appears multiple times in your code, you should think about whether the code will be easier to maintain if that idea should have a single representation.

For example: the black pieces on a chess board are placed opposite their white counterparts; but that idea is only implied by the fact that the magic numbers in Board:setBlackPieces happen to match those in Board:setWhitePieces.

Tests

Maintaining code without making errors is statistically unlikely; having automated checks significantly reduces the changes of an undetected error.

Make learning at least one testing framework a priority. JUnit is a good starting point.

Separate IO from your core

The boundary where your code touches the outside is hard to test. Try to make that boundary as skinny as possible.

For example:

public void printBoard(){
    for (int y=7;y>=0;y--){
        for(int x=0;x<8;x++)
        {
            System.out.print(squares[x][y].getPieceString()+" ");
        }
        System.out.print('\n');
    }
}

There's no reason that Board should be coupled to System.out.

public void printBoard(PrintStream out){
    for (int y=7;y>=0;y--){
        for(int x = 0; x<8; x++)
        {
            out.print(squares[x][y].getPieceString()+" ");
        }
        out.print('\n');
    }
}

Separate UI from core

This is really the same idea as the previous; checking the correctness of the UI is hard, so you want to make the adapter between your app and the GUI framework as thin as you possibly can.

Same idea, different spelling: you want the UI adapters to be dumb; they do what they are told, but don't do logic for themselves. Your UI should know a lot about java.awt, but very little about chess.

An interesting way to challenge yourself here is to introduce a second UI; for example, the command line. When you find yourself duplicating logic in the second adapter, you have a big hint that you are doing that work in the wrong place.

Square square;
if (boardManager.getCurrentPlayer() == PlayerType.BLACK) {
    square = squares[row][col];
} else {
    square = squares[row][7 - col];
}

if (square.isOccupied()) {
    String playerString;
    String pieceString;
    if (square.getPiece().getPlayer() == PlayerType.WHITE) {
        playerString = "white";
    } else {
        playerString = "black";
    }
    if (square.getPiece().getType() == PieceType.KING) {
        pieceString = "King";
    } else if (square.getPiece().getType() == PieceType.PAWN) {
        pieceString = "Pawn";
    } else if (square.getPiece().getType() == PieceType.ROOK) {
        pieceString = "Rook";
    } else if (square.getPiece().getType() == PieceType.KNIGHT) {
        pieceString = "Knight";
    } else if (square.getPiece().getType() == PieceType.BISHOP) {
        pieceString = "Bishop";
    } else {
        pieceString = "Queen";
    }

    Image image = null;
    try {
        image = ImageIO.read(getClass().getResource(
                "/" + playerString + pieceString + ".png"));
    } catch (IOException e) {
    }
    button.setIcon(new ImageIcon(image));

} else {
    button.setIcon(null);
}

Look carefully: that's just a function that computes a URL for a (row, column) pair. So make the function explicit, and then put it where it belongs (the functional core).

Don't eat exceptions

try {
    image = ImageIO.read(getClass().getResource(
            "/" + playerString + pieceString + ".png"));
} catch (IOException e) {
}

If you are sure that something can never happen, then throw an AssertionError that contains the exception that will "never" happen.

Duplication of state

Something to recognize about chess: the current state of the board can be calculated from the initial board layout and the list of moves in the game.

But the board by itself isn't enough. See, for instance, the Langstaff position.

So your logic on BoardManager:move probably needs a re-think.

\$\endgroup\$
  • \$\begingroup\$ +1 for a very detailed review and for checking the GUI. One thing that bothers me is the duplication of state comment. The board manager class does contain a moveList variable that contains the list of moves in the game. Could you clarify that point for me? I am really satisfied with your answer and your review of the general structure of the code. Wish there was a way to mark two correct answers. \$\endgroup\$ – Gnik May 26 '18 at 9:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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