2

I am trying to design a Chess game where each piece of the game inherits from an abstract Piece class. The piece just does a few things:

  • It tells which color it is (White / Black)
  • It tells what type it is
  • It tells if a move is valid or not
  • and maybe what cell / square on the board it is currently at

Earlier I was thinking of making the validation a part of the piece itself so that there would be an abstract validateMove() method that each piece would have to implement, but then this might lead to a lot of code changes if, in future, I decide to change the move validation mechanism of a specific piece, something similar to the case of Rubber duck with no fly method described in the Head First Design book.

Hence, I thought of creating this interface called MoveValidator, which would have piece-specific implementations and each piece would have a validator as one of its variables rather than being a validator itself or implementing the validation itself.

So a Pawn would have a PawnMoveValidator as one of its composition.

So I have the following abstract Piece class

@RequiredArgsConstructor
public abstract class Piece {

    @NonNull
    @Getter
    private final PieceColor pieceColor;

    @NonNull
    @Getter
    private final MoveValidator moveValidator;

    @Getter
    @Setter
    private Point currentLocation;


    private boolean basicValidation(Cell[][] board, Point src, Point dest) {
        .........
        .........
    }


    public boolean validateMove(Cell[][] board, Point src, Point dest) {
        return basicValidation(board, src, dest) && moveValidator.validateMove(board, src, dest);
    }

}

Cell.java

@RequiredArgsConstructor
public class Cell {

    @NonNull
    @Getter
    private final CellColor cellColor;

    @NonNull
    @Getter
    private final Integer row;

    @NonNull
    @Getter
    private final Integer col;
}

Point.java

@RequiredArgsConstructor
public class Point {

    @NonNull
    @Getter
    private final Integer x;

    @NonNull
    @Getter
    private final Integer y;
}

MoveValidator.java

public interface MoveValidator {

    validateMove(Cell[][] board, Point src, Point dest);
}

So I have a few questions here.

  1. Is this a good design or can this be improved further?
  2. If the Piece doesn't have any abstract method to implement, should it still remain an abstract class or should it become a concrete class?
  3. What if the move validation of a specific piece requires the knowledge of the state of that specific piece for which it's implemented? For ex: The pawn wants to move two cells ahead. The Pawn class has a flag telling whether it's the first move or not. In that case, if the PawnMoveValidator wanted to validate the move, it would need to know the state of the flag. How can that be possible without making the Pawn class implement the validator itself?

Thanks for your time

1

This a good design or can this be improved further?

For me, a design can always be improved. At least I see some problem with the movement that cannot be solved using the current design.

If the Piece doesn't have any abstract method to implement, should it still remain an abstract class or should it become a concrete class?

If a class does not have any abstract methods, what prevent it from being instantiable? If we want to reuse some pieces of code, use composition. If we want to satisfy some typing constraints, just use interfaces. Personally, I only use abstract class when I have some abstract methods there.

What if the move validation of a specific piece requires the knowledge of the state of that specific piece for which it's implemented? For ex: The pawn wants to move two cells ahead. The Pawn class has a flag telling whether it's the first move or not. In that case, if the PawnMoveValidator wanted to validate the move, it would need to know the state of the flag. How can that be possible without making the Pawn class implement the validator itself?

Unfortunately, there are many things take part in the process of deciding whether a move is valid:

Some rules need information about the current state of the board:

  • Is the target cell a possible move of the current piece?
  • Is the target cell empty?
  • Are there any other pieces block the move?

Some rules need information about the current piece and maybe the related piece (in castling):

  • Is this the first move of the pieces (in case of pawns, kings and rooks)?
  • Is the pawn promoted?

And as you can see, a move is not only moving one piece from one cell to another cell. In castling, there are two pieces in one move.

In summary, the current design is quite clear to me. However, you may need more work to model a complete chessboard with all of its rules. I have some suggestions:

  • Making the "move" a separate class (or a family of classes).
  • The move validators has the Board as their private member and accept a Move object in the validate method.
|improve this answer|||||
1

You're making the design significantly more complicated then it needs to be. The board handles move validation, not the pieces. This is because a piece only knows what kind of piece it is. It doesn't know where it is, or what the board looks like.

What benefits do we get? The code ends up being a lot simpler.

  • We don't need a class for each kind of piece
  • We don't need a MoveValidator, just a short function
  • We don't need a Cell class
  • We don't have to update a piece every time we move it; we only have to update the board

Location is not a quality intrinsic to a piece; it's something the piece only has in relation to the board. Don't ask for the location of a piece; ask for the piece at a location on the board. This is an important distinction.

  • Pieces don't have a location when they're captured
  • The rules of chess assume that pieces of the same type and color are identical.

One of the conditions for ending a game of chess is that the chessboard enters the same state 3 times. This results in a Draw.

The simplest way to correctly implement this rule is to keep track of all previous states of the board. This is simpler than it sounds - if it's easy to make a copy of the board, you can just save a copy of the board before every move. When checking this condition, all we have to do is compare the current state to the list of previous states.

If two horses of the same color switch positions on the board, but nothing else changes, the board is still the same. That means we shouldn't consider those horses to be different to begin with.

Together, this means less files, fewer lines of code, and better performance.

We can re-write the code like this:

public class Piece {
    public enum Color { 
        Black, White 
    }
    public enum Type {
        Pawn, King, Queen, Rook, Knight, Bishop
    }
    @NonNull
    @Getter
    private Color color;
    @NonNull
    @Getter
    private Type type;

    public Piece(Color color, Type type) {
        this.color = color;
        this.type = type;
    }
    public Piece(Piece p) {
        this.color = p.color;
        this.type = p.type;
    }

    @Override
    public boolean equals(Object o) {
        Piece p = (Piece)o;
        return p.color == color && p.type == type;
    }

    // Gets the point value of the piece
    public int pointValue() {
        switch(type) {
            case Pawn: return 1;
            case King: return 1000;
            case Queen: return 9;
            case Rook: return 5;
            case Knight: return 3;
            case Bishop: return 3;
        }
    }
    // Check if a move between two points is legal for the piece under
    // normal circumstances
    // X is the column, Y is the row. X and Y are from 0 to 7
    public boolean moveLegal(int startX, int startY, int endX, int endY) {
        int dx = endX - startX; 
        int dy = endY - startY;
        // If the color is black, flip how the board looks
        // So that the home row is always 0 and the row with pawns is 1
        if(color == Black) {
            startY = 7 - startY; 
        }

        // You can't move a piece off the board
        if(endX < 0 || endX > 7) return false;
        if(endY < 0 || endY > 7) return false;

        // Not moving a piece isn't a legal move
        if(dx == 0 && dy == 0) 
            return false; 

        // If the change in position is always positive it simplifies the math
        if(dx < 0) dx = -dx;
        if(dy < 0) dy = -dy;

        switch(type) {
            case Pawn:
                // A pawn can move forward 1, unless it's in the first row
                return dy == 1 || (dy == 2 && startY == 1);
            case Rook: 
                // It can move horizontally or vertically, but not both
                return (dx != 0 && dy == 0) || (dx == 0 && dy != 0);
            case Knight:
                // A night can do a diagonal hop
                return (dx == 1 && dy == 2) || (dx == 2 && dy == 1);
            case King:
                return dx == 1 || dy == 1;
            case Queen: 
                 return dx == 0 || dy == 0 || dx == dy; 
            case Bishop:
                 return dx == dy; 
        }
    }
}

As you can see, a piece still knows how to move, but it doesn't need anMoveValidator or any other complex solution. You don't need separate classes for different types of pieces, and you don't even need a piece to know it's current location. The board will take care of that with just a few simple member functions:

public class Board {
    // Set up board in constructor
    Piece[][] pieces;
    private boolean bad(int index) {
        return index < 0 || index > 7;
    }
    public boolean isEmpty(int x, int y) {
        return pieces[x][y] == null; 
    }
    public boolean moveValid(int startX, int startY, int endX, int endY) {
    // If the move is off the board, it's invalid
       if(bad(startX) || bad(startY) || bad(endX) || bad(endY)) 
             return false;

        // You can't move where there is no piece
        if(isEmpty(startX, startY)) 
            return false;

        Piece piece = pieces[startX, startY];
        // Check that the endpoint doesn't contain a piece of the same color
        if(!isEmpty(endX, endY))
            if(pieces[endX][endY].getColor() == piece.getColor())
                return false; 

        // If the piece can't move that way, it's not valid
        if(!piece.moveLegal(startX, startY, endX, endY))
            return false; 
        // To-do: check that cells in-between are empty
    }
}        
|improve this answer|||||
  • What if the destination square is blocked by another piece? Or you have a piece which is pinned to the king by a Queen/Rook/Bishop? Or the king is in Check and the piece being moved doesn't remove the Check situation? The movement rules in chess aren't just about where a piece can move, the wider context about what's happening elsewhere on the board is important, and in some cases, previous moves are important too - for example, En passant and castling. Having a tight relationship between movement rules and individual pieces doesn't really work – Ben Cottrell Dec 9 '19 at 12:51
  • @Perez, as mentioned in my answer, the rule of chess is not as simple as checking whether a piece can move vertically, horizontally or diagonally. There are rules that are applied to only knights (not blocked by other pieces), pawns (being promoted, en passant, starting move), kings (caslting, checked) and rooks (castling). The OP does not make the problem overcomplicated but you think the problem too simple. – Hieu Le Dec 9 '19 at 14:19

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.