Return to Revisions

Use Java 16 Records

Coordinate is a typical transparent data-carrier.

record Coordinate(int row, int col) { }

Apply Information expert principle

Coordinate type should own the logic on how to parse it and validate its properties, not an external class.

record Coordinate(int row, int col) {
    public static boolean isValid(int row, int col) { ... }
    public static Coordinate[] parseCoordinates(String input) { ... }
}

Use Switch expressions

Switch expressions are way more readable and compact than switch statements.

int col = switch (coordinate.charAt(1)) {
    case 'a' -> 0;
    case 'b' -> 1;
    case 'c' -> 2;
    case 'd' -> 3;
    case 'e' -> 4;
    case 'f' -> 5;
    case 'g' -> 6;
    case 'h' -> 7;
    default  -> throw new IllegalArgumentException(); // not 'default -> 0;'
};

Also, avoid returning a valid value from the default case. Make your switch exhaustive, and if default case required (for instance, with enums it's not the case) throw an exception.

And, by the way, it's not necessary to use switch in to parse user input, intead you can leverage the face that char is numeric type (see code below).

Fail Fast

Avoid propagating objects with invalid state.

It means that ideally Game should receive from the user only valid coordinates, to achieve that you need to include a validation step into parsing logic.

public record Coordinate(int row, int col) {
    private static final Pattern VALID_INPUT = Pattern.compile("([1-8][a-h])-([1-8][a-h])");
    private static final char FIRST_CHARACTER = 'a';

    public static boolean isValid(int row, int col) {
        return row >= 0 && row <= 7
            && col >= 0 && col <= 7;
    }
    
    public static Coordinate[] parseCoordinates(String input) {
        var matcher = VALID_INPUT.matcher(input);
        if (!matcher.matches()) throw new IllegalArgumentException();
        
       return new Coordinate[]{
           parse(matcher.group(1)),
           parse(matcher.group(2))
       };
    }
    
    private static Coordinate parse(String coordinate) {
        int row = 8 - Character.getNumericValue(coordinate.charAt(0));
        int col = coordinate.charAt(1) - FIRST_CHARACTER; // equivalent of switch shown above

        return new Coordinate(col, row);
    }
}

Regular expression "([1-8][a-h])-([1-8][a-h])" takes care of validating user input:

• every group ([1-8][a-h]) matches a two-character string;
• 1-8 matches a single character between '1' and '8';
• a-h matches a single character in the range between 'a' and 'h'.

Also consider handling the exception (you can define a custom exception type instead of using IllegalArgumentException) caused by malformed user input.

That would improve user experience by giving them the opportunity to retry instead of throwing stack trace at them.

Don't expose encapsulated data

Object-orientation is about exposing behavior, not data.

There's no proper encapsulation in the Game class.

This is not quite right:

public char[][] getBoard() { return board; }

ui.printBoard(game.getBoard());

Exposing the array board is wrong for several reasons:

• it's a mutable object holding internal state exposed via public method;
• usage of char[][] array is an implementation detail of the Game class;
• method printBoard() is aware of how to dial with the internal data structure of the Game class, i.e. you created a tight coupling between TerminalUI and Game.

What you should do instead? - Apply the Information expert principle.

Instead of making TerminalUI aware of how the string representation of the board should look like, move this logic into toString() method of the Game class.

Inline Comments in the code is a smell

When you need to place a comment in the code in order to reason about it, it means that something is lacking. The essence of what you described in the comment should be expressed through proper method names and variable names.

Example:

public boolean isGameOver() {
    for (int i = 0; i < board.length; ++i) {
        for (int j = 0; j < board[0].length; ++j) {
            // If a piece of the current player can move, then game is not over
            if (players[currentPlayer] == board[i][j] && canMove(new Coordinate(j, i))) return false;
        }
    }
    return true;
}

If you extract the condition into a separate method with a self-explanatory name, there would be no need for the comment.

When you can not simplify the code by extracting methods (you tried, and It's still messy), consider introducing an abstraction.

Document behavior / Avoid useless doc comments

Documentation comment should be used to provide information on the intention of the code element.

You can explain what problem it solves (if it's not immediately obvious from the name), provide usage examples if needed, specify valid ranges of expected parameters, describe side effects, behavior in edge cases, constraints and assumptions, and other subtleties which reader of the code can not deduce from the name.

But you should not include redundant and obvious information and things like implementation details.

Example:

/**
 * Constructs the class and its attributes
 */
public TerminalUI() {
    in = new Scanner(System.in);
}

That's an example of obvious information which is not helpful for the user of your code.

Avoid using different names for the same concept

Example: x, y and row, col.

Don't alternate between different names, that causes confusion for the reader. Choose the one that fits better.

Define static constants

Turn the strings scatted across the methods in the UI into compile time constants:

class TerminalUI {
    private static final String NEXT_TURN_PROMPT = "Player %s - your turn.";

    // ...

    public void printPlayerTurn(Player player) {
        System.out.println(NEXT_TURN_PROMPT.formatted(player));
    }
}

Make use of String.formatted() when you need to inject data into these static final strings.

The name of a Code element should reflect its Purpose

Class Main based on its contents should be named GameRunner or GameManger.

My advise: rename Main to GameRunner and create and create class Main hosting only main() method in a sepate file.

Consider introducing abstractions to manage code complexity

I would advice to consider defining some abstractions that would enable simplification of the business logic.

NOTE: that all the code in the answer should be considered as a source of inspiration, not as the only proper implementation

So, to begin with, think about defining an enum named Player (instead of juggling with 1 and 0, X and O) and a nested class Cell. The board would became an array of cells.

public class Game {
    
    public enum Player {
        WHITE("X"), BLACK("O");
        
        private final String symbol; 
        
        Player(String symbol) {
            this.symbol = symbol;
        }
        
        @Override
        public String toString() {
            return symbol;
        }
    }

    private static final Cell BLACK_EMPTY_CELL = new EmptyCell(Cell.Color.BLACK);
    private static final Cell WHITE_EMPTY_CELL = new EmptyCell(Cell.Color.WHITE);
    private static final Cell CELL_WITH_WHITE_PIECE = new OccupiedCell(Player.WHITE);
    private static final Cell CELL_WITH_BLACK_PIECE = new OccupiedCell(Player.WHITE);
    private final Cell[][] board;
    private Player currentPlayer = Player.WHITE;

    // other stuff
    
    interface Cell {
        boolean isEmpty();
        boolean isOccupiedBy(Player player);
        
        enum Color {
            BLACK("■"), WHITE(" "); // symbols can be used in the UI to represent unoccupied cells
            
            private final String symbol;
            
            Color(String symbol) {
                this.symbol = symbol;
            }
            
            @Override
            public String toString() {
                return symbol;
            }
        }
    }
    
    private record EmptyCell(Color color) implements Cell {
        
        @Override
        public boolean isEmpty() {
            return true;
        }
        
        @Override
        public boolean isOccupiedBy(Player player) {
            return false;
        }
        
        @Override
        public String toString() {
            return color.toString();
        }
    }
    
    private record OccupiedCell(Player player) implements Cell {
        
        @Override
        public boolean isEmpty() {
            return false;
        }
        
        @Override
        public boolean isOccupiedBy(Player player) {
            return this.player == player;
        }
        
        @Override
        public String toString() {
            return player.toString();
        }
    }

}

If you have a requirement to display 0 or 1 on the UI when referring to a player, use Enum.ordinal() method to get the position of the enum constant.

Note, that there would be only created 4 (!) instances of the Cell type. They are stateless and we can reuse them.

Now, let's first refactor the constructor by moving all the heavy logic from it into a factory method:

public class Game {
    
    // ...
    
    private Game(Cell[][] board) {
        this.board = board;
    }
    
    public static Game create() {
        var board = new Cell[8][8];
        
        setWhitePieces(board);
        setBlackPieces(board);
        setEmptyCells(board);
        
        return new Game(board);
    }
    
    private static void setWhitePieces(Cell[][] board) {
        for (int i = 0; i < 3; ++i) {
            for (int j = 0; j < 8; ++j) {
                if (isBlackCell(i, j)) {
                    board[i][j] = CELL_WITH_WHITE_PIECE;
                }
            }
        }
    }
    
    private static void setBlackPieces(Cell[][] board) {
        for (int i = 7; i > 4; --i) {
            for (int j = 7; j >= 0; --j) {
                if (isBlackCell(i, j)) {
                    board[i][j] = CELL_WITH_BLACK_PIECE;
                }
            }
        }
    }
    
    private static void setEmptyCells(Cell[][] board) {
        for (int i = 0; i < 8; ++i) {
            for (int j = 0; j < 8; ++j) {
                if (board[i][j] != null) continue;
                
                if (!isBlackCell(i, j)) {
                    board[i][j] = BLACK_EMPTY_CELL;
                } else {
                    board[i][j] = WHITE_EMPTY_CELL;
                }
            }
        }
    }
    
    private static boolean isBlackCell(int i, int j) {
        return (i + j) % 2 == 1;
    }
}

Pay attention to the fact that helper methods setWhitePieces, setBlackPieces and setEmptyCells correspond to the sections of your initial code sprinkled with inline comments.

Now, let's address your question regarding reducing complexity of methods like canMove, canJump, ect.

As an example, let's refactor canJump:

public class Game {
    
    // ...
    
    private boolean canJump(int row, int col) {
        if (!isOccupiedByCurrentPlayer(row, col)) return false;
        
        return canCapturePiece(row, col, ColumnDirection.LEFT)
            || canCapturePiece(row, col, ColumnDirection.RIGHT);
    }
    
    private boolean canCapturePiece(int row, int col, 
                                    ColumnDirection direction) {
        return hasEnemyPiece(row, col, direction)
            && canJump(row, col, direction);
    }
    
    private boolean hasEnemyPiece(int row, int col, ColumnDirection colDirection) {
        int enemyRow = row + rowDirection();
        int enemyCol = col + colDirection.toInt();
        
        return Coordinate.isValid(enemyRow, enemyCol)
            && isOccupiedByEnemy(enemyRow, enemyCol);
    }
    
    private boolean canJump(int row, int col, 
                            ColumnDirection colDirection) {
        int targetRow = row + 2 * rowDirection();
        int targetCol = col + 2 * colDirection.toInt();
        
        return Coordinate.isValid(targetRow, targetCol) 
            && isEmpty(targetRow, targetCol);
    }
    
    private int rowDirection() {
        return currentPlayer == Player.WHITE ? -1 : 1;
    }
    
    private boolean isEmpty(int row, int col) {
        return board[row][col].isEmpty();
    }
    
    private boolean isOccupiedByCurrentPlayer(int row, int col) {
        return board[row][col].isOccupiedBy(currentPlayer);
    }
    
    private boolean isOccupiedByEnemy(int row, int col) {
        return !board[row][col].isEmpty()
            &&  board[row][col].isOccupiedBy(currentPlayer);
    }
    
    private enum ColumnDirection {
        LEFT(-1), RIGHT(1);
        private int ratio;
        
        ColumnDirection(int ratio) {
            this.ratio = ratio;
        }
        
        public int toInt() {
            return ratio;
        }
    }
}

Note, parameters row and col are used directly in this instead of Coordinate.

Coordinate type is useful and convenient when we are parsing user input and for interacting with the Game instance from the outside, namely through method move:

public void move(Coordinate from, Coordinate to)

But then inside Game class while passing around instances of Coordinate you're introding local variables representing rows and column and then again constructing new Coordinate instances just to invoke internal methods (look at canMove method). That means that in these cases usage of this type is no longer helpful.

Here's how canMove can be implemented based on the functionality intoduced earlier:

public boolean canMove(int row, int col) {
    if (!isOccupiedByCurrentPlayer(row, col)) return false;
    if (anyCanJump()) return false;
    
    return canMove(row, col, ColumnDirection.LEFT)
        || canMove(row, col, ColumnDirection.RIGHT);
}

private boolean anyCanJump() {
    boolean canJump = false;
    for (int row = 0; row < board.length; row++) {
        for (int col = 0; col < board[0].length; col++) {
            canJump = canJump(row, col);
            if (canJump) break;
        }
    }
    return canJump;
}

private boolean canMove(int row, int col, ColumnDirection colDirection) {
    int targetRow = row + 2 * rowDirection();
    int targetCow = col + 2 * colDirection.toInt();
    
    return Coordinate.isValid(targetRow, targetCow)
        && isEmpty(targetRow, targetCow);
}