Basic TicTacToe game in java

Question

I was assigned to create a simple Tic Tac Toe game in java for class, and here is what I got. I would like to know how the code could possibly be shortened without using really advanced topics since I haven't covered them yet. Here goes:

import java.util.*;

import javax.swing.JOptionPane;

public class TicTacToe {
    enum Turn {PLAYER, COMPUTER};

    public static void main(String[] args) {
        char[][] gameBoard = {{'1','2','3'},
                              {'4','5','6'},
                              {'7','8','9'}};

        ArrayList<Integer> choicesMade = new ArrayList<Integer>();

        Turn turn = Turn.PLAYER;


        while(!boardFull(gameBoard)) {
            if(turn.equals(Turn.PLAYER)) {
                int userChoice = getUserChoice(gameBoard);
                if(!placeTaken(userChoice, choicesMade)) {
                    turn = Turn.COMPUTER;
                    choicesMade = addToArray(userChoice, choicesMade);
                    gameBoard = changeBoard(userChoice, gameBoard, 'X');
                    if(checkHorizontalWin(gameBoard, 'X')|| checkVerticalWin(gameBoard, 'X') || checkDiagonalWin(gameBoard, 'X')) {
                        JOptionPane.showMessageDialog(null, "You Win!");
                        break;
                    }
                }
                else {
                    JOptionPane.showMessageDialog(null, "That place is taken! Please try again!");
                }
            }
            if(turn.equals(Turn.COMPUTER)) {
                int computerChoice = 1 + (int)(Math.random() * 9);
                if(!placeTaken(computerChoice, choicesMade)) {
                    turn = Turn.PLAYER;
                    choicesMade = addToArray(computerChoice, choicesMade);
                    gameBoard = changeBoard(computerChoice, gameBoard, 'O');
                    displayBoard(gameBoard);
                    if(checkHorizontalWin(gameBoard, 'O') || checkVerticalWin(gameBoard, 'O') || checkDiagonalWin(gameBoard, 'O')) {
                        JOptionPane.showMessageDialog(null, "Computer Wins!");
                        break;
                    }
                }
            }
        }
    }
    public static void displayBoard(char[][] gameBoard) {
        String output = "";
        for(int i = 0; i < gameBoard.length; i++) {
            output += Arrays.toString(gameBoard[i]) + "\n";
        }
        JOptionPane.showMessageDialog(null, output);
    }

    public static boolean boardFull (char[][] gameBoard) {
        for(int i = 0; i < gameBoard.length; i++) {
            for(int j = 0; j < gameBoard[i].length; j++) {
                if(Character.isDigit(gameBoard[i][j])) {
                    return false;
                }
            }
        }
        return true;
    }

    public static int getUserChoice(char[][] gameBoard) {
        String output = "";
        for(int i = 0; i < gameBoard.length; i++) {
            output += Arrays.toString(gameBoard[i]) + "\n";
        }
        String userInput = JOptionPane.showInputDialog(null, output);
        int choice = Integer.parseInt(userInput);

        return choice;
    }
    public static boolean placeTaken(int userChoice, ArrayList<Integer> choices) {
        if(choices.contains(userChoice)) {
            return true;
        }
        return false;
    }
    public static ArrayList<Integer> addToArray(int userChoice, ArrayList<Integer> choicesMade) {
        choicesMade.add(userChoice);
        return choicesMade;
    }
    public static char[][] changeBoard(int userChoice, char[][] gameBoard, char characterPlaceholder) {
        switch(userChoice) {
            case 1:
                gameBoard = updateBoard(gameBoard, 0, 0, characterPlaceholder);
                break;
            case 2:
                gameBoard = updateBoard(gameBoard, 0, 1, characterPlaceholder);
                break;
            case 3:
                gameBoard = updateBoard(gameBoard, 0, 2, characterPlaceholder);
                break;
            case 4:
                gameBoard = updateBoard(gameBoard, 1, 0, characterPlaceholder);
                break;
            case 5:
                gameBoard = updateBoard(gameBoard, 1, 1, characterPlaceholder);
                break;
            case 6:
                gameBoard = updateBoard(gameBoard, 1, 2, characterPlaceholder);
                break;
            case 7:
                gameBoard = updateBoard(gameBoard, 2, 0, characterPlaceholder);
                break;
            case 8:
                gameBoard = updateBoard(gameBoard, 2, 1, characterPlaceholder);
                break;
            case 9:
                gameBoard = updateBoard(gameBoard, 2, 2, characterPlaceholder);
                break;
        }

        return gameBoard;
    }
    public static char[][] updateBoard(char[][] gameBoard, int row, int column, char placeHolder) {
        gameBoard[row][column] = placeHolder;
        return gameBoard;
    }

    public static boolean checkHorizontalWin(char [][] gameBoard, char character) {
        if(gameBoard[0][0] == character && gameBoard[0][1] == character && gameBoard[0][2] == character) {
            return true;
        }
        else if(gameBoard[1][0] == character && gameBoard[1][1] == character && gameBoard[1][2] == character) {
            return true;
        }
        else if(gameBoard[2][0] == character && gameBoard[2][1] == character && gameBoard[2][2] == character) {
            return true;
        }

        return false;
    }

    public static boolean checkVerticalWin(char[][] gameBoard, char character) {
        if(gameBoard[0][0] == character && gameBoard[1][0] == character && gameBoard[2][0] == character) {
            return true;
        }
        else if(gameBoard[0][1] == character && gameBoard[1][1] == character && gameBoard[2][1] == character) {
            return true;
        }
        else if(gameBoard[0][2] == character && gameBoard[1][2] == character && gameBoard[2][2] == character) {
            return true;
        }

        return false;
    }

    public static boolean checkDiagonalWin(char[][] gameBoard, char character) {
        if(gameBoard[0][0] == character && gameBoard[1][1] == character && gameBoard[2][2] == character) {
            return true;
        }
        else if(gameBoard[0][2] == character && gameBoard[1][1] == character && gameBoard[2][0] == character) {
            return true;
        }

        return false;
    }

}

Answer 1

There's lots of duplication here. Cleaning that up will leave things a lot shorter.

 switch(userChoice) {
        case 1:
            gameBoard = updateBoard(gameBoard, 0, 0, characterPlaceholder);
            break;
        case 2:
            gameBoard = updateBoard(gameBoard, 0, 1, characterPlaceholder);
            break;
        case 3:
            gameBoard = updateBoard(gameBoard, 0, 2, characterPlaceholder);
            break;
        case 4:
            gameBoard = updateBoard(gameBoard, 1, 0, characterPlaceholder);
            break;
        case 5:
            gameBoard = updateBoard(gameBoard, 1, 1, characterPlaceholder);
            break;
        case 6:
            gameBoard = updateBoard(gameBoard, 1, 2, characterPlaceholder);
            break;
        case 7:
            gameBoard = updateBoard(gameBoard, 2, 0, characterPlaceholder);
            break;
        case 8:
            gameBoard = updateBoard(gameBoard, 2, 1, characterPlaceholder);
            break;
        case 9:
            gameBoard = updateBoard(gameBoard, 2, 2, characterPlaceholder);
            break;
    }

There's an easy way to get the coordinates out of the board position numbering you're using, with integer division and the mod operator. Integer division is like ordinary division, throwing away the remainder, and the mod operator is like division where we throw away the result and keep the remainder. If we use userChoice / 3 and userChoice % 3, we almost get what we want. Subtracting 1 from userChoice will give us what we want.

Now we can rewrite it in a much shorter way:

int x = (userChoice - 1) / 3;
int y = (userChoice - 1) % 3;
gameBoard = updateBoard(gameBoard, x, y, characterPlaceholder);

Note that even if you didn't see this trick, there are other ways of writing it that are also much shorter. For example, you could have made a pair of arrays that let you look up the corresponding x and y coordinate for a grid position, and ended up with code like this:

gameBoard = updateBoard(gameBoard, x[userChoice], y[userChoice], characterPlaceholder);

This method looks at positions one at a time:

    public static boolean checkHorizontalWin(char [][] gameBoard, char character) {
    if(gameBoard[0][0] == character && gameBoard[0][1] == character && gameBoard[0][2] == character) {
        return true;
    }
    else if(gameBoard[1][0] == character && gameBoard[1][1] == character && gameBoard[1][2] == character) {
        return true;
    }
    else if(gameBoard[2][0] == character && gameBoard[2][1] == character && gameBoard[2][2] == character) {
        return true;
    }

    return false;
}

What we're doing, really, is checking each row. So let's do that more explicitly:

    public static boolean checkHorizontalWin(char [][] gameBoard, char character) {
        for(int i = 0; i < 3; i++) {
            if(gameBoard[i][0] == character && gameBoard[i][1] == character && gameBoard[i][2] == character) {
                return true;
            }
        }

        return false;
    }

You could go a step further and turn the if statement into a loop as well. That doesn't help us a ton now, but if your instructor suddenly decided he wanted you to code tic-tac-toe for a 5x5 board instead, that would make it easy instead of hard.

In this code, you do the exact same thing for a computer turn as for a player turn, with only a few differences:

 if(turn.equals(Turn.COMPUTER)) {
                int computerChoice = 1 + (int)(Math.random() * 9);
                if(!placeTaken(computerChoice, choicesMade)) {
                    turn = Turn.PLAYER;
                    choicesMade = addToArray(computerChoice, choicesMade);
                    gameBoard = changeBoard(computerChoice, gameBoard, 'O');
                    displayBoard(gameBoard);
                    if(checkHorizontalWin(gameBoard, 'O') || checkVerticalWin(gameBoard, 'O') || checkDiagonalWin(gameBoard, 'O')) {
                        JOptionPane.showMessageDialog(null, "Computer Wins!");
                        break;
                    }
                }

The differences are: the character representing a move, the way of changing whose turn it is, the message for a win, and the way of picking the move. You could have a checkForWin() method; all you'd have to tell it is what character to check for and what message to display for a win. You could have a changeTurn() method that would swap whose turn it is. You could have a checkMove() method that would check whether the move was valid.

You could probably make the main loop of the program into about 4 or 5 lines of code if you did all that (or something similar). Of course, it would be calling methods with a few lines of code each, and it would end up doing about the same amount of work, but looking at a couple of lines of code at a time where there's only one thing going on is a lot easier than looking at a couple of dozen lines of code where there are several different things going on.

If you think about what things might change, it can help you abstract your code enough to be more flexible. For example, you assume that the player will always be X and the computer will always be O. What if you needed the player to be O? What if you needed to let 2 computer players play against each other? What if you needed to let 2 humans play each other?

Answer 2

import java.util.*;

It is generally considered to be better to write out separate imports rather than use the * operator.

        ArrayList<Integer> choicesMade = new ArrayList<Integer>();

As a general rule, it is preferred to use the interface as the variable type rather than the implementation.

        List<Integer> choicesMade = new ArrayList<>();

If you are using the latest Java, you don't need to write out Integer twice. Just saying <> will allow the compiler to figure it out the second time.

        String output = "";
        for(int i = 0; i < gameBoard.length; i++) {
            output += Arrays.toString(gameBoard[i]) + "\n";
        }
        JOptionPane.showMessageDialog(null, output);

When building strings for output, it is usually better to use a StringBuilder.

        StringBuilder output = new StringBuilder();
        for (char[] row : gameBoard) {
            output.append(Arrays.toString(row) + "\n");
        }

        JOptionPane.showMessageDialog(null, output.toString());

Since a String is immutable, it creates a new object every time you use the += operator.

Also, you don't have to manually manage the loop iteration variable. Java will do it for you if you want.

        for(int i = 0; i < gameBoard.length; i++) {
            for(int j = 0; j < gameBoard[i].length; j++) {
                if(Character.isDigit(gameBoard[i][j])) {
                    return false;
                }
            }
        }

Again, you can use the Java for each form:

        for (char[] row : gameBoard) {
            for (char square : row) {
                if (Character.isDigit(square)) {
                    return false;
                }
            }
        }

I find that easier to read.

        if(choices.contains(userChoice)) {
            return true;
        }
        return false;

You can just say

        return choices.contains(userChoice);

There's no need for an if statement. Actually, there's no need for this method.

                if(!placeTaken(userChoice, choicesMade)) {

You could just say

                if (!choicesMade.contains(userChoice)) {

The method is just a rewriting of that.

    public static ArrayList<Integer> addToArray(int userChoice, ArrayList<Integer> choicesMade) {

Again, the general rule is to use the interface rather than the implementation. So

    public static List<Integer> addToArray(int userChoice, List<Integer> choicesMade) {

And it's not clear that you need this method either.

                    choicesMade = addToArray(userChoice, choicesMade);

You can just say

                    choicesMade.add(userChoice);

Shorter, more readable, and it doesn't require an extra method.

Similarly, consider

    public static char[][] changeBoard(int userChoice, char[][] gameBoard, char characterPlaceholder) {
        int choiceIndex = userChoice - 1;
        gameBoard[choiceIndex / gameBoard[0].length][choiceIndex % gameBoard[0].length] = characterPlaceholder;
    }

That saves a large switch statement and a helper method.