6
\$\begingroup\$

I have written code for making a Chess Validator, which will validate the moves of a chess game. There is a Main class which is taking the input from the user, about the move.

Problem Description

The board is maintained in the form of a 2D matrix, which contains the pieces as {color}{type}.

Eg. BH : black horse.

Pieces:

  • K: king,
  • Q: queen,
  • P: pawn,
  • R: rook,
  • C: bishop,
  • H: horse

A Move is defined as {color}{type} {src i}{src j} {dest i}{dest j} where \$i\$ denotes row and \$j\$ gives column.
Eg. BP 12 22, which move black pawn from \$(1,2)\$ to \$(2,2)\$.

The Implementation

Piece is taken as an abstract class, which is being implemented by the different pieces King, Rook, Queen, Bishop, Horse, Pawn. A Piece has a basic validation rule defined as common. All pieces have their own specific move, and a validation specific to the piece type.
Bishop move is still missing in code.

Using Factory design pattern, the object of the specific piece is created, and further movements and validations are performed. Board allows its default, as well as user defined initialization.

Piece class

public abstract class Piece {

private char color;

public char getColor() {
    return color;
}

public void setColor(char color) {
    this.color = color;
}


public abstract boolean validateForPiece(int srcX, int srcY, int destX, int destY);

public void move(int srcX, int srcY, int destX, int destY, String piece) {
    Board.CHESS_BOARD[destX][destY]=piece;
    Board.CHESS_BOARD[srcX][srcY]="--";
}

public boolean checkMiddlePieces(int srcX, int srcY, int destX, int destY) {
    if(srcX==destX && srcY!=destY) {
        for(int i=srcY+1;i<destY;i++) {
            if(Board.CHESS_BOARD[destX][i]!="--")
                return false;
        }
    }
    else if(srcX!=destX && srcY==destY) {
        for(int i=srcX+1;i<destX;i++) {
            if(Board.CHESS_BOARD[i][destY]!="--")
                return false;
        }
    }
    else if(Math.abs(srcX-destX)==Math.abs(srcY-destY)) {
        for(int i=srcX+1,j=srcY+1;i<destX && j<destY;i++,j++) {
            if(Board.CHESS_BOARD[i][destY]!="--")
                return false;
        }
    }
    return true;
}

public boolean validateFirst(int srcX, int srcY, int destX, int destY, String piece) {
    char color = piece.charAt(0);
    if(srcX>=8 || destX>=8 || srcY>=8 || destY>=8) {
        System.out.println("Exception handled");
        return false;
    }
    if(Board.CHESS_BOARD[srcX][srcY].equals("--")) {
        System.out.println("Invalid move: No piece in this place");
        return false;
    }
    if(!Board.CHESS_BOARD[srcX][srcY].equals(piece)) {
        System.out.println("Invalid move: Different piece at given source");
        return false;
    }
    if(Board.CHESS_BOARD[destX][destY]!="--") {
        if(Board.CHESS_BOARD[destX][destY].charAt(0)==color) {
            System.out.println("Invalid move: Your color piece at that place");
            return false;
        }
    }
    return true;
}
}

Pawn class

public class Pawn extends Piece {

@Override
public boolean validateForPiece(int srcX, int srcY, int destX, int destY) {
    if(Board.TOP_WHITE) {
        if(this.getColor()=='W' && destX-srcX<0)
            return false;
        if(this.getColor()=='B' && destX-srcX>0)
            return false;
    }
    else {
        if(this.getColor()=='B' && destX-srcX<0)
            return false;
        if(this.getColor()=='W' && destX-srcX>0)
            return false;
    }
    if(((srcX==1 || srcX==6) && (Math.abs(destX-srcX)==2)) 
            || ((srcX!=1 || srcX!=6) && (Math.abs(destX-srcX)==1)) 
            && srcY==destY) {
        return true;
    }
    if(Math.abs(srcY-destY)==1) {
        if(Board.CHESS_BOARD[destX][destY]=="--")
        return true;
    }
    if(Math.abs(srcX-destX)==1 && Math.abs(srcY-destY)==1) {
        return true;
    }
    return false;
}
}

The other classes like Rook, King, Queen, etc are also written in the same format, with different validation logic.

MoveImpl class

public class MoveImpl implements Move {

String piece;
int srcX;
int srcY;
int destX;
int destY;
char color;
char pieceType;

private void initializeValues(String inputMove[]) {
    piece = inputMove[0];
    srcX = Integer.parseInt(""+inputMove[1].charAt(0));
    srcY = Integer.parseInt(""+inputMove[1].charAt(1));
    destX = Integer.parseInt(""+inputMove[2].charAt(0));
    destY = Integer.parseInt(""+inputMove[2].charAt(1));
    color = piece.charAt(0);
    pieceType = piece.charAt(1);

}


@Override
public void move(String inputMove[]) {
    initializeValues(inputMove);
    Piece pieceObj = PieceFactory.getPiece(piece);
    boolean check1=pieceObj.validateFirst(srcX, srcY, destX, destY, piece);
    boolean check2=pieceObj.validateForPiece(srcX, srcY, destX, destY);
    if(!check1 || !check2)
        System.out.println("Invalid move");
    if(check1 && check2)
        pieceObj.move(srcX, srcY, destX, destY, piece);
}
}

Main class

public class Main {

public static void main(String[] args) throws IOException {

    Move move = new MoveImpl();
    System.out.println("Input:");
    BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
    String inp = br.readLine();
    if(inp.equals("Board")) {
        String board[][] = new String[8][8];
        for(int i=0;i<8;i++) {
            String row = br.readLine();
            String rowArr[]=row.split(" ");
            if(row.trim()!="")
            for(int j=0;j<8;j++) {
                board[i][j] = rowArr[j];
            }
        }
        BoardInitializer.initialize(board);
        System.out.println();
        System.out.println("INITIAL STATE:");
        BoardInitializer.displayBoardState();
    }
    else if(inp.equals("Display")) {
        BoardInitializer.initialize();
        System.out.println();
        System.out.println("INITIAL STATE:");
        BoardInitializer.displayBoardState();
    }
    inp=br.readLine();
    if(inp.equals("Moves")){
        while(inp!=null && inp.trim()!="") {
            if(inp.trim().equals(""))
                break;
            inp = br.readLine();
            if(inp.trim().equals(""))
                break;
            String inputMove[]=inp.split(" ");
            move.move(inputMove);

            BoardInitializer.displayBoardState();
        }
    }
}
}

Github link: https://github.com/yashi320/Chess-Validator

Sample Input and Output:

Input:
Display

INITIAL STATE:
CURRENT BOARD:
BR BH BC BK BQ BC BH BR 
BP BP BP BP BP BP BP BP 
-- -- -- -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
WP WP WP WP WP WP WP WP 
WR WH WC WQ WK WC WH WR 
Moves
BP 12 32
CURRENT BOARD:
BR BH BC BK BQ BC BH BR 
BP BP -- BP BP BP BP BP 
-- -- -- -- -- -- -- -- 
-- -- BP -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
WP WP WP WP WP WP WP WP 
WR WH WC WQ WK WC WH WR 
BP 32 12
Invalid move
CURRENT BOARD:
BR BH BC BK BQ BC BH BR 
BP BP -- BP BP BP BP BP 
-- -- -- -- -- -- -- -- 
-- -- BP -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
-- -- -- -- -- -- -- -- 
WP WP WP WP WP WP WP WP 
WR WH WC WQ WK WC WH WR 

Please suggest possible improvements in the design.

\$\endgroup\$
1
  • \$\begingroup\$ Not sure if this can be considered design feedback, but you would need to look at more than just the board to determine if moves like en passant and castling are legal. \$\endgroup\$
    – Ben Jones
    Commented Jun 21, 2023 at 21:04

3 Answers 3

8
\$\begingroup\$

Use Standards

Use standard notations and representations whenever possible: see Algebraic Notation for example.

Avoid Global Variables

Board.CHESS_BOARD is a global variable. Global variables are bad. So instead of a global variable, pass the board as an argument to the method. It is very possible that a program may need to consider multiple chessboard states at the same time, so static variables are just unsuitable.

Test and Fix Bugs

pawn movement is buggy:

if(((srcX==1 || srcX==6) && (Math.abs(destX-srcX)==2)) 
        || ((srcX!=1 || srcX!=6) && (Math.abs(destX-srcX)==1)) 
        && srcY==destY) {
    return true;
}

You haven't checked for the emptiness of the destination square, and returned true. A pawn can move forward, but cannot attack forward. In order to make the method clearer split it into canAttack and canMove

Buggy as well:

if(Math.abs(srcY-destY)==1) {
    if(Board.CHESS_BOARD[destX][destY]=="--")
    return true;
}

Pawns can attack but cannot move diagonally.

Use Conventional Formatting

Non-standard formatting looks confusing, weird and unprofessional. When you are new to a language it isn't obvious to you, but when you get used to the language it's obvoius and jarring. Here are a few very common and widely accepted conventions that your code doesn't follow from Sun's Java Code Convention

  • A keyword followed by a parenthesis should be separated by a space.

if (, while (, for (, try (

  • All binary operators except . should be separated from their operands by spaces.

  • The expressions in a for statement should be separated by blank spaces.

    Example: for (expr1; expr2; expr3)

\$\endgroup\$
6
\$\begingroup\$

Thank you for releasing your code! You have some clever solutions and I would like to add few more points to the previous answers.

Naming

Piece

The class Pawn confuses me, not because it is to complicated, but because of its name. Pawn is used to validate a movement and to do a move of a piece on the board, while the name suggests that it is the piece on the board. So with other words: It is not a piece it is a utility of a piece

The abstract class Piece could be renamed to AbstractMovements and Pawn to PawnMovements.

Note the appended s to the names, which indicates that it is a utility class. You can find some in the Java-World like Collections, Objects and so on.

Move

Additional the class MoveImpl is not responsible for a move. The moves are done by the implementations of Piece (AbstractMovements). Actual MoveImpl response is to extract the movement information from the users input and delegate it to a Piece. I think it would make sense to rename the abstract class Move to Reader and MoveImpl to ComandLineReader.

Primitive Obsession

Primitive Obsession is using primitive data types to represent domain ideas. For example, we use a String to represent a message [...]

The code-base contains a heavy use of int to represent a Position as a source and destination, String gets abused to represent a Piece and String[][] represents the board.

Examples

The method signiture of validateForPiece in Piece could look like

public boolean validateForPiece(Position source, Position destination)

or the class MoveImpl like

public class MoveImpl implements Move {

    Piece piece;
    Position source;
    Position destination;

    // ...
}

With the new data-types the if-statements can be clearer. For example

public boolean checkMiddlePieces(int srcX, int srcY, int destX, int destY) {
   if(srcX==destX && srcY!=destY) { /*..*/ }
   // ..
}

could be look like

public boolean checkMiddlePieces(Position source, Position destination) {
    if(source.hasSameRowAs(destination) && source.hasNotSameColoumAs(destination)) { /*.. */ }
    // ..
}

The Main Class

The main looks like a Factory with super power. It should be possible to move these if-statements into its own class. The main class could interact with with a class called ChessGame or something like that, that knows all your chess logic.

To check if the player enters the correct task at the correct time you could use the State Pattern. The InputState makes it possible for example that the user can't enter "Display" five times in a row or don't "Move" before "Display" or "Board"

public class Main {
    private ChessGame chessGame = new ChessGame();

    public static void main(String[] args) throws IOException {
         Move move = new MoveImpl();
         System.out.println("Input:");
         BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
         String input = br.readLine();

         chessGame.play(input);
    }
}

public class ChessGame {

    private InputState inputState;

    public ChessGame() {
       // user needs to enter first "Board" or "Display"
       this.inputState = new InformationInputState(this); 
     }

    void setState(InputState inputState) {
        this.inputState = inputState;
    }

    public void play(String input) {
       inputState.execute(input); 
    }
}

interface InputState {
    void execute(ChessGame, String input);
}

class MoveInputState implements InputState {

    private ChessGame chessGame;

    // constructor

    public void execute(String input) {
        if (input.equals("Move")) {
            // ..
            chessGame.setState(new InformationInputState(chessGame))
        }
    }
}

class InformationInputState implements InputState {

    private ChessGame chessGame;

    // constructor

    public void execute(String input) {
        if (input.equals("Board")) {
            // ..
            chessGame.setState(new MoveInputState(chessGame))
        } else if ("Display") {
            // ..
            chessGame.setState(new MoveInputState(chessGame))
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Here, Pawn is a type of piece only. And the validate method in Pawn is validating rules which should be enforced for Pawn. Similarly, I have code for other pieces as well, which gives different implementation of validation of move for all. \$\endgroup\$ Commented Feb 20, 2019 at 17:27
  • \$\begingroup\$ I will encorporate other suggestions in the code, escpecially Position class. Thanks @Roman \$\endgroup\$ Commented Feb 20, 2019 at 17:33
  • \$\begingroup\$ @YashiSrivastava you're welcome! Learn a lot by reading your code :] Keep it up! \$\endgroup\$
    – Roman
    Commented Feb 21, 2019 at 10:08
2
\$\begingroup\$

I would like to add few more points to the previous answers with respect to design.

If you are following the approach that chess board has the information about piece positioning, and piece is just type and have information about its type of movement and whether it can be blocked or not.

Considering this, Piece abstract class should not have attribute 'color'. As board is responsible for knowing at which place which piece is there and what is the actual colour of the piece.

public abstract class Piece {
    private char color; // It can be removed.

As the Piece is actually providing a contract about its movement, You should follow approach of interface implementation and can define Piece as an interface with contract method of movement and which can be implemented by type of piece King(K) , Knight(N) , Bishop(B) etc and these classes will be provide the implementation about there movement

public interface Piece{
    //for checking the move is valid or not
    boolean canMove(int srcX, int srcY, int destX, int destY, String[][] board);
    //for checking whether the movement is blocked by any piece in between or not
    //i.e. Knight won't get blocked, But other pieces can get blocked if any piece is in between the path
    boolean isBlocked(int srcX, int srcY, int destX, int destY, String[][] board);
}

Other normal validation like source/ destination is in bound , source has valid piece or not. can be handled as separate validation class which can follow Chain of responsibility design pattern. Implementation i am leaving to you.

Please open a new question with these changes, if you agree with the points; then we can look in depth for the other comments.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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