Xs and Os game logic in Java [closed]

Question

I am practicing programing in Java. I have started to make a Xs and Os (Tic-Tac-Toe) game and I began with baseline logic of the game. I would appreciate any advice on my programming style, what am I doing wrong and what is good about my code.

Combinations class

public class Combinations
{
    public static Map<Field, Mark> firstRow;
    public static Map<Field, Mark> secondRow;
    public static Map<Field, Mark> thirdRow;
    
    public static Map<Field, Mark> firstColumn;
    public static Map<Field, Mark> secondColumn;
    public static Map<Field, Mark> thirdColumn;
    
    public static Map<Field, Mark> backDiagonal;
    public static Map<Field, Mark> forwardDiagonal;
    
    private static List<Map<Field, Mark>> combinations = Arrays.asList(firstRow, secondRow, thirdRow, 
                                                                firstColumn, secondColumn, thirdColumn,
                                                                backDiagonal, forwardDiagonal);
    
    private static Map<Field, Mark> winingCombination = null;
    
    public Combinations(Board board)
    {
        firstRow = Map.of(Field.A1, board.A1, 
                          Field.B1, board.B1, 
                          Field.C1, board.C1);
        
        secondRow = Map.of(Field.A2, board.A2, 
                           Field.B2, board.B2, 
                           Field.C2, board.C2);
        
        thirdRow = Map.of(Field.A3, board.A3, 
                          Field.B3, board.B3, 
                          Field.A2, board.C3);
        
        firstColumn = Map.of(Field.A1, board.A1, 
                             Field.A2, board.A2, 
                             Field.A3, board.A3);
        
        secondColumn = Map.of(Field.B1, board.B1, 
                              Field.B2, board.B2, 
                              Field.B3, board.B3);
        
        thirdColumn = Map.of(Field.C1, board.C1, 
                             Field.C2, board.C2, 
                             Field.C3, board.C3);
        
        backDiagonal = Map.of(Field.A1, board.A1, 
                              Field.B2, board.B2, 
                              Field.C3, board.C3);
        
        forwardDiagonal = Map.of(Field.A3, board.A3, 
                                 Field.B2, board.B2, 
                                 Field.C1, board.C1);
    }
    
    public static boolean isFilled(Map<Field, Mark> combination)
    {
        for(Entry<Field, Mark> field : combination.entrySet())
        {
            if(!Mark.isEmpty(field.getValue()))
                return false;
        }
        
        return true;
    }
    
    public static boolean isWiner(Map<Field, Mark> combination, Mark playerMark)
    {
        for(Entry<Field, Mark> field : combination.entrySet())
        {
            if(field.getValue() != playerMark)
                return false;
        }
        
        return true;
    }
    
    public static boolean hasWiningCombination()
    {
        return winingCombination != null;
    }
    
    public Map<Field, Mark> getWiningCombination(Mark playerMark)
    {
        for(Map<Field, Mark> combination : combinations)
        {
            if(isWiner(combination, playerMark))
            {
                winingCombination = combination;
            }
        }
        
        return winingCombination;
    }
}

Field enum class for fields on the board

public enum Field
{
    A1, A2, A3,
    B1, B2, B3,
    C1, C2, C3;
    
    public boolean isCorner(Field field)
    {
        switch(field)
        {
            case A1:
            case A3:
            case C1:
            case C3:
                return true;
                
            default:
                return false;
        }
    }
    
    
}

Mark enum class for player marks on fields

public enum Mark
{
    EMPTY,
    X,
    O;
    
    public static boolean isEmpty(Mark field)
    {
        switch(field)
        {
            case EMPTY:
                return true;
            
            default:
                return false;
        }
    }
    
    public static Mark oposite(Mark mark)
    {
        switch(mark)
        {
            case X:
                return O;

            case O:
                return X;
                
            default:
                return EMPTY;
        }
    }
}

Answer 1

I would like to mention couple of points.

1. You don't need to use switch cases in enum. It make code over complicated. You can rather use EnumSet and normal if-else condition for it. (updated code added below)

2. Correct the spelling of opposite (form oposite). (updated code added below)

3. Static fields should be initialized in static block, rather than in constructor.

4. Better to initialize all static fields all together. Rather than initializing few in constructor and few at class level directly. Consider initializing combinations with other fields. Because when you will reset other fields with new object initialization, this field won't get initialized (or update).

5. Rather than making class level fields public, make it accessible through the public method, and make these class level fields private.

6. Rather than making all methods as static, make use of Object Oriented Paradigm. So that if you want to give multiple implementation of class, you can inherit class and override the implementation. And different behavior can be achieved.

7. If all fields and methods are static, there is not use of making constructor and no need to class initialization. Rather mark class final and private constructor. This is prevent object initialization, which is not need in case of all static fields/methods of class. And provide static method to reset fields. (Not valid if you convert methods to non-static as per point 6)

8. Rename class to make it self-explanatory. Combination name doesn't clear what it actually does.

9. I believe you are using Java 8 or above. Consider using Streams to iterate or Map/List.

---

public enum Field {
  A1, A2, A3, 
  B1, B2, B3, 
  C1, C2, C3;

  private static EnumSet<Field> corners;

  static {
    corners = EnumSet.of(A1, A3, C1, C3);
  }

  public boolean isCorner(final Field field) {
    return corners.contains(field);
  }
}

---

public enum Mark {
  EMPTY, O, X;

  public static boolean isEmpty(final Mark field) {

    return field == EMPTY;
  }

  public static Mark oppsite(final Mark mark) {

    if (mark == O) {
      return X;
    } else if (mark == X) {
      return O;
    }

    return EMPTY;
  }
}

---

If you can share Board class, probably we can help you better.

Answer 2

Mix of static vs. non-static

The biggest flaw, in my opinion, is the mix of static vs. non-static in the class. With your current implementation, if you create more than one instance of the class Combinations at the same time, the static maps will become unstable; since they will be shared across all instances and recreated in EACH new instance of the class.

Make all the code static, with no constructor (using a Static Initialization Blocks), as stated by @notescrew

Based on the current implementation, in my opinion, this class can be converted to a static utils class that check if there's a winner in the board.

Exposition of mutable items

Another flaw is that you expose your mutable collections to the outside of the class. This is a bad habit, in my opinion, since you can edit the state from any other class, you lose the control of the data inside the collection. To prevent that, hide the collections with the private keyword and ALWAYS return, either a new instance of the collection or an unmodifiable implementation in the getters or methods that return it (Combinations#getWiningCombination, ect).