Use Java 16 Records
Coordinate is 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 this case because char is numeric type.
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))
};
}
public 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 - a single character between 1 and 8, a-h - a single character in the range between a and h).
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 theGameclass; - method
printBoard()is aware of how to dial with the internal data structure of theGameclass, i.e. you created a tight coupling betweenTerminalUIandGame.
What you should do instead? - Apply the Information expert principle.
Instead of making TerminalUI aware what how string representation of the board should look like, move this logic into toString() method of the Game class.
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 method with a self-explanatory name, there would be no need for the comment.
If 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 example of obvious information which is not helpful for the user of your code.
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() to inject data into the predefined string template.
Consider introducing abstraction to manage code complexity
I would advice to consider defining some abstraction 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 the enum named Player (instead of juggling with 1 and 0, X and O) and 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(" ");
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();
}
}
}
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;
}
}
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 {
public 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 enemyCow = col + colDirection.toInt();
return Coordinate.isValid(enemyRow, enemyCow)
&& isOccupiedByEnemy(enemyRow, enemyCow);
}
private boolean canJump(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);
}
private int rowDirection() {
return currentPlayer == Player.WHITE ? -1 : 1;
}
public boolean isEmpty(int row, int col) {
return board[row][col].isEmpty();
}
public boolean isOccupiedByCurrentPlayer(int row, int col) {
return board[row][col].isOccupiedBy(currentPlayer);
}
public boolean isOccupiedByEnemy(int row, int col) {
return !board[row][col].isEmpty()
&& board[row][col].isOccupiedBy(currentPlayer);
}
enum ColumnDirection {
LEFT(-1), RIGHT(1);
private int ratio;
ColumnDirection(int ratio) {
this.ratio = ratio;
}
public int toInt() {
return ratio;
}
}
}