4
\$\begingroup\$

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);

        }

    }

}
\$\endgroup\$
  • \$\begingroup\$ I'd say the most immediate improvement you could do would be to add comments. For our benefit, for your benefit, for future you's benefit, for mankind's benefit... Also avoid using just constants. The last few lines are full of 1s and 5s and %10s but they're just magic numbers at the moment. \$\endgroup\$ – hnefatl Aug 12 '17 at 22:23
  • 1
    \$\begingroup\$ @hnefatl I disagree! Instead of comments we want better identifier names and no magic numbers. \$\endgroup\$ – Timothy Truckle Aug 12 '17 at 22:24
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Completely agreed, except for the naming convention on boolean variables. The variables should have plain names (gameOver is perfect), but the getter (if any) should be named isGameOver. This is because there is no way in which if (!gameOver) is hard to read. \$\endgroup\$ – Roland Illig Aug 13 '17 at 8:00
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I dislike that isThree implicitly relies on "global" state. I'd prefer if it were explicit about the dependency on player \$\endgroup\$ – Vogel612 Aug 14 '17 at 8:28
  • \$\begingroup\$ @Vogel612 By the way, isThree also relies on the global state in board. I wrote it exactly this way because I wanted to keep isWon as short and concise as possible. And, checking for three in a row is really only ever needed for the current player, so I don't see a need to make that dependency explicit. \$\endgroup\$ – Roland Illig Aug 14 '17 at 19:41
1
\$\begingroup\$

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
}
\$\endgroup\$
  • \$\begingroup\$ Your last advice could lead to code that allocates a new scanner for each line, which is wrong. \$\endgroup\$ – Roland Illig Aug 13 '17 at 18:09
  • \$\begingroup\$ @RolandIllig I meant it as spanning the whole main() class... :) \$\endgroup\$ – h.j.k. Aug 14 '17 at 12:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.