## Use [Java 16 Records][record]
Coordinate is a typical transparent data-carrier.
```
record Coordinate(int row, int col) { }
```
## Apply [Information expert principle][information_expert]
`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]
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][code_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][self-doc], 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()`][formatted] when you need to inject data into these static final strings.
## 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()`][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.
<sup>
<b>Note</b> that what I told about <i>Coordinate</i> is an observation made base the code you shared, <b>not</b> a strong recomendation. After introducing a series of changes it might still hold true, or might not.
The point is that you need to develop a habit of a making small incremental changes while evolving your disign and constanly evaluating the code elements you're work with at the moment.
The driving motivation behind every design change should be your understanding of the value this change might bring.
It's normal to revert the change if it doesn't improve the design. It's also normal for individual components of the system to be discarded when they no longer serve their purpose or don't play well with each other. In such cases, it may be necessary to introduce different abstractions to facilitate the addition of a new feature.
The abstractions I proposed to add (<i>Cell</i> and <i>Player</i>) are also not something set in stone. Introducing them into the code is a feasible and relatively small change which you can immediately leverage to improve the code quality. After you finish with this change, evaluate your design. You might come to a conclusion that you need to alter them in someway, or you might decide to introduce <i>Board</i> abstraction and incorporate them into it.
</sup>
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);
}
[record]: https://docs.oracle.com/en/java/javase/22/language/records.html
[information_expert]: https://en.wikipedia.org/wiki/GRASP_(object-oriented_design)#Information_expert
[switch]: https://docs.oracle.com/en/java/javase/17/language/switch-expressions.html
[self-doc]: https://en.wikipedia.org/wiki/Self-documenting_code
[formatted]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#formatted(java.lang.Object...)
[code_smell]: https://en.wikipedia.org/wiki/Code_smell
[ordinal]: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Enum.html#ordinal()