How can I simplify this code? (Chess game obstruction testing)

Question

I'm making a chess game in Java, and testing to make sure there are no pieces blocking the path of the piece being moved. The piece moves from (srcX,srcY) to (dstX,dstY).

I've written this code which checks if there are any obstructions for a rook:

    if(dstY == srcY) {

            // No change on Y axis, so moving east or west

            if(dstX > srcX) {
                // Moving east
                // Test every cell the piece will pass over
                for(int x = srcX+1; x < dstX; x++) {
                    // Is the cell set?
                    if(isPiece(x, srcY)) {
                        return true;
                    }
                }
            } else {
                // Must be moving west
                // Test every cell the piece will pass over
                for(int x = srcX-1; x > dstX; x--) {
                    // Is the cell set?
                    if(isPiece(x, srcY)) {
                        return true;
                    }
                }
            }

        } else if(dstX == srcX) {

            // No change on X axis, so moving north or south

            if(dstY > srcY) {
                // Moving north
                // Test every cell the piece will pass over
                for(int y = srcY+1; y < dstY; y++) {
                    // Is the cell set?
                    if(isPiece(srcX, y)) {
                        return true;
                    }
                }
            } else {
                // Must be moving south
                // Test every cell the piece will pass over
                for(int y = srcY-1; y > dstY; y--) {
                    // Is the cell set?
                    if(isPiece(srcX, y)) {
                        return true;
                    }
                }
            }
        }

but it's a bit big and I'm sure it can be simplied.. any ideas?

ps, this is ONLY obstruction testing. I've already validated everything else.

Answer 1

My soultion would be: introduce a direction class, and then do the check in this style: isBlocked(startPossition, direction, numberOfFields)

I have done a little example, using 3 Classes.

• Direction - an enum to represent the 8 directions (2 horizontal, 2 vertical, 4 diagonal)
• Position - the x and y value of an position
• LinarMove - represent one linear Move(startPossition, direction, numberOfFields) and contains the isBlockedMethod

The Enum:

public enum Direction {

    UP(0, 1),
    DOWN(0, -1),
    LEFT(1, 0),
    RIGHT(-1, 0),

    UP_LEFT(UP, LEFT),
    UP_RIGTH(UP, RIGHT),
    DOWN_LEFT(DOWN, LEFT),
    DOWN_RIGHT(
            DOWN, RIGHT);

    private final int incrementX;
    private final int incrementY;


    private Direction(int incrementX, int incrementY) {
        this.incrementX = incrementX;
        this.incrementY = incrementY;
    }

    private Direction(Direction sub1, Direction sub2) {
        this.incrementX = sub1.incrementX + sub2.incrementX;
        this.incrementY = sub1.incrementY + sub2.incrementY;
    }

    public Position oneField(Position start) {
        return new Position(start.getX() + this.incrementX, start.getY()
                + this.incrementY);
    }
}

The purpuse of second constructor is only that it alowes to write the diagonal moves in a more readable way.

public class Position {
    private final int x;
    private final int y;

    public Position(int x, int y) {
        super();
        this.x = x;
        this.y = y;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    @Override
    public String toString() {
        return "x:" + x + ", y:" + y;
    }
}

The Move contains the isBlocked Method -- you can see how small it get, and how readable it become. At least there is no single direction related if statement left.

The name LinareMove sugget that there is possible an other kind of move for the knight.

public class LinearMove {

    private final Position start;
    private final Direction direction;

    /** Length of the move. */
    private final int numberOfFields;

    public LinearMove(Position start, Direction direction, int numberOfFields) {
        super();
        this.start = start;
        this.direction = direction;
        this.numberOfFields = numberOfFields;
    }

    boolean isBlocked() {
        Position pos = this.start;
        for (int i = 0; i < (this.numberOfFields - 1); i++) {
            pos = this.direction.oneField(pos);
            if (isPiece(pos)) {
                return true;
            }
        }
        return false;
    }

    boolean isPiece(Position pos) {
        //Dummy;
        System.out.println(pos);
        return false;
    }   
}

And this is how it works:

    public static void main(String[] args) {
    new LinearMove(new Position(1, 1), Direction.RIGHT, 3).isBlocked();
    }

You maybe noticed, that the knights move is some kind of probem. With this soultion you could model it in two ways:

- 4 special Directions
- an other kind of move class (this is the more cleaner way, because you could always return true, in the isBockedMethod)

Answer 2

Nearly the same, but with for loops:

// move along x axis
for (int x = 1; x < Math.abs(srcX - dstX); x++) {
    int curX = (srcX - dstX) < 0 ? srcX - x : srcX + x;
    if (isPiece(curX, srcY))
        return true;
}   
// move along y axis
for (int y = 1; y <= Math.abs(srcY - dstY); y++) {
    int curY = (srcY - dstY) < 0 ? srcY - y : srcY + y;
    if (isPiece(srcX, curY))
        return true;
}

Answer 3

Once you've tested for direction, you can set dx, dy values (e.g. dx=1, dy=0 for east). Then you can have a single for loop for all cases and just increment x and y by dx and dy respectively at each iteration.

You can then simplify the direction checking into the following:

if dstY == srcY: dy = 0
else: dy = (dstY - srcY) / abs(dstY - srcY)
if dstX == srcX: dx = 0
else: dx = (dstX - srcX) / abs(dstX - srcX)

Code:

int dx, dy;
if (dstY == srcY) dy = 0;
else dy = (dstY - srcY) / Math.abs(dstY - srcY);
if (dstX == srcX) dx = 0;
else dx = (dstX - srcX) / Math.abs(dstX - srcX);

while (srcX != dstX || srcY != dstY) {
  srcX += dx; srcY += dy;
  if (isPiece(srcX, srcY))
    return true;
}
return false;

Also beware that this code (and yours) will fail if the move is not horizontal, vertical or diagonal.

Answer 4

First, write tests. Lots and lots of tests. That way you can be confident that you're simplifying without changing the meaning of the code.

Refactoring without unit tests is like walking a high wire without a safety net.

Answer 5

You could do something along these lines (untested as I don't have a compiler to hand):

int dx = 0;
int dy = 0;
if (dstX != srcX) {
  dx = (dstX > srcX) ? 1 : -1;
} else if (dstY != srcY) {
  dy = (dstY > srcY) ? 1 : -1;
}
int x = srcX + dx;
int y = srcY + dy;
while (x != dstX || y != dstY) {
  if (isPiece(x, y)) return true;
  x += dx;
  y += dy;
}