Castling move in Chess

I have just finished building a Chess game in Java as an exercise. Whilst I am quite happy with the rest of the code, my castling move check code is lengthy and ugly and I want to see if there is a way to improve it.

There is a board class, which has a list of all pieces which are all unique classes extended from a single chessman class. This code is contained within the king class.

• getOwner returns an enum which has a value of 0 or 1 depending on the player

• checkPosition returns a boolean if there is a unit at the position or not

• doMove is the normal form of moving pieces

• getUnit retrieves a unit at a specified position

• getPossibleMoves is every legal move a unit can make in the form of a point list

```
@Override
public boolean move(Point target){
int owner = super.getOwner().getValue() * 2;
List castlingMove = new ArrayList<>(Arrays.asList(new Point(3,1),new Point(7,1), new Point(3,8), new Point(7,8)));
List rooks = new ArrayList<>(Arrays.asList(new Point(1,1), new Point(8,1), new Point(1,8), new Point(8,8)));
Board b = super.getBoard();
if (castlingMove.contains(target) && !this.moved) {
boolean success = false;
if (target.equals(castlingMove.get(owner + 1)) && b.checkPosition(rooks.get(owner + 1))){ // Castling Short
success = checkRook(b, rooks, castlingMove, owner, 1);
} else if(target.equals(castlingMove.get(owner)) && b.checkPosition(rooks.get(owner))){ // Castling Long
success = checkRook(b, rooks, castlingMove, owner, 0);
}
if (success) this.moved = true;
return success;
} else {
boolean success = this.doMove(target);
if (success) this.moved = true;
return success;
}
}

private boolean checkRook(Board b, List rooks, List castlingMove, int owner, int moveType) {
Chessman u = b.getUnit(rooks.get(owner + moveType));
if (u instanceof Rook){
Rook r = (Rook) u;
if (!r.getMovedStatus()) {

I see three main issues: the improper usage of primitive constants, the bad naming of everything, and too long and complex methods. Case study: private boolean checkRook(Board b, List rooks, List castlingMove, int owner, int moveType).

• The method name checkRook. If I call checkRook(...) and get "true" or "false", I have no idea what that actually means. Name the method so that "true" and "false" can be answers to the question. "checkRook"? "true." doesn't sound like a conversation. "rookCanMove?" "true." works much better (if that's what the method is doing, I'm still not sure...). In the context of a chess game the word "check" also has a special meaning, so you should be really, really careful to only use it correctly. "checkRook" makes no sense in that respect, either (unless it means "a rook giving a check to the opposing king").

• The variable should be Board board, not Board b. It's not 1970s, you don't need to save bytes. "board" is much more readable than "b", and readability should be the king.

• int owner and int moveType. These should be enums: Owner owner and MoveType moveType. For an integer owner, one has no idea what value is white and what value is black, but Owner.WHITE and Owner.BLACK are self-explanatory. Similarly, "moveType 5" says nothing to anyone -- compare with MoveType.CHECK and MoveType.EN_PASSANT.

• Rather than giving the method both the Board and a list of rooks, consider implementing a new method into the Board class, something like Point[] getPiecesByType(PieceType.WHITE_ROOK). The board should hold references to its pieces, so you should be able to get their positions from it. This helps you remove one of those method parameters. Anything more than three is usually too much anyway.

Point[] t = {(Point) super.getPosition().clone(), (Point) r.getPosition().clone()};
int[] s = {5, 9};
// ...
if (b.checkPosition(m) && m.x < s[moveType])

Again, horrible naming. These cannot be read out of context, nor without scrolling back and forth, looking for the variable initializations. Actually, many of those are incomprehensible even when you see the definition. s, seriously? The t is a list of two points, so apparently a move from Point A to Point B: this should be encapsulated into a new class, called, for example, Move.

Furthermore, assuming the method checkRook checks something about the rooks in relation to the castling, I don't know how you managed to make the method so long. The only thing you need to check about rooks is that they haven't moved. Then you should have other methods for checking that your king is not in check and that it's not going to move over or onto a square that is in check.

Overly long methods like checkRook violate the well known principle that is formalized in Clean Code as follows:


FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY.

Uncle Bob continues:


We should also be able to write a brief description of the class in about 25 words, without using the words “if,” “and,” “or,” or “but.”

and that's obviously true for methods too. So break your methods into smaller ones that have descriptive names.

StackExchange Code Review Q#126485, answer score: 5