2
\$\begingroup\$

I created this game in Java:

import java.util.Scanner;
public class TicTacToe {
    public static Scanner input = new Scanner(System.in);
    public static String getName(int noPlayer) {
        System.out.print("Enter name of Player " + noPlayer + ": ");
        return input.next();
    }
    public static int getMove(String board[], String player) {
        printBoard(board);
        System.out.print("Enter move for " + player + ": ");
        int move = input.nextInt();
        while (moveTaken(board, move)) {
            System.out.println("Move taken.");
            System.out.print("Enter move for " + player + ": ");
            move = input.nextInt();
        }
        return move;
    }
    public static String gameResult(String board[]) {
        final int WINNING_COMBOS[][] = {{0, 1, 2}, {3, 4, 5}, {6, 7, 8},
                                        {0, 3, 6}, {1, 4, 7}, {2, 5, 8},
                                        {0, 4, 8}, {2, 4, 6}};
        for (int[] i: WINNING_COMBOS) {
            if (board[i[0]].equals(board[i[1]]) && board[i[0]].equals(board[i[2]]) && board[i[1]].equals(board[i[2]])) {
                if (board[i[0]].equals("O")) {
                    return "O wins";
                }
                else {
                    return "X wins";
                }
            }
        }
        if (!board[0].equals("0") && !board[1].equals("1") && !board[2].equals("2") &&
            !board[3].equals("3") && !board[4].equals("4") && !board[5].equals("5") &&
            !board[6].equals("6") && !board[7].equals("7") && !board[8].equals("8")) {
            return "draw";
        }
        return "not completed";
    }
    public static boolean moveTaken(String board[], int move) {
        if (board[move].equals("O") || board[move].equals("X")) {
            return true;
        }
        return false;
    }
    public static void printBoard(String board[]) {
        System.out.println(" " + board[0] + " | " + board[1] + " | " + board[2] +
                           "\n-----------\n" +
                           " " + board[3] + " | " + board[4] + " | " + board[5] +
                           "\n-----------\n" +
                           " " + board[6] + " | " + board[7] + " | " + board[8]);
    }
    public static void conclusion(String result, String pO, String pX) {
        if (result.equals("O wins")) {
            System.out.println(pO + " wins!");
        }
        else if (result.equals("X wins")) {
            System.out.println(pX + " wins!");
        }
        else {
            System.out.println("Draw.");
        }
    }
    public static void main(String args[]) {
        String O = getName(1);
        String X = getName(2);
        String nextPlayer = O;
        String board[] = {"0", "1", "2", "3", "4", "5", "6", "7", "8"};
        int move;
        while (gameResult(board).equals("not completed")) {
            move = getMove(board, nextPlayer);
            if (nextPlayer == O) {
                board[move] = "O";
                nextPlayer = X;
            }
            else {
                board[move] = "X";
                nextPlayer = O;
            }
        }
        conclusion(gameResult(board), O, X);
    }
}

Which, as I think you can probably guess, is meant to replicate the classic pen and paper game TicTacToe digitally. Has anyone got any tips as to how to optimize/improve my code?

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Please note that the following points are in no particular order and I do not claim the list to be exhaustive, because frankly I'm far from perfect and also I won't touch really advanced stuff here. And it's always possible to do something more sophisticated, but I don't think that is required here.
Now that I got that out of the way, let's start with some general stuff:

You declared pretty much everything as static, which:

  • allows you to have everything only once per application
  • allows other classes to manipulate your game and set it to an invalid state

Basically you only use static for things that you really only want once, like constants or utility functions. WINNING_COMBOS would be a good candidate for a private final static variable, while your input scanner certainly is not.
In your case, remove static from all variables and methods except for moveTaken() and printBoard(), create a new TicTacToe() instance in the main method and call a new startGame() method on it which you create and in which you move the previous logic from the main method.


You declared everything as public. The general rule is to keep everything as contained as possible, so future changes are easier, the program is easier to undertand and bugs don't spread too far (the list of reasons is not exhaustive, see EffectiveJava for more details).
In your case, change everything from public to private, only the new startGame() method you might want to set to public. But beware once you do that multiple calls to that method would break your game, so be mindful of that.


Some of your variable and method names need improvement. By convention, getter methods are called getXyz(), methods which return a boolean are usually named isXyz(). Variable names consisting of one character like 0 and X are very poor, call them playerName0 and playerNameX or whatever. Also try not to use magic numbers or strings - create a well named private static final String or private static final int for them and use that instead. Magic numbers are horribly confusing when trying to understand someones (or your own after a while) code.

Now on to some specifics:
You are implementing the game result as some strings. I would not recommend that, I'd use something much less error-prone and more readable, an enum. Same goes for your nextPlayer tracking.


You are keeping track of multiple things during the game, all of which reside in the main method. While this works, as a result you need to pass quite a few arguments to each method you call. You should place some stuff into private class variables, they can be accessed by your no longer static methods no problem.

Final words: Note that I did not touch your game algorithm, I will leave that to others (my wall of text is long enouogh as is..) I also did not comment anything (which you should!) and I also did not care about possible errors (which you should!). The code for the refactored example can be found here.
Again, this is not perfect, but I prefer smaller steps at a time (and I'm hungry now :) )

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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