patternjavaMinor
Tic-Tac-Toe design
Viewed 0 times
tacdesigntictoe
Problem
Kindly look at this code and point out design flaws or areas for improvement. I think I've done the last part right (the enum). I think the rest of the design is flawed but I am very new to object-oriented programming.
Game
```
//Game class establishes rules and finds winner
public class Game {
String[] gameState = new String[9];
Player player1;
Player player2;
// updates the gamestate array with the latest move
public void updateStatus(int position, String symbol) {
gameState[position] = symbol;
}
public Game() {
player1 = new Player(1);
player2 = new Player(2);
for(int i = 0; i < 9; i++) {
gameState[i] = "";
}
}
// checks if game over. If game over, return winner or tie, else return "Game in prgress"
public String getGameStatus() {
if((gameState[0].equals(gameState[1]))
&& gameState[0].equals(gameState[2])
&& gameState[1].equals(gameState[2])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[3].equals(gameState[4]))
&& gameState[3].equals(gameState[5])
&& gameState[4].equals(gameState[5])
&& !gameState[3].equals("")){
return gameState[3];
}
else if((gameState[6].equals(gameState[7]))
&& gameState[6].equals(gameState[8])
&& gameState[7].equals(gameState[8])
&& !gameState[6].equals("")){
return gameState[6];
}
else if((gameState[0].equals(gameState[3]))
&& gameState[0].equals(gameState[6])
&& gameState[3].equals(gameState[6])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[1].equals(gameState[4]))
&& gameState[1].equals(gameState[7])
&& gameState[4].equals(gameState[7])
&& !gameState[1].equals("")){
return gameState[1];
}
else if((gameState[2].equals(gameState[5]))
&& gameState[2].equals(gameState
Game
```
//Game class establishes rules and finds winner
public class Game {
String[] gameState = new String[9];
Player player1;
Player player2;
// updates the gamestate array with the latest move
public void updateStatus(int position, String symbol) {
gameState[position] = symbol;
}
public Game() {
player1 = new Player(1);
player2 = new Player(2);
for(int i = 0; i < 9; i++) {
gameState[i] = "";
}
}
// checks if game over. If game over, return winner or tie, else return "Game in prgress"
public String getGameStatus() {
if((gameState[0].equals(gameState[1]))
&& gameState[0].equals(gameState[2])
&& gameState[1].equals(gameState[2])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[3].equals(gameState[4]))
&& gameState[3].equals(gameState[5])
&& gameState[4].equals(gameState[5])
&& !gameState[3].equals("")){
return gameState[3];
}
else if((gameState[6].equals(gameState[7]))
&& gameState[6].equals(gameState[8])
&& gameState[7].equals(gameState[8])
&& !gameState[6].equals("")){
return gameState[6];
}
else if((gameState[0].equals(gameState[3]))
&& gameState[0].equals(gameState[6])
&& gameState[3].equals(gameState[6])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[1].equals(gameState[4]))
&& gameState[1].equals(gameState[7])
&& gameState[4].equals(gameState[7])
&& !gameState[1].equals("")){
return gameState[1];
}
else if((gameState[2].equals(gameState[5]))
&& gameState[2].equals(gameState
Solution
If you like enums, this example has two of them. The code is untested but might give you an idea.
Further improvements:
public class Game {
private final Symbol[] gameState = new Symbol[9];
private final Symbol player1;
private final Symbol player2;
private Symbol currentPlayer;
public void updateStatus(int position, Symbol symbol) {
gameState[position] = symbol;
}
public Game() {
player1 = Symbol.X;
player2 = Symbol.O;
currentPlayer = player1;
Arrays.fill(gameState, Symbol.EMPTY);
}
public GameState getGameStatus() {
// check horizontal
for (int i = 0; i < 9; i += 3) {
if (gameState[i] != Symbol.EMPTY
&& gameState[i] == gameState[i+1]
&& gameState[i+1] == gameState[i+2]) {
return gameState[i].won();
}
}
// check vertical
for (int i = 0; i < 3; i ++) {
if (gameState[i] != Symbol.EMPTY
&& gameState[i] == gameState[i+3]
&& gameState[i+3] == gameState[i+6]) {
return gameState[i].won();
}
}
// check diagonal
if (gameState[0] != Symbol.EMPTY
&& gameState[0] == gameState[4]
&& gameState[4] == gameState[9]) {
return gameState[0].won();
}
if (gameState[2] != Symbol.EMPTY
&& gameState[2] == gameState[4]
&& gameState[4] == gameState[7]) {
return gameState[2].won();
}
// check in progress
for (int i = 0; i < 9; i++) {
if (gameState[i] == Symbol.EMPTY) {
return GameState.RUNNING;
}
}
// draw
return GameState.DRAW;
}
public Symbol play(int position) {
Symbol playing = currentPlayer;
updateStatus(position, playing);
currentPlayer = currentPlayer == player1 ? player2 : player1;
return playing;
}
}
public enum Symbol {
EMPTY(null),
X(GameState.X_WON),
O(GameState.O_WON);
private final GameState won;
private Symbol(GameState won) {
this.won = won;
}
public GameState won() {
return won;
}
}
public enum GameState {
RUNNING("Game in progress"),
X_WON("X won"),
O_WON("O won"),
DRAW("It's a tie!!");
private String message;
private GameState(String message) {
this.message = message;
}
@Override
public String toString() {
return message;
}
}- use loops to reduce the number of if/else statements
- use meaningful objects wherever possible
- use use
Strings only where necessary
- reduce the number of mutable objects (here we have only one)
Further improvements:
- reduce the number of public methods
- split long methods (
getGameStatusin this case)
- reduce the number of conditionals in
getGameStatus, it is still spaghetti code
Code Snippets
public class Game {
private final Symbol[] gameState = new Symbol[9];
private final Symbol player1;
private final Symbol player2;
private Symbol currentPlayer;
public void updateStatus(int position, Symbol symbol) {
gameState[position] = symbol;
}
public Game() {
player1 = Symbol.X;
player2 = Symbol.O;
currentPlayer = player1;
Arrays.fill(gameState, Symbol.EMPTY);
}
public GameState getGameStatus() {
// check horizontal
for (int i = 0; i < 9; i += 3) {
if (gameState[i] != Symbol.EMPTY
&& gameState[i] == gameState[i+1]
&& gameState[i+1] == gameState[i+2]) {
return gameState[i].won();
}
}
// check vertical
for (int i = 0; i < 3; i ++) {
if (gameState[i] != Symbol.EMPTY
&& gameState[i] == gameState[i+3]
&& gameState[i+3] == gameState[i+6]) {
return gameState[i].won();
}
}
// check diagonal
if (gameState[0] != Symbol.EMPTY
&& gameState[0] == gameState[4]
&& gameState[4] == gameState[9]) {
return gameState[0].won();
}
if (gameState[2] != Symbol.EMPTY
&& gameState[2] == gameState[4]
&& gameState[4] == gameState[7]) {
return gameState[2].won();
}
// check in progress
for (int i = 0; i < 9; i++) {
if (gameState[i] == Symbol.EMPTY) {
return GameState.RUNNING;
}
}
// draw
return GameState.DRAW;
}
public Symbol play(int position) {
Symbol playing = currentPlayer;
updateStatus(position, playing);
currentPlayer = currentPlayer == player1 ? player2 : player1;
return playing;
}
}
public enum Symbol {
EMPTY(null),
X(GameState.X_WON),
O(GameState.O_WON);
private final GameState won;
private Symbol(GameState won) {
this.won = won;
}
public GameState won() {
return won;
}
}
public enum GameState {
RUNNING("Game in progress"),
X_WON("X won"),
O_WON("O won"),
DRAW("It's a tie!!");
private String message;
private GameState(String message) {
this.message = message;
}
@Override
public String toString() {
return message;
}
}Context
StackExchange Code Review Q#31769, answer score: 5
Revisions (0)
No revisions yet.