simplicity vs. conciseness in a chess program

I've read multiple articles/stack exchange questions on code smells, code duplication, etc, and am still having a bit of trouble deciding what to do about my problem. I'm writing a simple chess program, which has an abstract class called Piece. Right now, the Piece class has two very similar methods, addRank and addFile:

 /**
 * Adds squares that constitute legal moves in the same rank as a piece to an ArrayList
 * Used in Rook, King, and Queen implementations of getAvailableMoves()
 * @param availableMoves The ArrayList that the squares will be added to
 * @param startIndex The starting index for additions (for Rook, Queen, this 
 * will be the beginning of the board; for Kings it will be one space to 
 * their left
 * @param endIndex The ending index for additions (for Rook, Queen, this 
 * will be the end of the board; for Kings it will be one space to 
 * their right
 */

public void addRank(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
    int currX = this.square.getXCoord();
    int currY = this.square.getYCoord();

    for(int i = currX + 1; i < endIndex; i++) {
        if(Game.board[i][currY].getHasPiece()) {
            if(hasOppositePiece(this, Game.board[i][currY])){ //capturing a piece of a different color constitutes a legal move
                availableMoves.add(Game.board[i][currY]);
            }
            break; //Rooks, Queens, King cannot move past another piece
        }
        availableMoves.add(Game.board[i][currY]);
    }

    for(int i = currX - 1; i >= startIndex; i--) {
        if(Game.board[i][currY].getHasPiece()) {
            if(hasOppositePiece(this, Game.board[i][currY])){
                availableMoves.add(Game.board[i][currY]);
            }
            break;
        }
        availableMoves.add(Game.board[i][currY]);
    }
}

/**
 * Adds squares that constitute legal moves in the same file as a piece to an ArrayList
 * Used in Rook, King, and Queen implementations of getAvailableMoves()
 * @param availableMoves The ArrayList that the squares will be added to
 * @param startIndex The starting index for additions (for Rook, Queen, this 
 * will be the beginning of the board; for Kings it will be one space 
 * above them
 * @param endIndex The ending index for additions (for Rook, Queen, this 
 * will be the end of the board; for Kings it will be one space 
 * below them
 */

public void addFile(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
    int currX = this.square.getXCoord();
    int currY = this.square.getYCoord();


    for(int i = currY + 1; i < endIndex; i++) {
        if(Game.board[currX][i].getHasPiece()) {
            if(hasOppositePiece(this, Game.board[currX][i])){
                availableMoves.add(Game.board[currX][i]);
            }
            break;
        }
        availableMoves.add(Game.board[currX][i]);
    }

    for(int i = currY - 1; i >= startIndex; i--) {
        if(Game.board[currX][i].getHasPiece()) {
            if(hasOppositePiece(this, Game.board[currX][i])){
                availableMoves.add(Game.board[currX][i]);
            }
            break;
        }
        availableMoves.add(Game.board[currX][i]);
    }
}

now, obviously these two methods are nearly identical, and I could probably merge them into a single method. However, that would involve adding more parameters to my function, as well as a more complicated if/else structure, and I feel that I would be sacrificing readability for less duplication. In your opinion, is it permissible to have two smaller methods duplicated if it improves the readibility and reduces the complexity of your code, or is it always better to reduce duplication?

2 answers

  • answered 2018-02-13 02:19 sprinter

    You might like to abstract the behaviour of pieces and moves further. Something like:

    interface Piece {
        Stream<Move> getSingleMoves();
        boolean allowsMultipleMoves();
        Position apply(Move move);
    }
    
    interface Move {
        Position apply(Position position);
        Move add(Move other);
    }
    

    Then your logic would become:

    List<Move> moves = piece.getSingleMoves()
            .flatMap(move -> multipleMoves(piece, move))
            .collect(Collectors.toList());
    
    private Stream<Move> multipleMoves(Piece piece, Move move) {
        if (piece.allowsMultipleMoves()) {
            return Stream.iterate(move, m -> board.isOpen(piece.apply(m)), Move::add);
        } else if (board.isOpen(piece.apply(move)) {
            return Stream.of(move);
        }
    }
    

    That's probably not exactly right (I've ignored opposition pieces and moving off the board) but it might give you an idea of how to abstract further and avoid duplication. Your algorithm shouldn't really need to know about implementation details of the board such as storing data in arrays. Ideally it should be able to know enough about pieces, positions and boards to generate legal moves without having to iterate through indices.

  • answered 2018-02-13 02:19 shmosel

    It's obviously very subjective, but given the four very similar loops, I would say it's worth refactoring. One approach is to move the loop body to another method that returns false when it hits a piece:

    public void addRank(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
        int currX = this.square.getXCoord();
        int currY = this.square.getYCoord();
    
        for(int i = currX + 1;
                i < endIndex && addMove(availableMoves, Game.board[i][currY]);
                i++);
    
        for(int i = currX - 1;
                i >= startIndex && addMove(availableMoves, Game.board[i][currY]);
                i--);
    }
    
    public void addFile(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
        int currX = this.square.getXCoord();
        int currY = this.square.getYCoord();
    
        for(int i = currY + 1;
                i < endIndex && addMove(availableMoves, Game.board[currX][i]);
                i++);
    
        for(int i = currY - 1;
                i >= startIndex && addMove(availableMoves, Game.board[currX][i]);
                i--);
    }
    
    private boolean addMove(List<Square> availableMoves, Square square) {
        if(square.getHasPiece()) {
            if(hasOppositePiece(this, square)) {
                availableMoves.add(square);
            }
            return false;
        }
        availableMoves.add(square);
        return true;
    }
    

    Another option is to pass the iteration logic itself to another method. You can do this in Java 9 using streams:

    public void addRank(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
        int currX = this.square.getXCoord();
        int currY = this.square.getYCoord();
    
        addMoves(availableMoves,
                IntStream.iterate(currX + 1, i -> i < endIndex, i -> i + 1)
                        .mapToObj(i -> Game.board[i][currY]));
    
        addMoves(availableMoves,
                IntStream.iterate(currX - 1, i -> i >= startIndex, i -> i - 1)
                        .mapToObj(i -> Game.board[i][currY]));
    }
    
    public void addFile(ArrayList<Square> availableMoves, int startIndex, int endIndex) {
        int currX = this.square.getXCoord();
        int currY = this.square.getYCoord();
    
        addMoves(availableMoves,
                IntStream.iterate(currY + 1, i -> i < endIndex, i -> i + 1)
                        .mapToObj(i -> Game.board[currX][i]));
    
        addMoves(availableMoves,
                IntStream.iterate(currY - 1, i -> i >= startIndex, i -> i - 1)
                        .mapToObj(i -> Game.board[currX][i]));
    }
    
    private void addMoves(List<Square> availableMoves, Stream<Square> squares) {
        for(Iterator<Square> iter = squares.iterator(); iter.hasNext(); ) {
            Square square = iter.next();
            if(square.getHasPiece()) {
                if(hasOppositePiece(this, square)){
                    availableMoves.add(square);
                }
                break;
            }
            availableMoves.add(square);
        }
    }