0
\$\begingroup\$

I was trying to design a chess game in Java. I have just started learning design patterns and am not sure if my approach here is right. Could you please suggest ways to better this design ? Below is how the code looks like :

public enum SquareState {
    OCCUPIED, EMPTY
}
public enum Color {
    BLACK, WHITE 
}
public class ChessBoard {
    public static final int MAX_ROWS = 8;
    public static final int MAX_COLS = 8;
    public static final int NUM_SQUARES = 64;
    private Square squares[][];
    private List<Piece> whitePieces;
    private List<Piece> blackPieces;
    private Map<Square, Piece> occupiedSquares;

    public Piece pieceAt(Square square) {
        return occupiedSquares.get(square);
    }
}
//Square class
public class Square {
    private int row, col;
    private Color color;
    private SquareState state;
}
//Piece class -not an abstract class(do I need one, I already have PieceType)
public class Piece {
    private PieceType type;
    private Color color;
    private Square position;

    public void move(ChessBoard chessBoard, Square dest) {
        //place the piece from its position to dest on the board
    }
}
public class Move {
    private MoveType moveType;  //undo operation will require move type
    private Square source;
    private Square destination;
    private Piece sourcePiece;  //at source 
    private Piece killedPiece; //at dest
    private Move previous, next;
}
public class Player {
    private boolean isWhite;
    private  ChessBoard chessBoard;
    private MovementGenerator movementGenerator;
    private List<Piece> alivePieces;

    public void makeMove() {
        Piece chosenPiece = alivePieces.get(0);
        List<Square> squares = movementGenerator.getPossibleMoves(chessBoard, chosenPiece);
        chosenPiece.move(chessBoard, squares.get(0));
    }
}
public class MovementGenerator {
   private MovementStrategy movementStrategy;
   private MovementStrategyResolver movementStrategyResolver;

   public MovementGenerator(MovementStrategy movementStrategy) {
       this.movementStrategy = movementStrategy;
   }

   public List<Square> getPossibleMoves(ChessBoard chessBoard, Piece piece) {
       return movementStrategyResolver.resolveStrategy(piece).getPossibleMoves(chessBoard, piece);
   }

}
public interface MovementStrategy {
    public List<Square> getPossibleMoves(ChessBoard chessBoard, Piece piece);
}
public class BishopMovementStrategy implements MovementStrategy{
    @Override
    public List<Square> getPossibleMoves(ChessBoard chessBoard, Piece piece) {
        // TODO Auto-generated method stub
        return null;
    }   
}
public class KnightMovementStrategy implements MovementStrategy {

    @Override
    public List<Square> getPossibleMoves(ChessBoard chessBoard, Piece piece) {
        // TODO Auto-generated method stub
        return null;
    }

}
//A class to validate if move made by the player is a legal one

public class MovementValidator {

    private MovementGenerator movementGenerator;

    public boolean isValidMove(ChessBoard board, Move move) {
        Square source = move.getSource();
        Square dest = move.getDestination();
        if (outsideBoard(source) || outsideBoard(dest))
            return false;
        // can't kill own pieces
        if (board.pieceAt(source).getColor() == board.pieceAt(dest).getColor())
            return false;
        // if dest empty then proceed(should also check for checkmate condition
        // in this case)
        if (dest.getState() == SquareState.EMPTY)
            return true;
        // try to kill the opponent
        List<Square> possiblePositions = movementGenerator
                .getPossibleMoves(move.getSourcePiece());
        if (!possiblePositions.contains(dest))
            return false;
        return true;
    }

    private boolean outsideBoard(Square square) {
        int x = square.getRow();
        int y = square.getCol();
        if (x < 0 || x > 8 || y > 0 || y > 8)
            return false;
        return true;
    }
}

Also, isValidMove() method of MovementValidator class should get called as soon as a player makes a move and should flag an error if the move made is illegal. How can I enforce this in the above code(given that validating the legality of a move shouldn't be the responsibility of the Player class)?

Alternatively, should I make Piece as a base class and extend it for King, Queen, Bishop, Rook, Pawn and let all these classes have reference to concrete MovementStartegy objects? Would that be a better design?

\$\endgroup\$

closed as off-topic by vnp, 200_success, Toby Speight, Ludisposed, Graipher Jul 17 '18 at 9:31

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – vnp, 200_success, Toby Speight, Ludisposed, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ Welcome to Code Review. This code is, in my opinion, too sketchy to review. Since it is unfinished, it's hard to tell what parts of the code are real, what parts are hypothetical, and what parts are broken. \$\endgroup\$ – 200_success Jul 17 '18 at 4:11
1
\$\begingroup\$

Your Piece class should be abstract and you should extend all pieces to it. You can get rid of your conventional MovementValidator and MovementGenerator classes this way. Shorter code is better as long as it's also done conventionally.

public abstract class Piece {
    private PieceType type;
    private Color color;
    private Square position;

    /**
    * @param move to be checked
    * @return if move is valid
    */
    public abstract boolean isValidMove(Move move);

    /**
    * @return List of moves
    */
    public abstract List<Move> getValidMoves();

    public void move(ChessBoard chessBoard, Square dest) {
        //place the piece from its position to dest on the board
        Move move;

        /*Instantiate move*/

        if (validMove(move)){
            //Make the move
        }
    }
}

As a silly suggestion, since you are using a Color object for certain checks see if you can implement more colors than just black and white. No point in programming if you can't have a bit of fun.

\$\endgroup\$
  • \$\begingroup\$ getValidMovements needs the board to determine whether calling is still allowed and to check for en passant captures. \$\endgroup\$ – Roland Illig Jul 17 '18 at 1:44

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