How can I better test whether the player won in my tic-tac-toe?

Question

I created a tic-tac-toe using java and I would like to know if there are any practices that I can improve and make the code cleaner and more performative

import java.util.Scanner;
public class Main {
    public static boolean test(String[] values){
        //Check diagonals
        if((values[0].equals("X") && values[4].equals("X") && values[8].equals("X")) || (values[0].equals("0") && values[4].equals("0") && values[8].equals("0"))){
            System.out.print(values[0] + " win");
            return true;
        }
        if((values[2].equals("X") && values[4].equals("X") && values[6].equals("X")) || (values[2].equals("0") && values[4].equals("0") && values[6].equals("0"))){
            System.out.print(values[2] + " win");
            return true;
        }
        //Check verticals
        for(int x = 0;x < 9;x+=3){
            if((values[0+x].equals("X") && values[1+x].equals("X") && values[2+x].equals("X")) || (values[0+x].equals("0") && values[1+x].equals("0") && values[2+x].equals("0"))){
                System.out.print(values[0+x] + " win");
                return true;
            }
        }
        //Checks horizontal
        for(int x = 0;x < 3;x++){
            if((values[0+x].equals("X") && values[3+x].equals("X") && values[6+x].equals("X")) || (values[0+x].equals("0") && values[3+x].equals("0") && values[6+x].equals("0"))){
                System.out.print(values[0+x] + " win");
                return true;
            }
        }
        return false;
    }
    public static void main(String[] args) {
        Scanner read = new Scanner(System.in);
        String[] game = {" ", " ", " ", " ", " ", " ", " ", " ", " "};
        String[] players = {"X", "0"};
        for(int x = 0;x < 9;x++){
            System.out.print(players[x%2] + ": ");
            int currentPlay = read.nextInt();
            while(currentPlay < 1 || currentPlay > 9 || game[currentPlay -1] != " "){
                System.out.print("Invalid, play again: ");
                currentPlay = read.nextInt();
            }
            if(x % 2 == 0){
                game[currentPlay -1]  = "X";
            }
            else{
                game[currentPlay -1] = "0";
            }
            System.out.printf("%s | %s | %s\n%s | %s | %s\n%s | %s | %s\n", game[0], game[1], game[2], game[3], game[4], game[5], game[6], game[7], game[8]);
            if(test(game)){
                break;
            }
            if(!test(game) && x == 8){
                System.out.print("Tie");
            }
        }
    }
}

Answer 1

..are any practices that I can improve and make the code cleaner..

There are!

Don't over-enigeer

While you can create all fancy strategies to check tic-tac-too rules; I think it is best to keep it as simple as possible. So checking rows, columns and diagonals hard-coded is, imho, not too bad here.

Don't use String for state

You should never use a String to encode state. Better to use an enum

For example:

enum State { X, O, EMPTY }

State[] board 

Then you can use == instead of the verbose equals(), increasing readability.

 if (board[3] == State.X)

Consider using a 2D array

You could encode the board as a 2d array, for readability as well. This won't improve your big if-statement.

Consider extracting the 'player'

   public static boolean test(State[] board, State p){
        //Check diagonals
        if((board[0] == p && board[4] == p  && board[8] == p) || {
            System.out.print(p + " win");
            return true;
        }
...

   test(board, State.X);
   test(board, State.O);

Split code to methods with one-single-responsibility

test is responsible for checking the entire board. You should not print there; the printing is another responsibility.

So the test should get rid of all the System.out's

   public static boolean test(State[] board, State p){
        //Check diagonals
        if((board[0] == p && board[4] == p  && board[8] == p) || {
            return true;
        }

You could do this, elsewhere:

 if (test(board, State.X)) {
    System.out.println(State.X + " won!");
 }

Don't do too much

A player that didn't move, cannot win. So you should only have to check one player each time, instead of both players. You keep track of the current player, so you should only check that player after it made it's move.

Answer 2

I hate this sort of hard-coded testing - there is usually a better approach to represent our data.

A few thoughts come to mind.

1. You don't stop players playing into an already occupied cell.
2. Why not use a 2-d array (actually an array of arrays)? Then you can more naturally scan rows, columns, and even diagonals.
3. If you score X as 1 and O as -1, then a winning row, column or diagonal adds up to 3 (X wins) or -3 (O wins).
4. If you initialise the cells to an appropriate value, I think you can also detect a hung game early - for example if you initialise to 4, then any row, column or diagonal with a sum < 4 but not 3 or -3 is unwinnable. If they're all unwinnable, there's no point making further plays. (I need to think this through a bit more!)
5. I thought it through more carefully. If X is 1, O is -1 and an empty cell is 4 (for example), then a line summing to 3 is a win for X; summing to -3 is a win for O; 4 is an unwinnable line as it must have an X, an O and a blank cell; other values are undecided lines. If there are 8 unwinnable lines, then the match is a draw.]

Answer 3

Thanks for sharing your code.

Your code is a procedural approach to the problem.

There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.

But OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")

The most obvious problem in your code is the code duplication. You have lots of "sections" that do the same. In a first step you could extract the duplicated code into parameterized methods sticking to the procedural approach like this:

 public class Main {
    private static boolean isOwningThreeInLine(
                                 String player
                               , String[] board
                               , int[] positions){
      boolean isOwningLine = true;
      for(int position : positions)
          isOwningLine =  isOwningLine && 
                          player.equals(board[position]);
      return isOwningLine;
    }
    public static boolean test(String[] values){
        //Check diagonals
        if(isOwningThreeInLine("X",values, {0,4,8})
           || (isOwningThreeInLine("0",values, {0,4,8})){
            System.out.print(values[0] + " win");
            return true;
        }
        if(isOwningThreeInLine("X",values, {2,4,6})
           || (isOwningThreeInLine("0",values, {2,4,6})){
            System.out.print(values[2] + " win");
            return true;
        }

This shows, that the converted code still is duplicated with only the definition of the line changing. So we could collect the line definitions in a collection and loop over them like this:

 public class Main {
    private static final Collection<int[]> lineDefinitions = Arrays.asList(//
                   new int[]{0,4,8},
                   new int[]{2,4,6} /* more of that for the rows and columns*/)
    private boolean isOwningThreeInLine(String player, String[] board, int[] positions){
      boolean isOwningLine = true;
      for(int position : positions)
          isOwningLine && player.equals(board[position]);
      return isOwningLine;
    }
    public static boolean test(String[] values){
       for(int[] line : lines ) {
        if(isOwningThreeInLine("X",values, line)
           || (isOwningThreeInLine("0",values, line)){
            System.out.print(values[line[0]] + " win");
            return true;
        }
       }
      

And since lines now are objects in a collection this is your first object oriented solution despite we didn't even created a separate class for the line reperesentation.

The benefit is, that you no longer have to distinguish between "strait lines" and "diagonals", they both are nothing else that entries in the lines collection.

Of cause you could the same for the player:

 public class Main {
    private static final Collection<String> players = Arrays.asList("X", "0");
    private static final Collection<int[]> lineDefinitions = Arrays.asList(//
                   new int[]{0,4,8},
                   new int[]{2,4,6} /* more of that for the rows and columns*/)
    private boolean isOwningThreeInLine(String player, String[] board, int[] positions){
      boolean isOwningLine = true;
      for(int position : positions)
          isOwningLine && player.equals(board[position]);
      return isOwningLine;
    }
    public static boolean test(String[] values){
        for(String player : players) {
           for(int[] line : lines ) {
            if(isOwningThreeInLine(player,values, line)){
                System.out.print(player + " win");
                return true;
            }
           }
         }
     }