Simple Chess Game, Part 1: The Board

Question

I am currently creating a chess game, and have so far done the board.

The board is pretty much a two-dimensional array, where Pieces are stored in their positions. For example, board[0][0] will be the position A1, which at the beginning of the game, is a White Rook.

The board has some useful methods (as well as required methods), such as isProtected(), which checks if a specified square is protected, or startTest(), testMove(), and stopTest(), which allows test-moving. The only problem to this is that it doesn't handle En Passants. Here is the ChessBoard class:

import java.util.Arrays;

public class ChessBoard {

    /**
     * The size of the board.
     */
    public static final int SIZE = 8;

    /**
     * The value to pass to the Piece constructors. This means the piece is
     * white.
     */
    private static final boolean WHITE = true;

    /**
     * The value to pass to the Piece constructors. This means the piece is
     * black.
     */
    private static final boolean BLACK = false;

    /**
     * Stores the information about the ability to castle.
     * 
     * [x][y]
     * 
     * x:
     * 
     * 0: short castle 1: long castle
     * 
     * y:
     * 
     * 0: white 1: black
     */
    private boolean[][] canCastle = { { true, true }, { true, true } };

    /**
     * The indices for the returned boolean array in castleInfo(). At this
     * index, information about the castling move is stored.
     */
    private final int isCastle = 0;

    /**
     * The indices for the returned boolean array in castleInfo(). At this
     * index, it will store the information about if it is a short castle.
     */
    private final int isShortCastle = 1;

    /**
     * The indices for the returned boolean array in castleInfo(). At this
     * index, it will store the information about if it is a long castle.
     */
    private final int isLongCastle = 2;

    /**
     * The board, as a two-dimensional array.
     */
    private Piece[][] board;

    /**
     * The boolean that is true if it is in testing mode. During testing mode,
     * any moves done will affect the testing board, but not the actual board
     * itself.
     */
    private boolean isTesting = false;

    /**
     * The board that is used to test.
     */
    private Piece[][] testBoard;

    /**
     * The standard setup for a chess game.
     */
    private final Piece[][] STANDARD_SETUP = {
            { new Rook(WHITE), new Knight(WHITE), new Bishop(WHITE),
                    new Queen(WHITE), new King(WHITE), new Bishop(WHITE),
                    new Knight(WHITE), new Rook(WHITE) },
            { new Pawn(WHITE), new Pawn(WHITE), new Pawn(WHITE),
                    new Pawn(WHITE), new Pawn(WHITE), new Pawn(WHITE),
                    new Pawn(WHITE), new Pawn(WHITE) },
            { null, null, null, null, null, null, null, null },
            { null, null, null, null, null, null, null, null },
            { null, null, null, null, null, null, null, null },
            { null, null, null, null, null, null, null, null },
            { new Pawn(BLACK), new Pawn(BLACK), new Pawn(BLACK),
                    new Pawn(BLACK), new Pawn(BLACK), new Pawn(BLACK),
                    new Pawn(BLACK), new Pawn(BLACK) },
            { new Rook(BLACK), new Knight(BLACK), new Bishop(BLACK),
                    new Queen(BLACK), new King(BLACK), new Bishop(BLACK),
                    new Knight(BLACK), new Rook(BLACK) } };

    /**
     * Constructs a ChessBoard object, with the standard setup.
     */
    public ChessBoard() {
        this.board = STANDARD_SETUP;
    }

    /**
     * Constructs a ChessBoard object, with the specified setup.
     * 
     * @param board
     *            The board in which to copy the setup from.
     */
    protected ChessBoard(Piece[][] board) {
        this.board = Arrays.copyOf(board, SIZE);
        for (int i = 0; i < SIZE; i++) {
            this.board[i] = Arrays.copyOf(board[i], SIZE);
        }
    }

    /**
     * Moves a piece from a position to another position (specified), if it is
     * legal.
     * 
     * @param from
     *            The "from" position
     * @param to
     *            The "to" position
     * 
     * @throws IllegalMoveException
     *             If the move is not legal.
     */
    public void move(Position from, Position to) {
        if (!isTesting) {
            Piece fromPiece = board[from.getY() - 1][from.getX() - 1];
            Piece toPiece = board[from.getY() - 1][from.getX() - 1];
            boolean[] castleInfo;
            if (fromPiece == null) {
                throw new IllegalMoveException(from, to);
            } else if (toPiece.posMoves(this, from).contains(to)) {
                int white = fromPiece.isWhite() ? 0 : 1;
                if (fromPiece instanceof Rook) {
                    boolean isAFileRook;
                    if ((isAFileRook = from.getX() == 1) || from.getX() == 8) {
                        canCastle[isAFileRook ? 1 : 0][white] = false;
                    }
                } else if (fromPiece instanceof King) {
                    for (int i = 0; i < canCastle[white].length; i++) {
                        canCastle[white][i] = false;
                    }
                }
                board[to.getY() - 1][to.getX() - 1] = fromPiece;
                board[from.getY() - 1][from.getX() - 1] = null;
            } else if ((castleInfo = castleInfo(from, to))[isCastle]) {
                int side = fromPiece.isWhite() ? 0 : SIZE - 1;
                if (castleInfo[isShortCastle]) {
                    board[side][1] = board[side][3];
                    board[side][2] = board[side][0];
                    board[side][3] = null;
                    board[side][0] = null;
                } else if (castleInfo[isLongCastle]) {
                    board[side][5] = board[side][3];
                    board[side][4] = board[side][7];
                    board[side][3] = null;
                    board[side][7] = null;
                }
            } else {
                throw new IllegalMoveException(from, to);
            }
        }
    }

    /**
     * Turns test mode on, and makes a move in test mode.
     * 
     * @param from
     *            The "from" position.
     * @param to
     *            The "to" position.
     */
    public void testMove(Position from, Position to) {
        startTest();
        testBoard[to.getY() - 1][to.getX() - 1] = getPiece(from);
        testBoard[from.getY() - 1][from.getX() - 1] = null;
    }

    /**
     * Turns test mode on.
     */
    public void startTest() {
        if (!isTesting) {
            isTesting = true;
            testBoard = Arrays.copyOf(board, SIZE);
            for (int i = 0; i < SIZE; i++) {
                testBoard[i] = Arrays.copyOf(board[i], SIZE);
            }
        }
    }

    /**
     * Turns test mode off.
     */
    public void stopTest() {
        isTesting = false;
        testBoard = null;
    }

    /**
     * Checks if all the objects in the array is null.
     * 
     * @param elements
     *            The array to be checked.
     * @return true if it is all null, false otherwise.
     */
    private <E> boolean isAllNull(E[] elements) {
        for (E element : elements) {
            if (element != null) {
                return false;
            }
        }
        return true;
    }

    /**
     * Checks if the move is a castle, and whether it is a short castle or a
     * long castle.
     * 
     * @param from
     *            The "from" position.
     * @param to
     *            The "to" position.
     * @return A boolean array with three elements: The first one is true if it
     *         is a castle, the seconds one is true if it is a short castle, and
     *         the third one is true if it is a long castle.
     */
    private boolean[] castleInfo(Position from, Position to) {
        Piece fromPiece = board[from.getY() - 1][from.getX() - 1];
        Piece toPiece = board[from.getY() - 1][from.getX() - 1];
        int isWhite = fromPiece.isWhite() ? 0 : 1;
        Position shortCastlePreventor = new Position(fromPiece.isWhite() ? 1
                : SIZE, 6);
        Position longCastlePreventor = new Position(fromPiece.isWhite() ? 1
                : SIZE, 4);
        Piece[] shortCastlePosBlock = { getPiece(shortCastlePreventor),
                getPiece(new Position(fromPiece.isWhite() ? 1 : SIZE, 7)) };
        Piece[] longCastlePosBlock = {
                getPiece(new Position(fromPiece.isWhite() ? 1 : SIZE, 4)),
                getPiece(new Position(fromPiece.isWhite() ? 1 : SIZE, 3)),
                getPiece(new Position(fromPiece.isWhite() ? 1 : SIZE, 2)) };
        boolean isShortCastle = from.getX() == to.getX() + 2
                && canCastle[0][isWhite] && isAllNull(shortCastlePosBlock)
                && !isProtected(shortCastlePreventor);
        boolean isLongCastle = from.getX() == to.getX() - 2
                && canCastle[1][isWhite] && isAllNull(longCastlePosBlock)
                && isProtected(longCastlePreventor);
        return new boolean[] {
                toPiece instanceof King && (isShortCastle || isLongCastle),
                isShortCastle, isLongCastle };
    }

    /**
     * Gets the piece at the specified position.
     * 
     * @param pos
     *            The position to get the piece from.
     * @return The piece at the specified position.
     */
    public Piece getPiece(Position pos) {
        int x = pos.getX();
        int y = pos.getY();
        if (inSize(x) && inSize(y)) {
            if (!isTesting) {
                return board[y - 1][x - 1];
            } else {
                return testBoard[y - 1][x - 1];
            }
        }
        return null;
    }

    /**
     * Checks if the given integer is in the size of the board.
     * 
     * @param i
     *            The integer to check.
     * @return true if i > 0 and i <= SIZE, false otherwise.
     */
    public boolean inSize(int i) {
        return i > 0 && i <= SIZE;
    }

    /**
     * Checks if the specified position is protected.
     * 
     * @param pos
     *            The position to check.
     * @return true if it is safe, false otherwise.
     */
    public boolean isProtected(Position pos) {
        startTest();
        int x = pos.getX();
        int y = pos.getY();
        testBoard[y - 1][x - 1] = null;
        for (int i = 1; i <= SIZE; i++) {
            for (int j = 1; j <= SIZE; j++) {
                Position posThreat = new Position(i, j);
                Piece possibleThreat = getPiece(posThreat);
                if (possibleThreat != null
                        && possibleThreat.posMoves(this, posThreat).contains(
                                pos)) {
                    stopTest();
                    return false;
                }
            }
        }
        stopTest();
        return true;
    }

}

Many of the board's methods require you to pass a Position object as a parameter, whether you are calling getPiece(Position pos), or if it is move(Position from, Position to). A position simply is two integers, stored in an object:

public class Position {

    private final int x;
    private final int y;

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

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    @Override
    public String toString() {
        String result = "";
        int modifiableX = x;
        while (true) {
            if (modifiableX > 26) {
                result += (char) ((x % 26) + 64);
                modifiableX /= 26;
            } else {
                result += (char) (x + 64) + Integer.toString(y);
                return result;
            }
        }
    }

    @Override
    public boolean equals(Object obj) {
        if (!(obj instanceof Position)) {
            return false;
        }
        Position other = (Position) obj;
        return this.x == other.x && this.y == other.y;
    }

}

The main purpose of the Position class is to not have annoying methods like this:

move(int fromX, int fromY, int toX, int toY) {
    // ...
}

The move() method can throw an IllegalMoveException:

public class IllegalMoveException extends RuntimeException {

    private static final long serialVersionUID = 5302600450519015413L;

    /**
     * Constructs a new IllegalMoveException, with the specified positions.
     * 
     * @param from
     *            The position where the piece that is attempting to move is at.
     * @param to
     *            The position where the piece is attempting to move.
     */
    public IllegalMoveException(Position from, Position to) {
        super("Move from " + from.toString() + " to " + to.toString()
                + " is not possible.");
    }

}

Questions:

• Is it well-designed?
• Does it smell?
• Is the IllegalMoveException necessary?

Answer 1

I liked reading your code and thinking about it. While trying to find out what to improve I almost started doing all the refactoring that came to mind. But I stopped myself thinking it might be better if I describe what I think could be improved and you try factoring those ideas into your code.

What I think is very good style is that you try to avoid most mutable state. Lots of final variables, etc. Nice.

However, here are some tips for improvement.

Long classes

The first thing I noted when I opened your code in my editor is that ChessBoard is very long. It seems to be responsible for at least (actually more) two things: Modelling the board, on which pieces can be placed and moved around.

The other responsibility is modeling the game rules. Putting just the board mechanics in a seperate class is the first thing I would change in the code. Stuff like this would become much easier to read:

Piece fromPiece = board[from.getY() - 1][from.getX() - 1];

would be

Piece fromPiece = board.pieceAt(fromPosition);

Try to think like this: Suppose we have the classes Board, ChessGame and Position. As a ChessGame, I don't want to know about the internals of Position. I don't care if it is (x,y) tuples or 3 dimensional or polar coordinates. Ideally I just want to pass a Position to a Board and get the Piece.

Try to code as if you wanted the least amount of information possible in your current scope.

Bob Martin, the author of "Clean Code" coined the term single responsibility principle, which in my opinion is one of the most important things to consider when writing more complex code.

Don't abuse booleans, we have a type system

Booleans are booleans, yes or no, true or false. If you have some information that should be stored and can only have a certain set of values this means you have a new type. It is much better to introduce a new enum Player with the values BLACK and WHITE even if it only has two values. Something like this:

    enum Player {
            WHITE("White"), BLACK("Black");

            private final String stringRepresentation;

            Player(String stringRepresentation) {
                    this.stringRepresentation = stringRepresentation;
            }

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

Doing this, it is also easier to follow the Tell, don't ask principle. Instead of saying

new ImageIcon((isWhite ? "White " : "Black ") + NAME)

which translates to "Hey boolean, what are you? Oh you're a 'true'? So, based on the information about your state I'm gonna decide to use the String "White", you just say

new ImageIcon(player.toString())

which translates to "Just give me your name." Try to avoid making decisions based on information you receive from an object. Removing conditionals from your code in this way improves the readability and removes tight coupling. This is also true for switch and if statements. Clean code tends to have less of them and rely more on the type system and object interactions.

The next candidate for enums would be your castle code. Using arrays as tuples can be very confusing at times. Just make a new small class or better, use an enum. Such a class would also be a nice place to put the whole castle check logic in.

Code Duplication

Try to extract duplicated code into one place. One example is that in a lot of places you manually subtract 1 to get array indices. The following could easily be put in its own method:

board[to.getY() - 1][to.getX() - 1]

like this:

pieceAt(toPosition)

or as I said before, better in its own class.

Another form of duplication can be found in ChessBoard.castleInfo(): In this part you have a lot calls to the same method, wrapped in a ternary expression, which makes it very hard to read: fromPiece.isWhite() ? 1 : SIZE. You could factor that out into a local variable, say row. With this your Position instantiations look more like new Position(row, 3) or something like that.

Put logic where its data resides

This might be the most important thing. When reading code that contains lots of low level calculations (like adding indices, ternary expressions, etc.) your mind always has to make the effort to switch between abstaction levels. One moment you think about high level constructs (the chess rules) and in the next line you have to try to understand how the indices are mapped to pieces in some array.

Try to put code that operates on data in the class that holds the data. It is possible and also desirable to have code with less or often even no get*() methods. Consider this (also in ChessBoard.castleInfo(), shortened a little):

boolean isShortCastle = from.getX() == to.getX() + 2; 

Instead of asking for coordinates and then doing calculations with this data, calculate it in the class that holds the data, i.e. the Position class:

boolean isShortCastle = from.xDistanceTo(to) == 2;

or even something like:

boolean isShortCastle = from.distanceTo(to).equals(new Distance(2,0));

this might look over-engineered at first, but the Distance class could also model a move in the game (The moves in your Knight class for example). That way you could just do

Distance knightMove = new Distance(1,2)
Position newPosition = position.move(knightMove);

These are just ideas, so try to create different abstractions and to find out which one suits your problem best. Maybe something like this makes more sense in your case:

Move knightMove = new Move(1,2);
Position newPosition = knightMove.apply(oldPosition);

Again, the general idea is to have as little information (accessible variables, knowledge about state, logic, etc.) in your scope as possible. Don't be afraid to create heaps of small classes, just use sub-packages to group them.

Always implement hashCode() if you implement equals()

See this post on why.

I saw more things in your code I'd love to give some feedback on, but maybe I'll give you some time to consider the stuff I've written so far. This post might become too long otherwise. Have fun hacking!

Answer 2

1. BLACK and WHITE should be enums instead of boolean constants.

2. Returning an array from the castleInfo method and using constants to get information out of it doesn't look good. I would create a separate class for this purpose. I would also rename the castleInfo method into agetCastleInfo. It is conventional for a method to name an action(if it does not check a predicate). Something like this:

   class CastleInfo {
       public boolean isCastle;
       public boolean isShortCastle;
       public boolean isLongCastle;
   
       // Here should be a constructor.
    }
   
    ... 
   
   private CastleInfo getCastleInfo(Position from, Position to) {
       ...
   }
   

3. Having an IllegalMoveException is fine, but I would also add a method that checks if a move is valid and returns true or false without throwing an exception or making an actual move.