Java Tic-Tac-Toe game (implemented through MVC)

Question

After seeing Tic Tac Toe in MVC, I attempted to create something similar. My program uses the Swing library and it's relatively simple. When I run the game, it's just a 3x3 grid with no extra features.

Can anyone give me tips on how to improve my overall MVC design / code? I did a lot of reading, but I sure I don't fully understand the concept yet. My view.java file is extremely lengthy.

View.java

import java.awt.*;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JButton;

class View extends JFrame {

    GridLayout grid = new GridLayout(3, 3);
    JButton[] buttons;
    Model model;
    boolean gameEnd;

    public View(Model model) {
        super("tic-tac-toe");
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        addComponentsToPane(getContentPane());
        pack();
        setVisible(true);
        this.model = model;
        gameEnd = false;
    }

    public void addComponentsToPane(final Container pane) {
        final JPanel panel = new JPanel();
        panel.setLayout(grid);        
        panel.setPreferredSize(new Dimension(300, 300));

        buttons = new JButton[9];

        for (int i = 0; i < buttons.length; i++) {
            buttons[i] = new JButton();
            buttons[i].getPreferredSize();
            panel.add(buttons[i]);
        }

        pane.add(panel);
    }

    public boolean checkBoardForWin() {
        int x_sum_first_row, x_sum_second_row, x_sum_third_row,
            x_sum_first_column, x_sum_second_column, x_sum_third_column,
            x_sumDiagonalLR, x_sumDiagonalRL;

        x_sum_first_row = x_sum_second_row = x_sum_third_row
        = x_sum_first_column = x_sum_second_column = x_sum_third_column
        = x_sumDiagonalLR = x_sumDiagonalRL = 0;

        int o_sum_first_row, o_sum_second_row, o_sum_third_row,
            o_sum_first_column, o_sum_second_column, o_sum_third_column,
            o_sumDiagonalLR, o_sumDiagonalRL;

        o_sum_first_row = o_sum_second_row = o_sum_third_row
        = o_sum_first_column = o_sum_second_column = o_sum_third_column
        = o_sumDiagonalLR = o_sumDiagonalRL = 0;

        for (int i = 0; i < 3; i++) {
            if (buttons[i].getText() == "X")
                x_sum_first_row++;
            if (buttons[i].getText() == "O")
                o_sum_first_row++;
        }

        for (int i = 3; i < 6; i++) {
            if (buttons[i].getText() == "X")
                x_sum_second_row++;
            if (buttons[i].getText() == "O")
                o_sum_second_row++;
        }

        for (int i = 6; i < 9; i++) {
            if (buttons[i].getText() == "X")
                x_sum_third_row++;
            if (buttons[i].getText() == "O")
                o_sum_third_row++;
        }

        for (int i = 0; i < 9; i += 3) {
            if (buttons[i].getText() == "X")
                x_sum_first_column++;
            if (buttons[i].getText() == "O")
                o_sum_first_column++;
        }

        for (int i = 1; i < 9; i += 3) {
            if (buttons[i].getText() == "X")
                x_sum_second_column++;
            if (buttons[i].getText() == "O")
                o_sum_second_column++;
        }

        for (int i = 2; i < 9; i += 3) {
            if (buttons[i].getText() == "X")
                x_sum_third_column++;
            if (buttons[i].getText() == "O")
                o_sum_third_column++;
        }

        for (int i = 0; i < 9; i += 4) {
            if (buttons[i].getText() == "X")
                x_sumDiagonalLR++;
            if (buttons[i].getText() == "O")
                o_sumDiagonalLR++;
        }

        for (int i = 2; i < 7; i += 2) {
            if (buttons[i].getText() == "X")
                x_sumDiagonalRL++;
            if (buttons[i].getText() == "O")
                o_sumDiagonalRL++;
        }

        boolean someoneWon = false;

        if (x_sum_first_row == 3 || x_sum_second_row == 3 || x_sum_third_row == 3 ||
            x_sum_first_column == 3 || x_sum_second_column == 3 || x_sum_third_column == 3 ||
            x_sumDiagonalLR == 3 || x_sumDiagonalRL == 3 ||
            o_sum_first_row == 3 || o_sum_second_row == 3 || o_sum_third_row == 3 ||
            o_sum_first_column == 3 || o_sum_second_column == 3 || o_sum_third_column == 3 ||
            o_sumDiagonalLR == 3 || o_sumDiagonalRL == 3)
            someoneWon = true;

        return someoneWon;
    }

    public void updateGameState() { 
        if (model.movesCounter == 9 || checkBoardForWin() == true)
            gameEnd = true;
    }

    public void endGame() {
        for (int i = 0; i < buttons.length; i++)
            buttons[i].setEnabled(false);
    }

Model.java

public class Model {

    public int movesCounter;
    public String userSymbol;

    public Model() {
        movesCounter = 0;
    }

    public void setChoice() {
        if (movesCounter % 2 == 0 )
            userSymbol = "X";
        else
            userSymbol = "O";
    }

    public void incrementMovesCounter() { movesCounter++; }

}

Controller.java

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JButton;

class Controller implements ActionListener {

    View view;
    Model model;

    public Controller(Model model, View view) {
        this.model = model;
        this.view = view;
        addActionListeners();
    }

    private void addActionListeners() {
        for (int i = 0; i < view.buttons.length; i++)
            view.buttons[i].addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if (view.gameEnd == false) {
            model.setChoice();
            ((JButton) e.getSource()).setText(model.userSymbol);
            model.incrementMovesCounter();
        }
    }

}

TicTacToe.java

public class TicTacToe {

    public static void main(String[] args) {
        Model model = new Model();
        View view = new View(model);
        Controller controller = new Controller(model, view);

        boolean run = true;
        while (run) {
            view.updateGameState();
            if (view.gameEnd == true) {
                view.endGame();
                run = false;
            }
            System.out.println(view.gameEnd);
            try {
                Thread.sleep(500);
            } catch(InterruptedException ex) {
                Thread.currentThread().interrupt();
            }
        }
    }

}

Answer 1

MVC and your structure

Can anyone give me tips on how to improve my overall MVC design / code? I did a lot of reading, but I sure I don't fully understand the concept yet. Any help is appreciated.

MVC is a bit of a confusing concept, as different people use the term differently. Just looking at the different images at different wikipedia pages shows that MVC can be implemented in different ways.

I don't think that you need to worry about implementing it 100% correct for now, but you should try to follow the basic idea.

So basically, you have three components:

• The Model: Holds the data and may contain business logic
• The View: Displays a representation of the data
• The Controller: Processes user input and applies it to the view and/or model

You then somehow have to string these components together, which is where the approaches to MVC differ a bit.

My view.java file is extremely lengthy..

That is because it doesn't just display the data, it actually contains program logic, which it shouldn't. It also holds the data (the current state of the game), which it also should not do.

Your Model

First of all, you need to document your code with JavaDoc comments for functions and public fields.

It's not really clear what movesCounter does, or what choice setChoice sets (especially without looking at the implementation).

Your model also has a very generic name. What I would have excepted is something like this:

/**
 * One field of the game grid.
 */
class Field {
    private Symbol owner;

    // getters, setters, constructor
}

/**
 * A represenation of an owner of a field.
 */
enum Symbol {
    X, O, NONE
}

class Game {
    private Field[][] gameGrid;

    /** 
     * sets the owner of the field determined by x and y.
     *
     * @param  owner  the new owner of the field
     * @param  x x coordinate
     * @throws IllegalMoveException in case the owner cannot be set
     */
    public setFieldOwner(Symbol owner, int x, int y) throws IllegalMoveException {
        // ...
    } 

    public boolean isGameOver()
    public Symbol getWinner()
    ...
}

Now you have a game field, which contains the current state of the game, as well as the logic of the game.

There is room for improvement here, but it should be good enough to show you the general idea.

Your View

As I said above, your view does too much. It should only show the current state of your model, nothing else.

Generally, it might implement an interface such as this:

interface View {

    // when this method is called, you would change the view of the corresponding element
    public setFieldOwner(Symbol owner, int x, int y);

    // here, you could show a message, and disable buttons, show start new game button, etc
    public informWin(Symbol winner);

    public informStart(Symbol winner);
}

Your Controller

Your controller is fine. If you use a similar approach to the one outlined above, you would need to change it a bit, but generally, it would still perform the same task as now (get input, apply it to model and view).

You should probably add a more generic InputInterface instead of using an ActionListener directly to separate the controller from the concrete GUI. It would probably only have one method: getMove, which should return the desired move in some way (either create a move class, or just use an array).

Misc

• fields should be private to encapsulate the concrete implementation.
• variable names should be camelCase, not snake_case.
• always use curly brackets even for one line statements to avoid future bugs and to increase readability.
• the name setX generally implies a setter, which would accept an argument and apply it to the object. It's a bit confusing that you use it differently.

Answer 2

This is my understanding of MVC.

The Model should handle all the logic and it knows the state of the board, such as where the pieces are, and the state for a Win.

The view only displays things like the board, menu and gamestate like winner.

The controller handles the communication between the user, view and model. For instance, it updates the model based on user input, and updates the view based on the model.

Someone stated the model "may have some business logic" and to my understanding, this is not really the case. It is supposed to be where most all of the business logic is, and it is in the exceptions that some biz logic shows up else where, not the other way around.

This means the checkBoardForWin should be in the model

Answer 3

I don't have the time at the moment for a full answer but...

In View.checkBoardForWin(), I would split this out into other methods. Store each win combination (e.g., top row, middle row, bottom row etc) in a LinkedList of buttons. For each element in each LinkedList, if X, increment an integer, if Y, decrement.

If variable =3, X wins, if var =-3, O wins. Have each method only check one LinkedList of potential winning combinations, and pass the LinkedList as a parameter.

I'd also abstract the game logic from the GUI, which would help reduce the number of lines per class, but would increase the number of classes. Advantage here is that you can plug your game into another GUI whenever appropriate.