MVC Java console tic-tac-toe

Question

public class Model extends TicTacToe
{

    public static boolean hasWon( int [][] matrix )
    {
        boolean retVal = false;

        //Check for horizontal win
        for( int row = 0; row < matrix.length; row++ ){
            int sum = 0;
            for( int col = 0; col < matrix[0].length; col++ ){
                sum += matrix[row][col];
            }
            if( sum == 5 ){
                System.out.println("X wins.");
                retVal = true;
            } else if ( sum == -5 ) {
                System.out.println("O wins.");
                retVal = true;
            }
        }

        //Check for vertical win
        for( int col = 0; col < matrix[0].length; col++ ){
            int sum = 0;
            for( int row = 0; row < matrix.length; row++ ){
                sum += matrix[row][col];
            }
            if( sum == 5 ){
                System.out.println("X wins.");
                retVal = true;
            } else if ( sum == -5 ) {
                System.out.println("O wins.");
                retVal = true;
            }
        }

        if( (matrix[0][0] + matrix[1][1] + matrix[2][2]+matrix[3][3]+matrix[4][4]) == 5 ){
            System.out.println("X wins.");
            retVal = true;
        } else if ( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == -5 ) {
            System.out.println("O wins.");
            retVal = true;
        }
        if( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == 5){
            System.out.println("X wins.");
            retVal = true;
        } else if ( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == -5 ) {
            System.out.println("O wins.");
            retVal = true;
        }

        //Check for cat game
        boolean foundSpace = false;
        for( int row = 0; row < matrix.length; row++ ){
            for( int col = 0; col < matrix[0].length; col++ ){
                if( matrix[row][col] == 0 ) 
                    foundSpace = true;
            }
        }
        if( foundSpace == false ){
            System.out.println("Ends in tie.");
            retVal = true;
        }
        return retVal;
    }
}

public class View extends TicTacToe
{   public static void printBoard( int [][] matrix ){

        for( int row = 0; row < matrix.length; row++ ){
            for( int col = 0; col < matrix[row].length; col++ ){
                if( matrix[row][col] == X )
                    System.out.print("X ");
                else if(matrix[row][col] == O )
                    System.out.print("O ");
                else 
                    System.out.print(". ");
            }
            System.out.println("");
        }
    }
}

import java.util.Scanner;
public class Controller  extends TicTacToe
{   
    Model model;
    View view;

    public Controller (Model model, View view) {
        this.model= model;
        this.view= view;
    }
    public static void main (String [] args) 
    {           
        Model model= new Model();
        View view= new View();
        Scanner input = new Scanner(System.in);

        int [][] board = new int[5][5];

        while( model.hasWon(board) == false){

            //Get the X player input and make the change if not taken.
            System.out.print("X, enter row: ");
            int row = input.nextInt();
            System.out.print("X, enter column: ");
            int col = input.nextInt();
            if(col<=4 && col>= 0) 
            {
                if( board[row][col] == 0 )
                    board[row][col] = X;
                view.printBoard(board);
            }
            else 
                System.out.println("Invalid Input");

            //Check to see if X's move won the game. If so, break out of game loop
            if( model.hasWon(board) == true )
                break;

            //Get the O player input and make the change if not taken.
            System.out.print("O, enter row: ");
            row = input.nextInt();
            System.out.print("O, enter column: ");
            col = input.nextInt();
            if(col<=4 && col>= 0) 
            {
                if( board[row][col] == 0 )
                    board[row][col] = O;                 
                view.printBoard(board);              
            }
            else 
                System.out.println("Invalid Input");

        }
        System.out.println("Game over.");
    }   
}

public class TicTacToe 

{
    static final int X =1;
    static final int O = -1;
}

This is what I have so far to make a Java console MVC-based tic-tac-toe can anyone help me to see if I am right? Any help appreciated.

Answer 1

welcome to code review and thanks for sharing your code.

You code reveled some misconceptions about the MVC-pattern:

Model

The Model is merely a data store. It provides Data Transfer Objects (DTOs), ValueObjects(VOS), Beans or Entities or alike. Any logic in the model is about persistence, integrity and infrastructure to inform the view about changes. In particular the model does not contain business logic.

Your model contains business logic (the game end check).

Controller

the controller manipulates the model. It applies the business logic to the data stored in the model. In particular it has no knowledge about the view.

In your code the controller interacts with the user by printing lines at the console itself.

View

The view displays the models current state and passes the user input to the appropriate method in the controller. It may update data in the model directly, but it should not. The view should delegate changes to the models state to the controller.

In fact User Interface would be a better name instead of View but then we wouldn't have a 3 letter acronym to reflect the 3 layer architecture...

Your view does not handle user input, it only displays the game field.

Answer 2

I agree with @Timothy Truckle in that you have some issues with the MVC-pattern. I disagree with where they've drawn the line between the model, view and controller.

As they've stated, the model should be a data store, containing the state of the application. In addition, the model may have logic about persistence, integrity and infrastructure; I don't disagree with that. But the model should also have logic which can be used to both update the model and query the state of the model; checking to see if the game is over (a win by either player or a tie game) is a state-query.

Amplifying this a bit. If you were to update your program so that it runs as a GUI, with buttons to click to make moves, the model should not need to be changed. The code to check for a win or a tie doesn't need to change; it would be the same whether the game is uses console input/output or has buttons and windows. It is merely a query of the game state, and certainly is allowed in the model.

Reporting the result of that query to user is the violation.

---

You are mixing symbolic identifiers and integer values. Consider:

static final int X = 1;
static final int O = -1;

Using an enum would be better, but using names for the values is an excellent step in the right direction. But consider:

            if( board[row][col] == 0 )
                board[row][col] = X;

If the board contains a zero, you store the symbol X. Wait. What is that zero? What does it mean? Perhaps you want to create and use another symbol:

static final int EMPTY = 0;

---

You are repeating yourself.

In main(), you have a while( model.hasWon(board) == false), and midway through the function, you have the same test if( model.hasWon(board) == false) break;. Before and after the if statement, you have almost exactly the same lines of code. You could move those lines of code into their own function, with parameters to identify which player's turn is being played. Eg)

    while( model.hasWon(board) == false) {

        getAndPlayMove(board, "X", X);

        if (model.hasWon(board) == false)
            break;

        getAndPlayMove(board, "O", O);
    }

Again, that while loop is doing the same operation twice, with the loop condition tested in the middle. It would be better to do twice the number of iterations through the loop, and swap which player is playing each time through. For example:

    int player = X;  // Starting player

    while (model.hasWon(board) == false) {

        String player_name = (player == X) ? "X" : "O";

        getAndPlayMove(board, player_name, player);

        player = (player == X) ? O : X;   // Switch players
    }

With a two player game, repeating the same code for each player may not seem like much of a burden, but for a multi-player game, it quickly becomes clear the correct thing to do is write the code once and change the player each pass through the loop.

---

hasWon() is a very misleading method name. In a tie-game, nobody has won, but the function still returns true.

isOver() would be a better name; you play the game while it is not over.

---

Hard coded numbers are BAD™. Using a hard-coded number once is almost acceptable, but the second time you use that number in the same context is an error waiting to happen. What if you want to change the number? Go from a 5x5 game grid to a 6x6 game grid. Find and replace every instance of a "5" with a "6"? If I had a nickel for every time find-and-replace replaced the wrong thing ...

public static final int SIZE = 5;
int [][] board = new int[SIZE][SIZE];

Good. Now we won't end up with a 5x6 grid. Change all of the if (sum == 5) to if (sum == SIZE) and all of the if (sum == -5) to if (sum == -SIZE) and we're golden, right? No more 5's!

Except when we play, we can't enter the cell 5,5, because you have if(col<=4 && col>=0). Oh! That wasn't a 4, that was a 5-1 optimized by hand to read 4. I guess we want...

if (col <= SIZE-1  &&  col >= 0)

But col <= SIZE-1 is ugly, and takes 3 too many characters. Use the < operator instead:

if (col < SIZE  &&  col >= 0)

BUG: You don't validate the row input! If the user enters 105 and 3, the program accepts it as valid input, but will crash with an IndexOutOfRangeException.

---

The game now works for a 6x6 grid with a row or column of 6 in-a-row. The diagonals fail, though.

    if( (matrix[0][0] + matrix[1][1] + matrix[2][2]+matrix[3][3]+matrix[4][4]) == SIZE ){
        System.out.println("X wins.");
        retVal = true;
    } else if ( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == -SIZE ) {
        System.out.println("O wins.");
        retVal = true;
    }
    if( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == SIZE){
        System.out.println("X wins.");
        retVal = true;
    } else if ( (matrix[0][4] + matrix[1][3] + matrix[2][2]+matrix[3][1]+matrix[4][0]) == -SIZE ) {
        System.out.println("O wins.");
        retVal = true;
    }

Yuk! Lots of hard-coded matrix indices.

You want need to use a loop here:

int diag1 = 0, diag2 = 0;
for (int i=0; i<SIZE; i++) {
    diag1 += matrix[i][i];
    diag2 += matrix[i][SIZE-1-i];
}

if (diag1 == SIZE || diag2 == SIZE) {
    System.out.println("X wins.");
    retval = true;
} else if (diag1 == -SIZE || diag2 == -SIZE) {
    System.out.println("O wins.");
    retval = true;
}

Shorter code, and it works with any SIZE game grid. Of course, you still want to refactor things to remove the output statements from the model.

Bug: It is possible to print multiple outcomes from the game. Imagine playing 24 moves with neither X nor O completing a row column or diagonal ...

X O X O X
O X X X O
O O . O O
O X X X O
X O X O X

Now play X's last and only move -- the centre cell. With your original code, you'd get this output:

X wins.
X wins.
X wins.
Ends in a tie.

---

Your model doesn't store anything. It has no data, and exactly 1 static method. To use your model, you call model.hasWon(board) ... so you are passing the state of the game into the model! If your model actually contained the state, you could simply call model.hasWon().

A better model might be:

public enum Player { X, O };

public class Model {
    public  final int        size;
    private final Player[][] board;
    private       int        moves_left;

    public Model(int size) {
        this.size = size;
        board = new Player[size][size];
        moves_left = size * size;
    }

    public boolean valid_cell(int row, int col) {
        return row >= 0  &&  row < size  &&  col >= 0  &&  col < size;
    }

    public boolean valid_move(int row, int col) {
        if (!valid_cell(row, col)  ||  moves_left == 0)
            return false;

        return board[row][col] == null;
    }

    public Player get_cell(int row, int col) {
        return board[row][col];
    }

    public boolean make_move(int row, int col, Player player) {
        if (!valid_move(row, col))
            throw new IllegalStateException("You didn't validate the player's move!");

        board[row][col] = player;
        spaces_left--;

        boolean won = ...
        /* Add code to see if player made a winning move */

        if (won)
            moves_left = 0;

        return won;
    }

    public int moves_left() {
        return moves_left;
    }
}

Both board and moves_left are private, to prevent tampering with the game state. The get_cell() method allows a read-only access to the board, so the board can be displayed. moves_left is used to keep track of the available spaces, without needing to search over the entire board for an empty spot.

make_move() is used to change the board's state. After ensuring the move is valid, it fills in the player's move, and decrements the moves_left counter. Then, it checks if the move has caused the player to win, and returns that to the caller.

By checking if the current player has won, you eliminate the need to check dual conditions (eg, if (sum == 5) or if (sum == -5)). You can additionally optimize the check for win by only checking the row and column the player moved in, instead of every row and every column in the board. (This optimization would be important if you try to build an AI which has to do an exhaustive search of possible moves.)

To use this model, you might write code along the lines of:

Model model = new Model(5);
Player player = Player.X;

while (model.moves_left() > 0) {

    // Get row, col from player, ensuring model.valid_move(row, col)
    // returns `true` before continuing.

    if (model.make_move(row, col, player)) {
       // Record or report the win
    }

    // Set player to the other player
}

System.out.println("Game over");