Tic-Tac-Toe game in Java

Question

I had to write a TicTacToe game as an assignment for class & the last program I wrote used a few continues here and there. When I asked for a peer code-review I was informed that I should use additional variables rather than continue and break for my use-case. Did I make the same mistake in this code?

import java.util.HashMap;
import java.util.Scanner;

public class TicTacToe {

    public enum Box { 

        BAD_PLACEMENT(-2),
        X(-1), 
        EMPTY(0), 
        O(1),
        SOLVED(2),
        RESET_GAME(3);
        
        public int value;
        
        @Override
        public String toString() {
            return Integer.toString(value);
        }
        
        public String asString() {
            String rtn;
            switch(value) {
            case -1:
                rtn = "X";
                break;
            case 0:
                rtn = "-";
                break;
            case 1:
                rtn = "O";
                break;
            default:
                rtn = "Something has gone terribly wrong";
            }
            return rtn;
        }
        
        Box(int boxType){
            this.value = boxType;
        }
    }
    
    public static boolean isInteger(String s) {
        try { 
            Integer.parseInt(s); 
        } catch(NumberFormatException e) { 
            return false; 
        } catch(NullPointerException e) {
            return false;
        }
        // only got here if we didn't return false
        return true;
    }
    
    public static class Board {
        
        // The referee will handle computing tictactoe solves
        public class Referee {
            
            Board currentBoard;
            
            public Referee(Board b) {
                this.currentBoard = b;
            }
            
            public Board getBoard() {
                return this.currentBoard;
            }
            
            public Box checkSolveColumns() {
                for (int x=0; x <= 2; x++) {
                    
                    int currentSum = 0;
                    
                    for(int y=0; y <= 2; y++)
                        currentSum+=getBoard().getBox(x, y).value;
                        
                    if(currentSum == 3)
                        return Box.X;
                    if(currentSum == -3)
                        return Box.O;
                }
                return Box.EMPTY;
            }
            
            public Box checkSolveRows() {
                for (int y=0; y <= 2; y++) {
                    
                    int currentSum = 0;
                    
                    for(int x=0; x <= 2; x++)
                        currentSum+=getBoard().getBox(x, y).value;
                    if(currentSum == 3)
                        return Box.X;
                    if(currentSum == -3)
                        return Box.O;
                }
                return Box.EMPTY;
            }
            
            public Box checkSolveDiagonals() {
                
                int topLeftToBottomRightSum = 0;
                int topRightToBottomLeftSum = 0;
                                                                         //  0  1  2 
                topLeftToBottomRightSum+=getBoard().getBox(0, 0).value;// 0 [X][ ][ ]
                topLeftToBottomRightSum+=getBoard().getBox(1, 1).value;// 1 [ ][X][ ]
                topLeftToBottomRightSum+=getBoard().getBox(2, 2).value;// 2 [ ][ ][X]
                                                                         //  0  1  2 
                topRightToBottomLeftSum+=getBoard().getBox(2, 0).value;// 0 [ ][ ][X]
                topRightToBottomLeftSum+=getBoard().getBox(1, 1).value;// 1 [ ][X][ ]
                topRightToBottomLeftSum+=getBoard().getBox(0, 2).value;// 2 [X][ ][ ]

                if(topLeftToBottomRightSum == 3 || topRightToBottomLeftSum == 3)
                    return Box.X;
                if(topLeftToBottomRightSum == -3 || topRightToBottomLeftSum == -3)
                    return Box.O;
                
                return Box.EMPTY;
            }
            
            public Box checkSolve() {
                
                if(isFull())
                    return Box.SOLVED;
                
                // Observe this is "checked" not "check" > NAMING CONVENTIONS ROCK!!!
                Box checkedSolveRows = checkSolveRows();
                Box checkedSolveColumns = checkSolveColumns();
                Box checkedSolveDiagonals = checkSolveDiagonals();
                
                // return whatever guy won
                if(checkedSolveRows != Box.EMPTY) {
                    return checkedSolveRows;}
                if(checkedSolveColumns != Box.EMPTY) {
                    return checkedSolveColumns;}
                if(checkedSolveDiagonals != Box.EMPTY) {
                    return checkedSolveDiagonals;}
                
                // fall back to an empty return
            return Box.EMPTY;
            }
            
        }
        
        Referee referee;
        
        public Box boardInternal[][];
        
        public Board() {
            this.referee = new Referee(this);
            this.boardInternal = new Box[3][3];
            for(int x=0; x<=2; x++) {
                for(int y=0; y<=2; y++) {
                    this.boardInternal[x][y] = Box.EMPTY; // initialize every slot with an empty box
                }
            }
            /* Ideally, this is our data structure for the boxes
             * 
             * [EMPTY][EMPTY][EMTPY]
             * [EMPTY][EMPTY][EMTPY]
             * [EMPTY][EMPTY][EMTPY]
             */
        }
        
        public Box getBox(int x, int y) {
            if(x > 2 || y > 2 || x < 0 || y < 0)
                return Box.BAD_PLACEMENT;
            
            return this.boardInternal[x][y];
        }
        
        public boolean isFull() {
            // neat trick i picked up from lua to check a boolean in a return method
            for(int x=0; x<=2; x++) {
                for(int y=0; y<=2; y++) {
                    if(getBox(x, y)==Box.EMPTY)
                        return false;
                }
            }
            return true;
        }
        
        public void resetGame() {
            // Reset the board c:
            this.referee = new Referee(this);
            this.boardInternal = new Box[3][3];
            for(int x=0; x<=2; x++) {
                for(int y=0; y<=2; y++) {
                    this.boardInternal[x][y] = Box.EMPTY; // initialize every slot with an empty box
                }
            }
            System.out.println("*** Game reset ***");
            System.out.println("*** Welcome to TicTacToe! ***");
            System.out.println("*** Type RESET to restart! ***");
            System.out.println("*** Type QUIT to exit! ***");
        }
        
        public Box setBox(int x, int y, Box player) {
            
            if(this.referee.checkSolve() == Box.SOLVED || player==Box.RESET_GAME) { resetGame(); return Box.RESET_GAME;} // board is already solved
            
            Box currentBox = getBox(x, y); // get the current value of the box
            
            Box rtn = null;
            
            switch(currentBox) {
                case EMPTY: // if the box is empty, we fill it with the player requesting the area
                    this.boardInternal[x][y] = player; // set the field
                    rtn = this.referee.checkSolve();
                    break; // break out of our switch
                case BAD_PLACEMENT:
                    System.out.println("That is a invalid placement option!");
                    rtn = Box.BAD_PLACEMENT;
                    break;
                default: // if the box is not EMPTY, we need to throw a invalid placement exception to notify the caller
                    System.out.println("That box is filled option!");
                    rtn = Box.BAD_PLACEMENT;
            }
        System.out.println(this.toString());
        return rtn;
        }
        
        public String toString() {
            StringBuilder boxResult = new StringBuilder(); // save the memories! (bad save the trees pun)
            
            boxResult.append(String.format("\n   1  2  3\nA [%s][%s][%s]\nB [%s][%s][%s]\nC [%s][%s][%s]\n",
                    this.boardInternal[0][0].asString(),
                    this.boardInternal[0][1].asString(),
                    this.boardInternal[0][2].asString(),
                    this.boardInternal[1][0].asString(),
                    this.boardInternal[1][1].asString(),
                    this.boardInternal[1][2].asString(),
                    this.boardInternal[2][0].asString(),
                    this.boardInternal[2][1].asString(),
                    this.boardInternal[2][2].asString()
                    ));
            
            return boxResult.toString();
        }
    }
    
    public static void main(String[] args) {
        Scanner keyboard = new Scanner(System.in);
        Board ourBoard = new Board();
        
        HashMap<String, Box> Players = new HashMap<String, Box>();
        Players.put("Player1", Box.X);
        Players.put("Player2", Box.O);
        
        String currentPlayer = "Player1";
        
        System.out.println("*** Welcome to TicTacToe! ***");
        System.out.println("*** Type RESET to restart! ***");
        System.out.println("*** Type QUIT to exit! ***");
        System.out.println(ourBoard.toString());
        do {
            System.out.printf("Your move, %s: ",currentPlayer);
            String desiredInput = keyboard.next();
            
            Box inputResult = null;
            
            if(desiredInput.toLowerCase().equals("quit")) // quit the program immediately
                break;
            if(desiredInput.toLowerCase().equals("reset")) // reset the game
                inputResult = ourBoard.setBox(0, 0, Box.RESET_GAME);
            else if(desiredInput != null) {
                // we got an input let's see where we're going with this
                if(desiredInput.length() != 2) {
                    // wrong answer buddy
                    System.out.printf("Hey, %s! Please enter your format in the format of \"LetterNumber\". ex. A2%n%s is in the wrong format!%n",
                            currentPlayer,
                            desiredInput);
                    continue;
                } else {
                    // this is probably the correct format... let's check
                    char columnPicked = desiredInput.toLowerCase().charAt(0);
                    String rowPicked = desiredInput.split("")[1]; // get the 2 characters input
                    
                    if(isInteger(rowPicked)) {
                        int decidedRow = Integer.parseInt(rowPicked);
                        decidedRow--;
                        switch(columnPicked) {
                        case 'a':
                            inputResult = ourBoard.setBox(0, decidedRow, Players.get(currentPlayer));
                            break;
                        case 'b':
                            inputResult = ourBoard.setBox(1, decidedRow, Players.get(currentPlayer));
                            break;
                        case 'c':
                            inputResult = ourBoard.setBox(2, decidedRow, Players.get(currentPlayer));
                            break;
                        default:
                            System.out.println("Could not decipher your column (A,B,C allowed.)");
                            continue;
                        }
                    } else {
                        System.out.println("Could not decipher your row (Only numbers allowed)");
                        continue;
                    }
                    
                }
            }
                
            // do a reset check here
            switch(inputResult) {
                case X:
                    System.out.println("Player 2 wins!"); // player2 win
                    currentPlayer = "Player1";
                    ourBoard.resetGame();
                    break;
                case O:
                    System.out.println("Player 1 wins!"); // player1 win
                    currentPlayer = "Player1";
                    ourBoard.resetGame();
                    break;
                case RESET_GAME:
                    System.out.printf("%n%s requested a reset for the previous game. Bad sport!%n", currentPlayer);
                    currentPlayer = "Player1";
                    break;
                case BAD_PLACEMENT:
                    // nothing to really handle here but we should catch every possible outcome
                    break;
                case SOLVED: // this doesn't mean solved, it's a draw
                    System.out.println("Draw game! Better luck next time!");
                    currentPlayer = "Player1";
                    ourBoard.resetGame();
                    break;
                case EMPTY:
                    // give each player a turn
                    if(currentPlayer.equals("Player1"))
                        currentPlayer = "Player2";
                    else
                        currentPlayer = "Player1";
                    break;
                default:
                    System.out.println("Something went wrong!");
                    System.out.println("RESULT: "+inputResult);
            }
            
        } while(true);
        
        keyboard.close();
    }

}
```

Answer 1

There are different opinions on this:

    public String asString() {
        String rtn;
        switch(value) {
        case -1:
            rtn = "X";
            break;
        case 0:
            rtn = "-";
            break;
        case 1:
            rtn = "O";
            break;
        default:
            rtn = "Something has gone terribly wrong";
        }
        return rtn;
    }

There are some who say that a function should have a single exit point, and this is what you have done. That makes a lot of sense in languages like C, where you have to clean up your resources before you exit, but Java does that automatically. So I prefer the other approach - return as soon as the work is complete:

    public String asString() {
        switch(value) {
        case -1: return "X";
        case 0: return "-";
        case 1: return "O";
        default:
            throw new IllegalStateException();
        }
    }

I've modified so that it always returns a 1-character string, and throws an exception in the invalid case (but see the caution about toString() calling this: overriding Object's toString() method, but I have to throw exceptions).

Answer 2

Your formatting is off at some places, use an automatic code formatter.

---

        BAD_PLACEMENT(-2),
        X(-1), 
        EMPTY(0), 
        O(1),
        SOLVED(2),
        RESET_GAME(3);

You're mixing state of a single box, global game-state and state of input. Ideally you wouldn't.

---

        @Override
        public String toString() {
            return Integer.toString(value);
        }

I'd keep the default toString, because that is what all debuggers will default to when you debug the application. Also, you theoretically constantly convert the value, it would be better to store the wanted value to begin with as a field on the enum itself (as you do with value).

---

        public String asString() {
            String rtn;
            switch(value) {
            case -1:
                rtn = "X";
                break;
            case 0:
                rtn = "-";
                break;
            case 1:
                rtn = "O";
                break;
            default:
                rtn = "Something has gone terribly wrong";
            }
            return rtn;
        }

You can remove this whole logic if you just store this representation as an additional field on the enum itself.

Also the name asString is rather poor, getPlaySymbol would be better.

Also you can directly return the value. I know, that there's the style of having only one exit. However, keeping this style would only be interesting if you need to do some cleanup afterwards, and that is nearly never the case in Java (mostly in C). So improve readability by returning directly, because that will clear any question what is returned.

---

Board currentBoard;

Why is this variable package-private?

---

public Referee(Board b) {

Don't shorten names just because you can. It makes the code harder to understand and maintain.

---

return this.currentBoard;

Normally you'd only use this when there's need to differentiate between a local and class variable (which should only be in the constructor or a setter). But that's personal preference.

---

                    for(int y=0; y <= 2; y++)
                        currentSum+=getBoard().getBox(x, y).value;

I'd advice to always use braces to define blocks/scopes to increase readability and decrease possibilities for mistakes.

---

            if(this.referee.checkSolve() == Box.SOLVED || player==Box.RESET_GAME) { resetGame(); return Box.RESET_GAME;} // board is already solved

Having this on a single line is evil.

---

HashMap<String, Box> Players = new HashMap<String, Box>();

Try to use the lowest common class or interface you can get away with to not bundle your logic by accident to specific implementations if not needed. In this case it would be Map.

Also typo, should be players.

---

// wrong answer buddy

I might be old and grumpy, but if you ain't got something useful to say in the comments, don't type that comment. It's a waste of everyones time at the end of the day and it is infuriating to find comments like "lol" or "whoopsie oopsie" in a large code-base that you need to work on.

---

Splitting the processing of the input logic into a few functions would help loosen up the main function.

Answer 3

I disagree wit the answer of @TobySpeight. Even in Java it is usefull to have a single exit out of your methods since it makes refactorings easier, especially if you want to split up methods that have grown into smaller ones.

In addition I find your method Box.asString() inconsistent with the concept of Java enums. You obviously already know that the enum values are Objects that you can configure via a constructor. You do that with the int-Value of the Box. So why don't you simply do the same for "translating" the int value to a String?

If you did your enum Box would change to this:

public enum Box { 

    BAD_PLACEMENT(-2,"Something has gone terribly wrong"),
    X(-1,"X"), 
    EMPTY(0,"-"), 
    O(1,"O"),
    SOLVED(2,"Something has gone terribly wrong"),
    RESET_GAME(3,"Something has gone terribly wrong");
    
    public int value;
    private final String representation;
    
    @Override
    public String toString() {
        return Integer.toString(value);
    }
    
    public String asString() {
        return representation;
    }
    
    Box(int boxType, String representation){
        this.value = boxType;
        this.representation = representation;
    }
}