6
\$\begingroup\$

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<Point> castlingMove = new ArrayList<>(Arrays.asList(new Point(3,1),new Point(7,1), new Point(3,8), new Point(7,8)));
    List<Point> 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<Point> rooks, List<Point> castlingMove, int owner, int moveType) {
    Chessman u = b.getUnit(rooks.get(owner + moveType));
    if (u instanceof Rook){
        Rook r = (Rook) u;
        if (!r.getMovedStatus()) {
            boolean noCheck = true;
            boolean freeSpace = true;
            for (Chessman unit : b.getAllUnits()){
                if (unit.getOwner() == super.getOwner()) continue;
                Point[] t = {(Point) super.getPosition().clone(), (Point) r.getPosition().clone()};
                int[] s = {5, 9};
                Point m = t[moveType];
                while (t[moveType].getX() < s[moveType]) {
                    if (unit.getPossibleMoves().contains(m)) noCheck = false;
                    m.translate(1, 0);
                    if (b.checkPosition(m) && m.x < s[moveType]) freeSpace = false;
                }
            }

            if (noCheck && freeSpace) {
                int[] p = {4,6};
                super.setPosition(castlingMove.get(owner + moveType));
                r.setPosition(new Point(p[moveType],super.getPosition().y));
                return true;
            }
        }
    }
    return false;
}

If anyone needs more code, just say so. I didn't include the whole project because it is a large amount of code.

\$\endgroup\$
5
\$\begingroup\$

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<Point> rooks, List<Point> 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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I will admit I did write this just so it would work, per your last paragraph there is a bit more than that, first I have to check if the move is a castling move, then which type of castling move (short/long) as well as if there is a unit in the rook position, then if that unit is a rook and it hasn't moved, then check the positions between the rook and king are empty as well as none of those positions putting the king into check which I cant do with my current checkForCheck() method as it only checks the kings position. The naming is horrible I agree, I was just making it work. \$\endgroup\$ – The Empire Strikes Back Apr 24 '16 at 0:36
  • \$\begingroup\$ I've extended my answer with a couple of quotes from Clean Code, my new favorite book. :) Since you needed that many words to describe what the method does, the method is obviously too long and should be broken into several smaller ones. \$\endgroup\$ – ZeroOne Apr 24 '16 at 15:16
  • \$\begingroup\$ also, single letter variable names can be hard to search for \$\endgroup\$ – I'll add comments tomorrow Apr 24 '16 at 15:32
  • \$\begingroup\$ Indeed that's a good point, @I'lladdcommentstomorrow. If you grep your code for variable "t", you will get almost every line of your program as a result. If you grep for "squaresReachableByWhite" or something similar, you'll end up with a nice and concise report of where exactly that method was called. \$\endgroup\$ – ZeroOne Apr 25 '16 at 7:02

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.