Simple Chess Game, Part 2: The Pieces

Question

As I was writing my first part of my Chess question, I realized that I couldn't fit all my code into one question (I exceeded the character limit), so I will continue here.

The pieces all share the same structure, and I had to have a way of storing them together, so I created the Piece interface:

import java.util.List;

import javax.swing.Icon;

public interface Piece {

    /**
     * Returns the possible moves for the piece, with the given board and position.
     * 
     * @param board The board.
     * @param pos The position in which the piece is located in the board.
     * 
     * @return All the possible moves the piece can do.
     */
    public List<Position> posMoves(ChessBoard board, Position pos);

    /**
     * Gets the value of the piece.
     * 
     * @return The value of the piece.
     */
    public int value();

    /**
     * Gets the name of the piece.
     * 
     * @return The name of the piece.
     */
    public String name();

    /**
     * Gets the image that this piece represents.
     * 
     * @return The image corresponding to the piece.
     */
    public Icon image();

    /**
     * Checks if the piece is white.
     * 
     * @return true if this piece is white, false otherwise.
     */
    public boolean isWhite();

}

The Piece interface is then implemented by all the Pieces.

The King class is probably the most complicated of all pieces. The posMoves() method makes sure the King does not step right into the line of fire, by checking every possible danger in every possible move:

import java.util.LinkedList;
import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class King implements Piece {

    /**
     * The value of a king.
     */
    private static final int VALUE = 15;

    /**
     * The name of the piece.
     */
    private static final String NAME = "King";

    /**
     * The possible moves of the piece.
     * 
     * In each array in the array, there will be two integers. One represents x,
     * and the other y.
     */
    private static final int[][] posMoves = { { 1, 1 }, { 1, 0 }, { 1, -1 },
            { -1, 0 }, { -1, 1 }, { -1, -1 }, { 0, 1 }, { 0, -1 } };

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new King object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public King(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new LinkedList<Position>();
        int x = pos.getX();
        int y = pos.getY();
        for (int[] posMove : posMoves) {
            int posX = x + posMove[0];
            int posY = y + posMove[1];
            if (board.inSize(posX) && board.inSize(posY)) {
                Position possibleMove = new Position(x, y);
                if (isSafe(board, pos, possibleMove)) {
                    result.add(possibleMove);
                }
            }
        }
        return result;
    }

    /**
     * Checks if the position at pos is safe for the piece at currentPos.
     * 
     * @param board
     *            The board.
     * @param currentPos
     *            The position where the piece is.
     * @param pos
     *            The position to check.
     * @return true if pos is safe, false otherwise.
     */
    private boolean isSafe(ChessBoard board, Position currentPos, Position pos) {
        board.testMove(currentPos, pos);
        for (int i = 1; i <= ChessBoard.SIZE; i++) {
            for (int j = 1; j <= ChessBoard.SIZE; j++) {
                Position posOfThreat = new Position(i, j);
                Piece possibleThreat = board.getPiece(posOfThreat);
                if (possibleThreat != null
                        && !possibleThreat.posMoves(board, posOfThreat)
                                .contains(pos)) {
                    return false;
                }
            }
        }
        board.stopTest();
        return true;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

Then the Rook class, which does the following:

1. Creates a counter and starts moving it in one direction.
2. Each square it lands on:
   • If it is empty, add the square in the results list, and continue.
   • If it is a piece of the same colour, break immediately.
   • If it is a piece of opposite colour, add the square in the results list, and break.
3. Repeat for all the other directions.

---

import java.util.LinkedList;
import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class Rook implements Piece {

    /**
     * The value of a rook.
     */
    private static final int VALUE = 5;

    /**
     * The name of the piece.
     */
    private static final String NAME = "Rook";

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new Rook object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public Rook(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new LinkedList<Position>();
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() + i;
            int y = pos.getY();
            if (x <= ChessBoard.SIZE) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX();
            int y = pos.getY() + i;
            if (y <= ChessBoard.SIZE) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() - i;
            int y = pos.getY();
            if (x > 0) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX();
            int y = pos.getY() - i;
            if (y > 0) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        return result;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

Next, the Bishop, which is very similar to the Rook:

import java.util.LinkedList;
import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class Bishop implements Piece {

    /**
     * The value of the bishop.
     */
    private static final int VALUE = 3;

    /**
     * The name of the piece.
     */
    private static final String NAME = "Bishop";

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new Bishop object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public Bishop(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new LinkedList<Position>();
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() + i;
            int y = pos.getY() + i;
            if (x <= ChessBoard.SIZE && y <= ChessBoard.SIZE) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() + i;
            int y = pos.getY() - i;
            if (x <= ChessBoard.SIZE && y > 0) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() - i;
            int y = pos.getY() + i;
            if (x > 0 && y <= ChessBoard.SIZE) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        for (int i = 1, max = ChessBoard.SIZE; i < max; i++) {
            int x = pos.getX() - i;
            int y = pos.getY() - i;
            if (x > 0 && y > 0) {
                Position posMove = new Position(x, y);
                if (board.getPiece(posMove) == null) {
                    result.add(posMove);
                } else if (board.getPiece(posMove).isWhite() ^ !isWhite) {
                    max = i;
                } else {
                    result.add(posMove);
                    max = i;
                }
            } else {
                max = i;
            }
        }
        return result;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

The Queen pretty much is a combination of a Rook and a Bishop:

import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class Queen implements Piece {

    /**
     * The value of a queen.
     */
    private static final int VALUE = 9;

    /**
     * The name of the piece.
     */
    private static final String NAME = "Queen";

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new Queen object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public Queen(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new Bishop(isWhite).posMoves(board, pos);
        result.addAll(new Rook(isWhite).posMoves(board, pos));
        return result;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

Next, the Knight, which is probably the easiest, as it can jump over piece:

import java.util.LinkedList;
import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class Knight implements Piece {

    /**
     * The value of a knight.
     */
    private static final int VALUE = 3;

    /**
     * The name of the piece.
     */
    private static final String NAME = "Knight";

    /**
     * The possible moves of the piece.
     * 
     * In each array in the array, there will be two integers. One represents x,
     * and the other y.
     */
    private static final int[][] posMoves = { { 1, 2 }, { -1, 2 }, { 1, -2 },
            { -1, -2 }, { 2, 1 }, { 2, -1 }, { -2, 1 }, { -2, -1 } };

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new Knight object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public Knight(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new LinkedList<Position>();
        for (int[] posMove : posMoves) {
            int posX = pos.getX() + posMove[0];
            int posY = pos.getY() + posMove[1];
            Position possibleMove = new Position(posX, posY);
            if (posX <= ChessBoard.SIZE
                    && posY <= ChessBoard.SIZE
                    && posX > 0
                    && posY > 0
                    && (board.getPiece(possibleMove) == null || (board
                            .getPiece(possibleMove).isWhite() ^ isWhite))) {
                result.add(possibleMove);
            }
        }
        return result;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

Finally, the pawn:

import java.util.LinkedList;
import java.util.List;

import javax.swing.Icon;
import javax.swing.ImageIcon;

public class Pawn implements Piece {

    /**
     * The value of a pawn.
     */
    private static final int VALUE = 1;

    /**
     * The name of the piece.
     */
    private static final String NAME = "Pawn";

    /**
     * The colour of the piece.
     */
    private final boolean isWhite;

    /**
     * The image representing the piece.
     */
    private final Icon image;

    /**
     * Constructs a new Pawn object.
     * 
     * @param isWhite
     *            The colour of the piece.
     */
    public Pawn(boolean isWhite) {
        this.isWhite = isWhite;
        this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
                + ".JPG");
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public List<Position> posMoves(ChessBoard board, Position pos) {
        // TODO En passant
        List<Position> result = new LinkedList<Position>();
        // First move possible 2 steps
        Position posMove = new Position(pos.getX(), pos.getY()
                + (isWhite ? 2 : -2));
        Position posMove2 = new Position(pos.getX(), pos.getY()
                + (isWhite ? 1 : -1));
        if (pos.getY() == (isWhite ? 2 : ChessBoard.SIZE - 2)
                && board.getPiece(posMove) == null
                && board.getPiece(posMove2) == null) {
            result.add(posMove);
        }
        if (board.getPiece(posMove2) == null) {
            result.add(posMove2);
        }
        // Capture
        posMove = new Position(pos.getX() + 1, pos.getY() + (isWhite ? 1 : -1));
        posMove2 = new Position(pos.getX() - 1, pos.getY() + (isWhite ? 1 : -1));
        if (board.getPiece(posMove) != null
                && (board.getPiece(posMove).isWhite() ^ isWhite)) {
            result.add(posMove);
        }
        if (board.getPiece(posMove2) != null
                && (board.getPiece(posMove2).isWhite() ^ isWhite)) {
            result.add(posMove2);
        }
        return result;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public int value() {
        return VALUE;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public String name() {
        return NAME;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public Icon image() {
        return image;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean isWhite() {
        return isWhite;
    }

}

Questions:

• Does it connect well with my ChessBoard class?
• Does it smell?
• Am I using JavaDoc correctly and well?
• Is it as efficient as it could be?

Answer 1

First observation: you should probably be thinking about separating the model of the game from the presentation of the game.

For example:

public interface Piece {
    public List<Position> posMoves(ChessBoard board, Position pos);
    public Icon image();

posMoves is about the game in the abstract - that's clearly a representation of the rules of the game (applied to the specific state). But a javax.swing.Icon ? That only makes sense in a presentation layer that is specifically using swing icons for something. So you clearly have an interface here that is representing two very different concerns; which in turn is going to make it hard to create good modules, tests, and so on.

(BTW: pos is not a verb. This should be getMoves(), or calculateMoves(), or....)

public class King implements Piece {

    private static final int VALUE = 15;
    private static final String NAME = "King";
    private static final int[][] posMoves = { { 1, 1 }, { 1, 0 }, { 1, -1 },
        { -1, 0 }, { -1, 1 }, { -1, -1 }, { 0, 1 }, { 0, -1 } };
    private final boolean isWhite;
    private final Icon image;

First thing to call out here is that - more than likely - every piece class is going to share these properties. That suggests that there should be a common base class that stores this information.

That shouldn't be to much of a surprise, if you think about your description for a moment -- "The posMoves() method makes sure the King does not step right into the line of fire" What this is really saying is that the king is a piece that has a specialized behavior for testing whether a move to a square is legal (because the king can't move into check).

I think you'll want to give some thought to your enumeration of moves. You are expressing them all as int-pairs, which is reasonable for piece moves, but that idiom doesn't work at all for en passant captures or castling. I think you'll be better served by actually teasing out a Move interface. Each piece would have a collection of moves to test. The int pairs become a RelativePositionMove, but the rules for more complicated situations become EnPassantCaptureMove, and StrongSideCastleMove, and so on. The piece logic then becomes a lot easier to manage.

Did you notice that your implementation of the possible moves for King included all nine directions, without worrying about the position on the board? There's no reason that you couldn't do that for Rook, Bishop, Queen, and Knight just as easily.

class Queen ... {
    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = new Bishop(isWhite).posMoves(board, pos);
        result.addAll(new Rook(isWhite).posMoves(board, pos));
        return result;
    }
}

It's pretty bad that to figure out how a queen moves, you are creating new pieces every time. A better implementation here would be to re-use the rook and bishop rules each time...

public class Queen implements Piece {
    private final Rook fakeRook;
    private final Bishop fakeBishop;

    public Queen(boolean isWhite) {
        this.fakeRook = new Rook(isWhite);
        this.fakeBishop = new Bishop(isWhite);
        // ...
    }

    public List<Position> posMoves(ChessBoard board, Position pos) {
        List<Position> result = fakeBishop.posMoves(board, pos);
        result.addAll(fakeRook.posMoves(board, pos));
        return result;
    }

I think doing it this way helps expose the fact that we aren't really talking about pieces here, but the rules for the pieces; again - it's another hint that the abstractions aren't quite right.

But if I ran the zoo, I'd have a canonical set of rook moves, and a canonical set of bishop moves, and the queen would have a set of rules that was the join of these two sets, rather than having the queen pretend to be two different kinds of pieces.

(See wikipedia for another hint that the queen should not be treated as a combination bishop and rook).

/**
 * The colour of the piece.
 */
private final boolean isWhite;

Umm, true isn't a colour. Neither is false. You should be using an enumeration here, rather than a boolean.

    this.image = new ImageIcon((isWhite ? "White " : "Black ") + NAME
            + ".JPG");

This is screaming for an IconFactory, which takes a name and color and returns the correct Icon.

Answer 2

Does it smell?

Regarding the general design:

• You are mixing the view and the model, which makes your code inflexible, and makes it harder to find the relevant pieces of code.
• the loading of the image icon seems to always be the same, I would create an abstract piece class and move it there.
• same goes for the getters.
• A boolean for the color is not optimal, use an enum instead.
• You are missing a Move class. Your posMoves method returns a position instead, which seems a bit odd.
• I'm not sure how much I like the idea of having a test mode inside the board model. I think I would prefer to have a simple deep copy method instead.

Regarding the implementation:

• Many of your posMoves functions are way too long and complex. 4 long for loops after another, without even a comment about what each one does is way too much. I would extract each loop into its own function, and give the function a sensible name.
• There is also a lot of duplicate code in those loops. One generic function with one loop accepting the direction and the boolean question as argument can be used instead of having 4 nearly identical loops.
• If you put this generic function inside an abstract piece class you should be able to reuse it for bishop and rook (although it is possible that all those extra function calls decrease performance too much, in which case your copy-paste solution is ok, just add comments).

Am I using JavaDoc correctly and well?

I would say so, just a couple of small points:

• You are missing class level comments.
• the meaning of the posMoves field is not immediately obvious, and the comment doesn't really help clear things up. Maybe add something like: [...] One represents how far the piece can move in what x direction, and the other [...].
• it's not immediately clear what safe in isSafe means (at first I thought it had something to do with AI), and the comments don't clear this up.

Misc

• don't shorten names. calculatePossibleMoves is a lot more readable than posMoves (which could also stand for positionMoves or something), and it also expresses that it is an expensive operation (not a simple get).
• getters should start with get (eg name -> getName, etc).
• don't use bitwise operations. They are hard to read, and the minimal performance gain is not worth it.
• Is the value of a piece really relevant for the game? It seems more relevant for an AI, and I wouldn't mix AI code with the general game code.
• the whole thing with max and max = i is confusing. I would just use break (or if you extracted the loops to functions, return).