2
\$\begingroup\$

Could you please have a look at my programming style? Is it ok? Do I create any redundancies in code which I haven't noticed?

import java.util.Scanner;

public class TicTacToe {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        int[][] board = {
                {0, 0, 0},
                {0, 0, 0},
                {0, 0, 0}
        };

        drawBoard(board); // Draw empty game board
        int row, column; // Cell coordinates on the board
        for (int i = 0; i < 9; i++) { // Game lasts 9 turns

            while (true) { // Infinite loop to read proper cell coordinates
                System.out.print("Enter a row (1, 2 or 3) for player " +
                        (i % 2 == 0 ? "X" : "O") + ": ");
                row = input.nextInt() - 1;
                System.out.print("Enter a column (1, 2 or 3) for player " +
                        (i % 2 == 0 ? "X" : "O") + ": ");
                column = input.nextInt() - 1;
                if (board[row][column] != 0) { // 0 denotes vacant cell
                    System.out.println("Row " + row + " and column " + column +
                            " is already occupied, try again");
                }
                else
                    break;
            }

            board[row][column] = (i % 2 == 0) ? 1 : 2;
            drawBoard(board);

            // Check whether one of the players won at the end of this turn
            switch (checkGameStatus(board)) {
                case 1:
                    System.out.println("X player won");
                    return; // Terminate program
                case 2:
                    System.out.println("O player won");
                    return;
            }
        }

        // In case after 9 turns nobody won
        System.out.println("It's a draw");
    }

    /** Visualize game board */
    public static void drawBoard(int[][] board) {
        for (int[] row : board) {
            for (int cell : row)
                switch (cell) {
                    case 0:
                        System.out.print("| ");
                        break;
                    case 1:
                        System.out.print("|X");
                        break;
                    case 2:
                        System.out.print("|O");
                        break;
                    default:
                        System.out.println("Wrong element on board");
                        System.exit(1);
                }
            System.out.println("|");
        }
    }

    /** Check whether current board state corresponds to the win of one
     * of the players */
    public static int checkGameStatus(int[][] board) {
        if (isFormLine(board, 1))
            return 1; // Player X won
        if (isFormLine(board, 2))
            return 2; // Player O won

        return 0; // Game continues
    }

    /** Checks whether digit in number parameter forms
     *  horizontal/vertical/diagonal line */
    public static boolean isFormLine(int[][] board, int number) {
        boolean lineFormed;

        // Check rows of the board for a line of 1 (denotes X) or 2 (denotes O)
        for (int i = 0; i < board.length; i++) {
            lineFormed = true;
            for (int j = 1; j < board[i].length; j++)
                if (board[i][j - 1] != number || board[i][j] != number)
                    lineFormed = false;
            if (lineFormed)
                return true; // Horizontal line formed
        }

        // Check columns
        for (int j = 0; j < board[0].length; j++) {
            lineFormed = true;
            for (int i = 1; i < board.length; i++)
                if (board[i - 1][j] != number || board[i][j] != number)
                    lineFormed = false;
            if (lineFormed)
                return true;
        }

        // Check major diagonal
        lineFormed = true;
        for (int i = 1; i < board.length; i++)
            if (board[i - 1][i - 1] != number || board[i][i] != number)
                lineFormed = false;
        if (lineFormed)
            return true;

        // Check minor diagonal
        lineFormed = true;
        for (int i = 1; i < board.length; i++)
            if (board[board.length - i][i - 1] != number ||
                    board[board.length - i - 1][i] != number)
                lineFormed = false;
        if (lineFormed)
            return true;

        return false; // If none of the lines is formed
    }
}

Sample run:

| | | |
| | | |
| | | |
Enter a row (1, 2 or 3) for player X: 1
3Enter a column (1, 2 or 3) for player X:
| | |X|
| | | |
| | | |
Enter a row (1, 2 or 3) for player O: 1
Enter a column (1, 2 or 3) for player O: 2
| |O|X|
| | | |
| | | |
Enter a row (1, 2 or 3) for player X: 2
Enter a column (1, 2 or 3) for player X: 2
| |O|X|
| |X| |
| | | |
Enter a row (1, 2 or 3) for player O: 3
Enter a column (1, 2 or 3) for player O: 3
| |O|X|
| |X| |
| | |O|
Enter a row (1, 2 or 3) for player X: 3
Enter a column (1, 2 or 3) for player X: 1
| |O|X|
| |X| |
|X| |O|
X player won

Process finished with exit code 0
\$\endgroup\$
3
\$\begingroup\$

Avoiding code repetition

System.out.print("Enter a row (1, 2 or 3) for player " +
        (i % 2 == 0 ? "X" : "O") + ": ");
row = input.nextInt() - 1;
System.out.print("Enter a column (1, 2 or 3) for player " +
        (i % 2 == 0 ? "X" : "O") + ": ");
column = input.nextInt() - 1;

These can be put into a method that should also handle int inputs safely... As it stands, if the user enters a non-integer input, input.nextInt() will throw an InputMismatchException. For example:

private static int getInteger(Scanner scanner, String description, String player) {
    System.out.printf("Enter a %s (1, 2 or 3) for player %s: %n", description, player);
    int result;
    // handle the safe parsing of user input here
    return result;
}

Tip: if you'll like an example of how to do the parsing, you can refer to one of my earlier answers for starters.

try-with-resources

If you are on Java 7 and above, you should use try-with-resources for efficient handling of the underlying I/O resource:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        // ...
        int row = getInteger(scanner, "row", currentPlayer);
        int column = getInteger(scanner, "column", currentPlayer);
    }
}

Representing players

Your choice of an int[][]-typed array, and 1, 2 to represent players is slightly odd, as it means you have an extra 'mapping' from those two digits to the appropriate "X" or "O" representation. You can consider using a char[][] array, so that you can place 'X' and 'O' in it. If, for some reason you still prefer an int[][] array, you can then consider using a String[] array to store the player types, and use a 0-based index to reference them:

String[] players = new String[]{ "X", "O" };
//  in other parts of the code, e.g. to display current player
int row = getInteger(scanner, "row", players[i % 2]);

Displaying the board

This ties in with the earlier section... When you have a slightly better way of representing players, displaying the board is simpler as well since you do not need a switch statement (or any extra statements, for that matter) to do the mapping. For example, if you are on Java 8 and have used the char[][] array approach:

// made it static and to return the String representation instead
private static String drawBoard(char[][] board) {
    return Arrays.stream(board).map(r -> new String(r).replaceAll("", "|"))
                .collect(Collectors.joining("\n"));
}
  1. Call Arrays.stream(T[]) with board to give us a Stream<char[]> stream.
  2. For each element of the stream, i.e. row, we convert it to a String, and then replaceAll() to wrap each letter with "|".
  3. Finally, we perform a collect() by joining() each row with a newline.
\$\endgroup\$
  • \$\begingroup\$ Thanks for valuable and detailed advice, especially "Displaying the board" section of your answer. PS If I follow your suggestion to "use try-with-resources" which would be better to use Scanner or input stream for reading input, if I decide to add unit tests for example? \$\endgroup\$ – Alexander Oct 30 '15 at 8:38
  • 1
    \$\begingroup\$ My suggestion is for the Scanner as it has support for parsing the input the desired type. \$\endgroup\$ – h.j.k. Oct 30 '15 at 8:57
  • \$\begingroup\$ Why new String(r) instead of r in drawBoard? \$\endgroup\$ – Landei Oct 30 '15 at 10:56
  • 1
    \$\begingroup\$ @Landei r is a char[] array, hence the conversion. \$\endgroup\$ – h.j.k. Oct 30 '15 at 12:21
2
\$\begingroup\$
(i % 2 == 0) ? ... : ...

In the first part of your code, you repeat this a lot, whether it is written like this:

(i % 2 == 0 ? "X" : "O")

or like this:

(i % 2 == 0) ? 1 : 2;

You should store, using i % 2 == 0, an index to an array that contains the player symbols. That would look like this:

char[] players = {'X', 'O'};
...
int player = i % 2 == 0 ? 1 : 0;

Then, you can get the player symbol like this:

players[player]

And you can get the place to draw the board like:

board[row][column] = player + 1;

You don't have any input validation.

What if I enter a number like -9001? You're program is probably going to throw an ArrayIndexOutOfBounds error. You should do some checking on the input received to make sure that the numbers are in the bounds of your board.


            System.out.print("Enter a row (1, 2 or 3) for player " +
                    (i % 2 == 0 ? "X" : "O") + ": ");
            row = input.nextInt() - 1;
            System.out.print("Enter a column (1, 2 or 3) for player " +
                    (i % 2 == 0 ? "X" : "O") + ": ");
            column = input.nextInt() - 1;

You shouldn't have to prompt the user twice for the row and column of the place that the user would like to go.

  1. It is tedious for the user.
  2. It adds repeated code.

If you are going to use a coordinate system for your board, then I recommend that you prompt the user for both the numbers at once, perhaps delimited by a comma.

An example of user input would be:

1,2

Now, all you have to do is parse the input for the two numbers:

String[] coords = input.split("\D");

Then, you can just parse each number.


You should split up your code more. In specific, I recommend creating a method for running/playing the game.

And my main reason for this is unit testing. Right now, your code is automatically going to read from System.in via a Scanner.

This does not make your code very testable. How can you write automated tests if you have to type in all the input yourself?

If you do split the code for running the game into a separate method, make sure to have this method take in, via parameter, a stream/scanner. Tests will pass in streams with preset strings and, when a user is going to actually play your game, a stream reading from System.in will be passed in.

Note that, if you do this, the method may/will also need a parameter specifying the output stream so that the tests can read the output.


Small note: always use braces.

    if (isFormLine(board, 1))
        return 1; // Player X won
    if (isFormLine(board, 2))
        return 2; // Player O won

Without them, unexpected and hard-to-find bugs can arise.


            for (int cell : row)
                switch (cell) {
                    case 0:
                        System.out.print("| ");
                        break;
                    case 1:
                        System.out.print("|X");
                        break;
                    case 2:
                        System.out.print("|O");
                        break;
                    default:
                        System.out.println("Wrong element on board");
                        System.exit(1);
                }
            System.out.println("|");

I see what you are trying to do with printing out that extra | at the end, but it's really unnecessary; you're only printing a few characters in total.

Avoid the extra println call and just print the | at the end of each print call (which can now become println calls`).

\$\endgroup\$
  • \$\begingroup\$ Many thanks for thorough answer! Two more questions: 1. "make sure to have this method take in, via parameter, a stream/scanner." - How method can determine whether a stream or a scanner are passed or I need attach a magic number denoting stream or scanner? 2. "Avoid the extra println call and just print the | at the end of each print call (which can now become println calls`)" - Each print drawing one cell and sense of println is to move on to the next line when one row of a board is printed. If I change each print("|X") to println("|X|") then output would be not 3x3 board but 1x9 board. \$\endgroup\$ – Alexander Oct 30 '15 at 8:17
  • 1
    \$\begingroup\$ When I said stream/scanner, I meant it's up to you: you can choose to keep using a Scanner, or you can pass in, for example, a BufferedInputStream (but a Scanner is just fine). And yes, now after looking back at the code, I see where my mistake is. The reason why I thought that you could make that change is because you omitted the braces on the inner for-loop. As stated, always use braces. \$\endgroup\$ – SirPython Oct 30 '15 at 22:21

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.