Novice Tic Tac Toe in Java

Question

I've never done anything in java, but today I had a couple hours of free time so I decided I would learn it and make a Tic Tac Toe game. Please give me some constructive criticism on anything you would do differently.

import java.util.Scanner;
import java.util.stream.IntStream;;

public class TicTacToe {

    static Scanner scan = new Scanner(System.in);
    static char board[] = {'A','B','C','D','E','F','G','H','I'};
    static final int scoreboard[] = {1,2,7,6,0,4,3,8,9};
    static int player = 0;
    static boolean gameOver = false;
    static int score[][] = {{0,0,0,0,0},{0,0,0,0}};
    static int turn[] = {0,0,5};

    public static void main(String[] args) {
        while (gameOver == false) {

            askForTurn();

        }
    }

    public static void askForTurn() {

        System.out.printf("\n-------------\n| %s | %s | %s |\n-------------\n| %s | %s | %s |\n-------------\n| %s | %s | %s |\n-------------",board[0],board[1],board[2],board[3],board[4],board[5],board[6],board[7],board[8]);
        boolean valid = false;
        System.out.printf("\nPlayer %o's turn:", (player+1));
        char input = scan.next().toUpperCase().charAt(0);
        int i;
        for (i = 0; i < 9; i++) {

            if (input == board[i]) {

                if (player == 0) {

                    valid = true;
                    board[i] = 'O';
                    score[0][turn[0]] = scoreboard[i];
                    turn[0]++;

                } else {

                    valid = true;
                    board[i] = 'X';
                    score[1][turn[1]] = scoreboard[i];
                    turn[1]++;

                }

            }

        }

        if (valid == false) {

            System.out.print("\nInvalid square");
            askForTurn();

        } else {

            checkForWin();
            player++;

            if (player%2 == 0) {

                player = 0;
            }

        }

    }

    public static void checkForWin() {

        int sum1 = IntStream.of(score[0]).sum();
        int sum2 = IntStream.of(score[1]).sum();
        int x;

        ifWin(1, 0, 3, sum1%10, 0);
        ifWin(1, 0, 3, sum2%10, 1);

        for (x = 0; x < turn[0]; x++) {

            ifWin(1, 0, 4, (sum1 - score[0][x])%10, 0);

        }

        for (x = 0; x < (turn[1] - 1); x++) {

            ifWin(1, 0, 4, (sum2 - score[1][x])%10, 1);

        }

        if (turn[0] == 5) {

            for (x = 0; x < 3; x++) {

                ifWin(1, 0, 5, (sum1-score[0][0]-score[0][x+1])%10, 0);
                ifWin(x, 0, 5, (sum1-score[0][1]-score[0][x+1])%10, 0);
                ifWin(x, 1, 5, (sum1-score[0][2]-score[0][3])%10, 0);
                ifWin(x, 1, 5, 0, 2);

            }

        }

    }

    public static void endGame(int player) {

        if (player == 3) {

            System.out.print("\nTie!");

        } else {

            System.out.printf("Player %o wins!", player);

        }

        gameOver = true;

    }

    public static void ifWin(int x, int z, int turnCheck, int statement, int player) {

        if (x > z && turn[player] == turnCheck && statement == 0) {

            endGame(player + 1);

        }

    }

}

Answer 1

Thanks for sharing your code.

This looks pretty good for a novices code since you managed to remove some code duplication by using parameterized methods.

here is what I would change:

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Naming Conventions

Please read (and follow) the Java Naming Conventions.

eg .: variables of type boolean (like gameOver) or methods returning a boolean should start with *is, has, can or alike.

Don't surprise your readers

Your method askForTurn() does nor only acquire user input, it actually does the turn. So it should better be named doTurn().

Magic numbers

replace the literal numbers by constants with expressive names.

Separation of Concerns

askForTurn() does more than one thing:

• it acquires user input,
• validates the user input
• translates the input into a field position
• checks the end condition

each of this actions should live in a method of its own.

hidden recursion/implicit error handling

In case of an invalid input you do a recursive call to askForTurn(). Beside that this also belongs in the Don't surprise your readers category it is a potential memory leak.

Use of the static key word

The only method that should be static in your code should be main. For the rest of your code you should avoid the static key word unless you have a good reason to use it.

In your case it would be quiet easy to do:
move the while loop from main into a new (non static) method and call it on an instance of the class:

public static void main(String[] args) {
    new TicTacToe().runTheGame();
}

This would safe you from all the statics in your program.

Answer 2

Your method of checking for a winner is so tricky that you should give the reader some hints. Or, better yet, replace it with simple, obvious code instead of using modulo arithmetics. Here is how I would do it:

boolean isThree(int a, int b, int c) {
    char playerChar = player == 0 ? 'O' : 'X';
    return board[a] == playerChar && board[b] == playerChar && board[c] == playerChar;
}

The above method checks a single row or column or diagonal, depending on the board indexes that are passed in the parameters. Then it becomes simple.

boolean isWon() {
    return isThree(0, 1, 2) || isThree(3, 4, 5) || isThree(6, 7, 8) // horizontal
        || isThree(0, 3, 6) || isThree(1, 4, 7) || isThree(2, 5, 8) // vertical
        || isThree(0, 4, 8) // top-left to bottom-right
        || isThree(2, 4, 6); // top-right to bottom-left
}

You don't need to check for the other player at all since he didn't make a turn, so nothing has changed his winning conditions.

---

You never modify turn[2], so there's no point in checking whether it is still 0.

---

Instead of that large printf line, I extracted the board-printing code to a separate method:

private void printBoard() {
    System.out.printf("-------------%n");
    System.out.printf("| %s | %s | %s |%n", board[0], board[1], board[2]);
    System.out.printf("-------------%n");
    System.out.printf("| %s | %s | %s |%n", board[3], board[4], board[5]);
    System.out.printf("-------------%n");
    System.out.printf("| %s | %s | %s |%n", board[6], board[7], board[8]);
    System.out.printf("-------------%n");
}

Look how nicely the code resembles the actual output on the screen.

Answer 3

In addition to @TimothyTruckle's answer...

Whitespacing

Your code has too much vertical whitespaces (i.e. blank lines), and yet I feel can do with more hornizontal whitespace (i.e. spaces between variables and operators) instead. For example, I might format this block as such:

for (x = 0; x < 3; x++) {
    ifWin(1, 0, 5, (sum1 - score[0][0] - score[0][x+1]) % 10, 0);
    ifWin(x, 0, 5, (sum1 - score[0][1] - score[0][x+1]) % 10, 0);
    ifWin(x, 1, 5, (sum1 - score[0][2] - score[0][3]) % 10, 0);
    ifWin(x, 1, 5, 0, 2);
}

Modifiers

Keep your variables private! :)

Boolean test

Instead of gameOver == false, you can just do !gameOver.

try-with-resources

Since Java 7, you should use try-with-resources on your Scanner for safe and efficient handling of the underlying I/O resource:

try (Scanner scanner = new Scanner(System.in)) {
    String input = scanner.nextLine();
    // other operations
}