3
\$\begingroup\$

Here is my previous question:

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

I'm reposting again because I made a lot of changes to the code. I wasn't able to implement everything as I don't know some of the stuff, but I tried to do as much as possible.

Summary of changes:

  • View doesn't have as much logic now.
  • Model has its own 'logic' for containing the game status separate from the GUI.
  • Added comments.

Is my MVC better now? What can I do to improve my design, code, and/or style?

View.java

import java.awt.GridLayout;
import java.awt.Container;
import java.awt.Dimension;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JButton;
import javax.swing.JOptionPane;

class View extends JFrame {
    GridLayout grid = new GridLayout(3, 3);     // default grid-size for tic-tac-toe
    JButton[] buttons;                          // an array of the buttons (9 of them)    

    /**
     * Overloaded constructor.
     */
    public View() {
        super("tic-tac-toe");
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        addComponentsToPane(getContentPane());
        pack();
        setVisible(true);

        getRootPane().setDefaultButton(buttons[4]);
        buttons[4].requestFocus();
    }

    /**
     * Adds the panel along with its buttons to the pane.
     */
    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);
    }

    /**
     * Informs the user who won.
     */
    public void informWin(Symbol userSymbol) {
        for (int i = 0; i < buttons.length; i++) {
            buttons[i].setEnabled(false);
        }
        JOptionPane.showMessageDialog(null, "Player " + userSymbol.toString() + " has won!");
    }

    /**
     * Informs the user of the tie.
     */
    public void informTie() {
        JOptionPane.showMessageDialog(null, "Tie!");
    }
}

Game.java

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

    Field() {
        owner = Symbol.values()[2];
    }

    public Symbol getOwner() {
        return owner;
    }

    public void setOwner(Symbol owner) {
        this.owner = owner;
    }

    @Override
    public String toString() {
        if (owner == Symbol.values()[0]) {
            return "X";
        } else if (owner == Symbol.values()[1]) {
            return "O";
        } else {
            return "NONE";
        }
    }
}

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

/**
 * The logic of the game.
 */
public class Game {
    private Field[][] gameGrid;
    private int turnsCounter; // the number of turns since the start of the game
    private Symbol userSymbol; // the current Symbol of the player
    private boolean didSomeoneWin; // to check if a player won or if it was a tie

    /**
     * Default constructor.
     * 
     * Initializes the gameGrid array with 9 Field objects and the other
     * to their appropriate values.
     */
    Game() {
        gameGrid = new Field[3][3];
        turnsCounter = 0;
        userSymbol = Symbol.values()[2];
        didSomeoneWin = false;
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                gameGrid[i][j] = new Field();
            }
        }
    }

    /**
     * Sets the symbol of a specific field.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public void setFieldOwner(Symbol owner, int x, int y) {
        gameGrid[x][y].setOwner(owner);
    }

    /**
     * Returns the owner of a specific field.
     * Note: I'm using this just for testing.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public Symbol getFieldOwner(int x, int y) {
        return gameGrid[x][y].getOwner();
    }

    /**
     * Prints the field.
     * Note: I'm using this just for testing.
     */
    public void printField() {
        System.out.println("---PRINTING FIELD---");
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                System.out.print(getFieldOwner(i, j) + " ");
            }
            System.out.println();
        }
    }

    /**
     * Evaluates the board to see if the game is over, and then checks if the number of turns is maxed out at 9.
     * If both are not true, return false;
     */
    public boolean isGameOver() {
        int[] scores = evaluateBoard();
        for (int score : scores) {
            if (score == 3 || score == -3) {
                didSomeoneWin = true;
                System.out.println("didSomeoneWin: " + didSomeoneWin);
                return true;
            }
        }

        if (turnsCounter == 9) {
            return true;
        }

        return false;
    }

    /**
     * Evaluates the board, stores the values into an array, and returns it.
     * 
     * @return      an array containing the score of each possible win condition.
     */
    public int[] evaluateBoard() {
        int[] scores = new int[8];

        // evaluate the first row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[0][i].getOwner() == Symbol.values()[0]) {
                scores[0]++;
            }
            if (gameGrid[0][i].getOwner() == Symbol.values()[1]) {
                scores[0]--;
            }
        }

        // evaluate the second row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[1][i].getOwner() == Symbol.values()[0]) {
                scores[1]++;
            }
            if (gameGrid[1][i].getOwner() == Symbol.values()[1]) {
                scores[1]--;
            }
        }

        // evaluate the third row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[2][i].getOwner() == Symbol.values()[0]) {
                scores[2]++;
            }
            if (gameGrid[2][i].getOwner() == Symbol.values()[1]) {
                scores[2]--;
            }
        }

        // evaluate the first column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][0].getOwner() == Symbol.values()[0]) {
                scores[3]++;
            }
            if (gameGrid[i][0].getOwner() == Symbol.values()[1]) {
                scores[3]--;
            }
        }

        // evaluate the second column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][1].getOwner() == Symbol.values()[0]) {
                scores[4]++;
            }
            if (gameGrid[i][1].getOwner() == Symbol.values()[1]) {
                scores[4]--;
            }
        }

        // evaluate the third column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][2].getOwner() == Symbol.values()[0]) {
                scores[5]++;
            }
            if (gameGrid[i][2].getOwner() == Symbol.values()[1]) {
                scores[5]--;
            }
        }

        // evaluate the left-to-right diagonal
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][i].getOwner() == Symbol.values()[0]) {
                scores[6]++;
            }
            if (gameGrid[i][i].getOwner() == Symbol.values()[1]) {
                scores[6]--;
            }
        }

        // evaluate the right-to-left diagonal
        for (int i = 0; i < 3; i++) { // rows
            for (int j = 0; j < 3; j++) { // columns
                if (i + j == 2) {
                    if (gameGrid[i][j].getOwner() == Symbol.values()[0]) {
                        scores[7]++;
                    }
                    if (gameGrid[i][j].getOwner() == Symbol.values()[1]) {
                        scores[7]--;
                    }
                }
            }
        }

        return scores;
    }

    /**
     * Increments the number of turns.
     */
    public void incrementTurnsCounter() {
        turnsCounter++;
    }

    /**
     * Returns the number of turns since the start of the game.
     */
    public int getTurnsCounter() {
        return turnsCounter;
    }

    /**
     * Sets the user symbol to a Symbol depending on whether the number of turns is even or odd.
     */
    public void setUserSymbol() {
        if (turnsCounter % 2 == 1) {
            userSymbol = Symbol.values()[0];
        } else {
            userSymbol = Symbol.values()[1];
        }
    }

    /**
     * Returns the current user symbol.
     */
    public Symbol getUserSymbol() {
        return userSymbol;
    }

    /**
     * Returns true if someone has won the game. Otherwise, false.
     */
    public boolean getDidSomeoneWin() {
        return didSomeoneWin;
    }
}

Controller.java

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

class Controller implements ActionListener {
    private Game game;
    private View view;

    /**
     * Overloaded constructor. Initializes the game and view, and
     * adds the action listeners to the buttons in view.
     *
     * @param   an instance of the Game class.
     * @param   an instance of the View class.
     */
    public Controller(Game game, View view) {
        this.game = game;
        this.view = view;
        addActionListeners();
    }

    /**
     * Adds an action listener to every button.
     */
    private void addActionListeners() {
        for (int i = 0; i < view.buttons.length; i++) {
            view.buttons[i].addActionListener(this);
        }
    }

    /**
     * Increments the number of moves since the start of the game, and
     * sets the user symbol. It then finds out what x and y coordinates that button
     * corresponds to in the Game object.
     * Examples: button[0] would be Field[0][0]. button [1] would be Field[0][1].
     * button[5] would be Field[1][2].
     * 
     * It then sets the owner of the field in the Game object, and modifies the View buttons.
     *
     * @param e     the action performed. In this game, it would be a mouse click.
     */
    @Override
    public void actionPerformed(ActionEvent e) {
        if (game.isGameOver() == false) {
            game.incrementTurnsCounter();
            game.setUserSymbol();
            int index = getMove((JButton) e.getSource());
            int x = 0; // row coordinate
            int y = 0; // column coordinate
            switch (index) {
                case 0: x = 0;
                        y = 0;
                        break;
                case 1: x = 0;
                        y = 1;
                        break;
                case 2: x = 0;
                        y = 2;
                        break;
                case 3: x = 1;
                        y = 0;
                        break;
                case 4: x = 1;;
                        y = 1;
                        break;
                case 5: x = 1;
                        y = 2;
                        break;
                case 6: x = 2;
                        y = 0;
                        break;
                case 7: x = 2;
                        y = 1;
                        break;
                case 8: x = 2;
                        y = 2;
                        break;
                default: break;
            }
            game.setFieldOwner(game.getUserSymbol(), x, y);
            ((JButton) e.getSource()).setText((game.getUserSymbol()).toString());
            ((JButton) e.getSource()).setEnabled(false);
        }
    }

    /**
     * Returns the index of the current JButton.
     * 
     * @param       the button that was clicked.
     */
    public int getMove(JButton button) {
        int index = 0;
        for (int i = 0; i < 9; i++) {
            if (button == view.buttons[i]) {
                index = i;
            }
        }
        return index;
    }

    /**
     * Informs the user of the outcome of the game depending on if someone won or not.
     */
    public void informOutcome() {
        if (game.getDidSomeoneWin()) {
            System.out.println(game.getDidSomeoneWin());
            System.out.println("Informing win.");
            view.informWin(game.getUserSymbol());
        } else {
            System.out.println("Informing tie.");
            game.printField();
            int[] scores = game.evaluateBoard();
            for (int score : scores) {
                System.out.print(score + " " );
            }
            view.informTie();
        }
    }
}

TicTacToe.java

public class TicTacToe {
    public static void main(String[] args) {
        Game game = new Game();
        View view = new View();
        Controller controller = new Controller(game, view);

        while (!game.isGameOver()) {
            try {
                Thread.sleep(250);
            } catch(InterruptedException ex) {
                Thread.currentThread().interrupt();
            }
        }

        controller.informOutcome();
    }    
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I always ended up using a GridBagLayout every awt project I did. Maybe you won't have to use it. I think that your code style is pleasing and that you can improve by also including tests for your methods. And you might want to consider a repaintor an update method for event-driven programming. \$\endgroup\$ – Niklas R. Apr 15 '16 at 18:42
2
\$\begingroup\$

So I have some recommendations:

Visibility

It's a little bit confusing the visibility in your classes, all the classes are in the same package?

If I had to do the game (I never code a game, I have mostly experience with webapps) using MVC I would create 3 packages, besides the one of the root project, so I wouls have somethincom.kimae.tictactoe, com.kimae.tictactoe.view, com.kimae.tictactoe.controller and com.kimae.tictactoe.model, and maybe some other packages like helpers, utils and etc...

Since I have this separation by package I would create all my classes that will be used in the context of my general application public, so I would have:

public class Field{}
public class Game{}

and so on.

Now that we have all my classes public I would change the visibility of the attributes to private (I would make exception to attributes that are final....but i don't recommend).

Enum

I don't know if you have some special motive to use the enum like that, but you could just use the names like instead of using userSymbol = Symbol.values()[2]; you could use userSymbol = Symbol.NONE, and in the toString of the field you could do:

@Override
public String toString() {
    return owner.toString();
}

I would put the ENum inside Field, since it concerns a state of Field, and instead initialize the Symbol.NONE in the constructor

Initialization

I would create static factories Field.getDefault()

Responsabilities

So I really thing the Game class is to big, and have to many responsabilities, it is almost acting as a controller.

I would create a class called board which have the matrix for the game and can change the state of the board. Now with this new class the game class would dictate the rules for the game :).

Show me the code!

Here is a draft of your classes

Field

public class Field {
    private Symbol owner;

    private Field(Symbol sym) {
        owner = sym;
    }

    public static Field getDefault(){
        return new Field(Symbol.NONE);
    }

    public Symbol getOwner() {
        return owner;
    }

    public void setOwner(Symbol owner) {
        this.owner = owner;
    }

    @Override
    public String toString() {
        return owner.toString();
    }

    public static enum Symbol {
        X, O, NONE
    }
}

Game

/**
 * The logic of the game.
 */
public class Game {
    private int turnsCounter; // the number of turns since the start of the game
    private Symbol userSymbol; // the current Symbol of the player
    private boolean didSomeoneWin; // to check if a player won or if it was a tie
    private Board board;

    /**
     * Default constructor.
     * 
     * Initializes the gameGrid array with 9 Field objects and the other
     * to their appropriate values.
     */
    public Game() {
        turnsCounter = 0;
        userSymbol = Symbol.NONE;
        didSomeoneWin = false;
        board = new Board();

    }

    /**
     * Sets the symbol of a specific field.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public void setFieldOwner(Symbol owner, int x, int y) {
        board.setFieldOwner(userSymbol, x, y);
    }

    /**
     * Returns the owner of a specific field.
     * Note: I'm using this just for testing.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public Symbol getFieldOwner(int x, int y) {
        return board.getFieldOwner(x,y);
    }

    /**
     * Prints the field.
     * Note: I'm using this just for testing.
     */
    public void printField() {
        System.out.println("---PRINTING FIELD---");
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                System.out.print(getFieldOwner(i, j) + " ");
            }
            System.out.println();
        }
    }

    /**
     * Evaluates the board to see if the game is over, and then checks if the number of turns is maxed out at 9.
     * If both are not true, return false;
     */
    public boolean isGameOver() {
        int[] scores = board.evaluateBoard();
        for (int score : scores) {
            if (score == 3 || score == -3) {
                didSomeoneWin = true;
                System.out.println("didSomeoneWin: " + didSomeoneWin);
                return true;
            }
        }

        if (turnsCounter == 9) {
            return true;
        }

        return false;
    }

    /**
     * Increments the number of turns.
     */
    public void incrementTurnsCounter() {
        turnsCounter++;
    }

    /**
     * Returns the number of turns since the start of the game.
     */
    public int getTurnsCounter() {
        return turnsCounter;
    }

    /**
     * Sets the user symbol to a Symbol depending on whether the number of turns is even or odd.
     */
    public void setUserSymbol() {
        if (turnsCounter % 2 == 1) {
            userSymbol = Symbol.O;
        } else {
            userSymbol = Symbol.X;
        }
    }

    /**
     * Returns the current user symbol.
     */
    public Symbol getUserSymbol() {
        return userSymbol;
    }

    /**
     * Returns true if someone has won the game. Otherwise, false.
     */
    public boolean getDidSomeoneWin() {
        return didSomeoneWin;
    }
}

Controller

public class Controller implements ActionListener {
    private Game game;
    private View view;

    /**
     * Overloaded constructor. Initializes the game and view, and
     * adds the action listeners to the buttons in view.
     *
     * @param   an instance of the Game class.
     * @param   an instance of the View class.
     */
    public Controller(Game game, View view) {
        this.game = game;
        this.view = view;
        addActionListeners();
    }

    /**
     * Adds an action listener to every button.
     */
    private void addActionListeners() {
        for (int i = 0; i < view.buttons.length; i++) {
            view.buttons[i].addActionListener(this);
        }
    }

    /**
     * Increments the number of moves since the start of the game, and
     * sets the user symbol. It then finds out what x and y coordinates that button
     * corresponds to in the Game object.
     * Examples: button[0] would be Field[0][0]. button [1] would be Field[0][1].
     * button[5] would be Field[1][2].
     * 
     * It then sets the owner of the field in the Game object, and modifies the View buttons.
     *
     * @param e     the action performed. In this game, it would be a mouse click.
     */
    @Override
    public void actionPerformed(ActionEvent e) {
        if (game.isGameOver() == false) {
            game.incrementTurnsCounter();
            game.setUserSymbol();
            int index = getMove((JButton) e.getSource());
            int x = 0; // row coordinate
            int y = 0; // column coordinate
            switch (index) {
                case 0: x = 0;
                        y = 0;
                        break;
                case 1: x = 0;
                        y = 1;
                        break;
                case 2: x = 0;
                        y = 2;
                        break;
                case 3: x = 1;
                        y = 0;
                        break;
                case 4: x = 1;;
                        y = 1;
                        break;
                case 5: x = 1;
                        y = 2;
                        break;
                case 6: x = 2;
                        y = 0;
                        break;
                case 7: x = 2;
                        y = 1;
                        break;
                case 8: x = 2;
                        y = 2;
                        break;
                default: break;
            }
            game.setFieldOwner(game.getUserSymbol(), x, y);
            ((JButton) e.getSource()).setText((game.getUserSymbol()).toString());
            ((JButton) e.getSource()).setEnabled(false);
        }
    }

    /**
     * Returns the index of the current JButton.
     * 
     * @param       the button that was clicked.
     */
    public int getMove(JButton button) {
        int index = 0;
        for (int i = 0; i < 9; i++) {
            if (button == view.buttons[i]) {
                index = i;
            }
        }
        return index;
    }

    /**
     * Informs the user of the outcome of the game depending on if someone won or not.
     */
    public void informOutcome() {
        if (game.getDidSomeoneWin()) {
            System.out.println(game.getDidSomeoneWin());
            System.out.println("Informing win.");
            view.informWin(game.getUserSymbol());
        } else {
            System.out.println("Informing tie.");
            game.printField();
            view.informTie();
        }
    }
}

Board

public class Board {
    private Field[][] gameGrid;
    private static final int BOARD_SIZE = 3;
    public Board(){
        gameGrid = new Field[BOARD_SIZE][BOARD_SIZE];
        for (int i = 0; i < BOARD_SIZE; i++) {
            for (int j = 0; j < BOARD_SIZE; j++) {
                gameGrid[i][j] = Field.getDefault();
            }
        }
    }

    /**
     * Evaluates the board, stores the values into an array, and returns it.
     * 
     * @return      an array containing the score of each possible win condition.
     */
    public int[] evaluateBoard() {
        int[] scores = new int[8];

        // evaluate the first row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[0][i].getOwner() == Symbol.values()[0]) {
                scores[0]++;
            }
            if (gameGrid[0][i].getOwner() == Symbol.values()[1]) {
                scores[0]--;
            }
        }

        // evaluate the second row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[1][i].getOwner() == Symbol.values()[0]) {
                scores[1]++;
            }
            if (gameGrid[1][i].getOwner() == Symbol.values()[1]) {
                scores[1]--;
            }
        }

        // evaluate the third row
        for (int i = 0; i < 3; i++) {
            if (gameGrid[2][i].getOwner() == Symbol.values()[0]) {
                scores[2]++;
            }
            if (gameGrid[2][i].getOwner() == Symbol.values()[1]) {
                scores[2]--;
            }
        }

        // evaluate the first column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][0].getOwner() == Symbol.values()[0]) {
                scores[3]++;
            }
            if (gameGrid[i][0].getOwner() == Symbol.values()[1]) {
                scores[3]--;
            }
        }

        // evaluate the second column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][1].getOwner() == Symbol.values()[0]) {
                scores[4]++;
            }
            if (gameGrid[i][1].getOwner() == Symbol.values()[1]) {
                scores[4]--;
            }
        }

        // evaluate the third column
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][2].getOwner() == Symbol.values()[0]) {
                scores[5]++;
            }
            if (gameGrid[i][2].getOwner() == Symbol.values()[1]) {
                scores[5]--;
            }
        }

        // evaluate the left-to-right diagonal
        for (int i = 0; i < 3; i++) {
            if (gameGrid[i][i].getOwner() == Symbol.values()[0]) {
                scores[6]++;
            }
            if (gameGrid[i][i].getOwner() == Symbol.values()[1]) {
                scores[6]--;
            }
        }

        // evaluate the right-to-left diagonal
        for (int i = 0; i < 3; i++) { // rows
            for (int j = 0; j < 3; j++) { // columns
                if (i + j == 2) {
                    if (gameGrid[i][j].getOwner() == Symbol.values()[0]) {
                        scores[7]++;
                    }
                    if (gameGrid[i][j].getOwner() == Symbol.values()[1]) {
                        scores[7]--;
                    }
                }
            }
        }

        return scores;
    }

    /**
     * Sets the symbol of a specific field.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public void setFieldOwner(Symbol owner, int x, int y) {
        gameGrid[x][y].setOwner(owner);
    }

    /**
     * Returns the owner of a specific field.
     * Note: I'm using this just for testing.
     * 
     * @param x     the value for the x coordinate of the Field
     * @param y     the value for the y coordinate of the Field
     */
    public Symbol getFieldOwner(int x, int y) {
        return gameGrid[x][y].getOwner();
    }
}

I didn't change the way you manage the player, but I would recommend the creation of a Class Player, and in the game you could have the Player[] and another variable indicating the who turn it is.

I hope this can help you :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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