0

I have an abstract class that represents chess pieces, it has an abstract method isMoveValid(Square futurePosition, PieceColor color) which checks if the piece moving to that square is valid or not, if the move is valid but there is an opponent piece on that square then the piece is trying to capture not just move.

I believed that the function is against the single responsibility principle, since it captures and checks if the move is valid, I wanted to remove the capturing logic and move it to a standalone function in the abstract class called willCapture() and make isMoveValid() use that function to check if the move is intended to capture.

The main issue is that each piece has its own logic for capturing (i.e a pawn can only capture diagonally), some values needed for the logic are already calculated in isMoveValid() and I did not want to repeat the same code twice, so I thought we can pass it to willCapture() as a parameter but some pieces do not need those values, how can I make this method accept different parameters depending on what object is calling it?

I thought about the strategy pattern by making a CaptureStrategy interface and a class that implements it for each piece and making willCapture() take a CaptureStrategy but that felt like I added an extra step and I am not really interested in switching the strategy dynamically since each piece will only ever use one strategy, is there a design pattern that solves such a problem?

Code snippets:

Pawn's capture logic in isMoveValid():

    int yPosDifference = Math.abs(futurePosition.getY() - this.getPosition().getY());
    int xPosDifference = Math.abs(futurePosition.getX() - this.getPosition().getX());
    if (Board.getBoard().isSquareOccupied(futurePosition) && xPosDifference == 1 && yPosDifference == 1)
    {
        Piece capturedPiece = Board.getBoard().getPieceFromSquare(futurePosition);
        if (capturedPiece.getColor() != this.getColor())
        {
            capture(capturedPiece);
            return true;
        }
        throw new IllegalArgumentException("You cannot capture your own piece.");
    }
    return false;

Queen's capture logic in isMoveValid():

        if (Board.getBoard().isSquareOccupied(futurePosition)) {
        Piece capturedPiece = Board.getBoard().getPieceFromSquare(futurePosition);
        if (capturedPiece.getColor() != this.getColor())
        {
            capture(capturedPiece);
            return true;
        }
        throw new IllegalArgumentException("You cannot capture your own piece.");
    }
    return false;

Pawn's isMoveValid() requires yPosDifference and xPosDifference in other parts of isMoveValid() but is also required in the capture logic, if we calculate it inside willCapture() we will have duplicate code and if we pass it as a parameter then the Queen and other pieces will not make use of it.

12
  • "some values needed for the logic are already calculated in isMoveValid()"- " but some pieces do not need those values". Could you please tell us precisely which values, or give an example where we can actually see your issue?
    – Doc Brown
    Commented Aug 14, 2022 at 17:31
  • ... and why does ChessPiece.isMoveValid require a piece color as parameter? I guess a chess piece should know its color already.
    – Doc Brown
    Commented Aug 14, 2022 at 17:40
  • Edited the question, now has code snippets. @DocBrown it is there to avoid a player moving a piece that aren't theirs. if (this.getColor() != color) throw new IllegalArgumentException("The piece chosen was not your piece."); It takes the current player's color (i.e white or black) and checks if the color of the current piece is the same as the player. I probably should rename the parameter.
    – Yoh
    Commented Aug 14, 2022 at 17:48
  • I see an issue with your separation in that I believe that in some circumstances isMoveValid() and willCapture() are related and need to exchange information. In the case of a pawn, a diagonal move is only valid, if it is capturing a piece.
    – Peter M
    Commented Aug 14, 2022 at 18:15
  • Your example still misses the crucial part you were asking about, the intended signatures of willCapture - come on - that is the core of your question, and you are holding this back? I think you can do better,
    – Doc Brown
    Commented Aug 14, 2022 at 18:31

3 Answers 3

6

The fact that different signatures would be needed suggest that something is wrong in the polymorphic design. It's not very useful to have a Piece abstraction, but have to use each Piece differently. This is a design that is not OCP compliant, since for every new kind of Piece, you'd have to modify the code that use the Pieces.

Typically, we can expect that a chess piece knows its position and its color. But a piece does generally not know the position of the other pieces that might be on the way and make the move impossible. Fortunately, wa can expect that the board, knows the pieces that are on each square.

With this in mind, to check the move, you'd use isMoveValid(Square futurePosition, Board board). There is all the information needed for every to find out if the move is valid, using a different implementation of the same isMoveValid(). They would just verify differently if the futurePosition is in range, and -- if needed -- the pieces that are on the way and/or on the target position).

7
  • isMoveValid() is already doing that, the board is a singleton so I get the instance inside of the method when needed instead of passing it as an argument, the issue is the process of capturing, I can make it return true if the move intended is to capture and is valid, but then how would board know how to update it? I'm thinking maybe an observer pattern where I can notify the board that a capture is in process.
    – Yoh
    Commented Aug 14, 2022 at 18:46
  • 1
    @MohammadSrahin if the board is singleton, how will your game be able to simulate future moves anticipating on possible moves of the adversary (e.g minimax or allha-beta pruning) ? the Singleton pattern is to be used in exceptional situations only, keeping in mind that it creates hidden couplings and constraints.
    – Christophe
    Commented Aug 14, 2022 at 19:19
  • 1
    @MohammadSrahin but your question remains valid; the responsibilities of the board, the pieces and the game need to be clearly defined. If the piece makes the capture, it has to inform the board of the removal of the piece. if the board takes note of moves that are known to be valid, the board could manage the capture based on the target position. But may be the game observes the board and makes the capture based on the move. All 3 approaches have pros and cons.
    – Christophe
    Commented Aug 14, 2022 at 19:26
  • The Board class has a List<Piece> that keeps track of all the pieces that are currently on the board and their positions, I made it a singleton so that the state is ensured to be the same across all classes, is this an incorrect usage of the singleton pattern? Also the program is meant for two human players, so there is no need to simulate future moves, imo.
    – Yoh
    Commented Aug 14, 2022 at 19:44
  • 1
    The intent of the singleton is “Ensure a class only has one instance, and provide a global point of access to it”. While you use the second part as a convenience, the question is: is it really necessary to ensure that there is only one instance of the board? In my opinion, everytime a singleton is used without this necessity of uniqueness is a symptom of possible misuse/antipattern.
    – Christophe
    Commented Aug 14, 2022 at 20:17
1

Single responsibility:

IsMoveValid: Look at the name. The function will return YES or NO depending on whether the move is valid or not. It will not perform the move.

PerformMove: Look at the name. It has one responsibility: To perform the move with all its side effects. Should never be called if the move is invalid (might throw an exception, or better assert because calling it for an invalid move is a programming error that needs to be fixed).

PerformMoveIfValid: Look at the name. It has clearly two responsibilities. Return NO if the move is not valid, or perform the move with all its side effects then return YES if the move was valid. Whether you are worried about the two responsibilities is up to you, I'm happy if the name expresses clearly what the function does.

1
  • Technically I can make it check if the move is valid in the client and make move() have only a single responsibility, but this defeats abstraction doesn't it? If we want to make one entry point to the system then we will have a method that will do more than one thing, is there a way to make it truly single responsibility?
    – Yoh
    Commented Aug 16, 2022 at 15:32
0

I think you need to think clearly about which methods need to be part of the public interface of all chess pieces, and which are internal implementation details of a particular piece.

If willCapture is only ever used inside the class, it can be private. Then it doesn't need to be part of the interface / abstract class, and doesn't need to have the same parameters - or even exist at all - in particular sub-classes.

On the other hand, if you want to use willCapture from outside the class, on any type of piece, then it needs to be public, and you need a standard set of parameters for it otherwise you won't be able to call it. That doesn't mean you can't share any internal logic, but rather than isMoveValid calling willCapture, you would make both methods call some other helper.

The pawn, for example, has a number of "special abilities", which might have their own helpers: isValidStartingRowMove, isValidForwardMove, isValidStandardCapture, isValidEnPassantCapture. Since these are internal, they can have whatever input and output makes them useful to the rest of the class.

0

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.