Tic Tac Toe in MVC

Question

Do I have basics of MVC and SOLID principles right in this simple 'Tic Tac Toe' game?

View.java

import javax.swing.GroupLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.WindowConstants;

class View extends JFrame{
    JButton[] knobs;
    JButton newGame;
    JLabel indicator;


    public View(){
        initComponents();
        setLayout();
        setVisible(true);
        pack();
        setTitle("Tic Tac Toe");
        setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    }

    public void initComponents(){
        this.knobs = new JButton[9];
        //TODO: Czy ini kazdego elementu jest potrzebne
        for (int i = 0;i<this.knobs.length;i++) {
            this.knobs[i] = new JButton(" ");

        }
        this.indicator = new JLabel("X");
        this.newGame = new JButton("←");// DEADLINE
        this.newGame.setEnabled(false);

    }

    private void setLayout(){
        /*Tworze i ustawiam LayoutManagera'a*/
        GroupLayout layout = new GroupLayout(getContentPane());
        getContentPane().setLayout(layout);

        /*Automatyczne przestrzenie między komponentami*/
       layout.setAutoCreateGaps(true);
       layout.setAutoCreateContainerGaps(true);

        layout.setHorizontalGroup(layout.createParallelGroup()
                .addComponent(newGame, GroupLayout.Alignment.CENTER)
                .addComponent(indicator, GroupLayout.Alignment.CENTER)
                .addGroup(layout.createSequentialGroup()
                    .addGroup(layout.createParallelGroup()
                        .addComponent(knobs[0])
                        .addComponent(knobs[1])
                        .addComponent(knobs[2]))
                    .addGroup(layout.createParallelGroup()
                        .addComponent(knobs[3])
                        .addComponent(knobs[4])
                        .addComponent(knobs[5]))
                    .addGroup(layout.createParallelGroup()
                        .addComponent(knobs[6])
                        .addComponent(knobs[7])
                        .addComponent(knobs[8])
        )));

        layout.setVerticalGroup(layout.createSequentialGroup()
            .addComponent(newGame)
            .addComponent(indicator)
            .addGroup(layout.createSequentialGroup()
                .addGroup(layout.createParallelGroup()
                    .addComponent(knobs[0])
                    .addComponent(knobs[3])
                    .addComponent(knobs[6]))
                .addGroup(layout.createParallelGroup()
                    .addComponent(knobs[1])
                    .addComponent(knobs[4])
                    .addComponent(knobs[7]))
                .addGroup(layout.createParallelGroup()
                    .addComponent(knobs[2])
                    .addComponent(knobs[5])
                    .addComponent(knobs[8])
        )));
    }

    public void changePlayer() {
        String newPlayer = reverseValue(getCurrentPlayerString());
        this.indicator.setText(newPlayer);
    }

    public void winGame() {
        this.indicator.setText(getCurrentPlayerString() + " wygrał!");
        for(JButton knob : this.knobs) {
            knob.setEnabled(false);
        }
        endGame();
    }

    public String reverseValue(String value) {
        if("X".equals(value)) {
            return "O";
        } else if("O".equals(value)) {
            return "X";
        }
        return null;
    }

    public Value reverseValue(Value value) {
        if(value == Value.X) {
            return Value.O;
        } else if (value == Value.O) {
            return Value.X;
        }
        return null;
    }

    public Value getCurrentPlayerValue() {
         if("X".equals(this.indicator.getText())) {
            return Value.X;
        } else if("O".equals(this.indicator.getText())) {
            return Value.O;
        }
         return null;
    }

    public String getCurrentPlayerString() {
         if("X".equals(this.indicator.getText())) {
            return "X";
        } else if("O".equals(this.indicator.getText())) {
            return "O";
        }
         return null;
    }

    void resetGame() {
        this.indicator.setEnabled(true);
    }
    void setNoWinner() {
        this.indicator.setText(null);
    }
    void endGame() {
        this.newGame.setEnabled(true);
    }

}

Model.java

public class Model {

    int[][] board = new int[3][3];
    public int movesCounter = 0;

    public void setChoice(Integer field, Value value) {
        Integer matrixValue = null;
        if (value == Value.X) {
            matrixValue = -1;
        } else if (value == Value.O) {
            matrixValue = 1;
        }
        board[field % 3][field / 3] = matrixValue;
        movesCounter++;
    }

    public boolean checkBoard() {
        int sumDiagonalLR = 0;
        int sumDiagonalRL = 0;
        int sumColumns = 0;
        int sumVerses = 0;

        for (int i = 0; i <= 2; i++) {
            sumDiagonalLR += board[i][i];
        }
        for (int i = 0; i <= 2; i++) {
            sumDiagonalRL += board[i][2 - i];
        }
        if (Math.abs(sumDiagonalLR) == 3 || Math.abs(sumDiagonalRL) == 3) {
            return true;
        }
        for (int i = 0; i <= 2; i++) {
            for (int j = 0; j <= 2; j++) {
                sumColumns += board[j][i];
                sumVerses += board[i][j];

            }
            if (Math.abs(sumColumns) == 3 || Math.abs(sumVerses) == 3) {
                return true;
            } else {
                sumColumns = 0;
                sumVerses = 0;
            }
        }
        return false;
    }

}

Controller.java

package app;

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

/**
 *
 * @author mateusz
 */
public class Controller implements ActionListener{
    Model model;
    View view;

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

    private void addActionListeners(){
    for (JButton knob : view.knobs) {
            knob.addActionListener(this);
            view.newGame.addActionListener(this);
        }
    }
    private boolean addChoice(Integer field, Value value) {
        model.setChoice(field, value);
        if(model.movesCounter >= 5) {
            if(model.checkBoard()) {
                view.winGame();
                view.endGame();
                return true;
            }
            if(model.movesCounter == 9) {
                view.endGame();
                view.setNoWinner();
            }
        } 
        return false;
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        Value currentPlayer = view.getCurrentPlayerValue(); //ustala znak gracza w trakcie wciśnięcia

        if (Arrays.asList(view.knobs).contains(e.getSource()) && currentPlayer != null) {//jeśli wciśnięto pole gry i ustawiony był gracz
            Integer knobIndex = Arrays.asList(view.knobs).indexOf((JButton)e.getSource());
            ((JButton) e.getSource()).setText(view.getCurrentPlayerString()); //Nastawia X lub O na przycisku
            view.pack();
            ((JButton) e.getSource()).setEnabled(false); //Dezaktywuje przycisk
            if(!addChoice(knobIndex, currentPlayer)) view.changePlayer();//Przekazuje index elementu w tablicy i ustawiony znak
        } else if(e.getSource().equals(view.newGame)) {
            this.view.dispose();
            this.view = new View();
            addActionListeners();
            this.model = new Model();
        }
        }
    }

Value.java

public enum Value {
    X, O
}

TicTacToe.java

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

Download: https://drive.google.com/file/d/0B6j704LEQ3wVRU5BVEMtWklsdWs/view?usp=sharing

Answer 1

It looks like your MVC-approach is fine. Your model does not directly know about the view or the controller, which is the most important part.

About SOLID, let's see...

first of all, do you know what SOLID is?

Single Responsibility Principle

I don't think reverseValue and getCurrentPlayer* methods belong in your view. You could add a reverse method to your Value enum. Also, that enum could deserve having a NONE value as well, to avoid using null.

Additionally, your actionPerformed method is responsible for two very different operations: Clicking the "new game button" and making a move inside the game. I would use two separate ActionListeners for this. If you're using Java 8, I would even use Lambdas. Otherwise, feel free to use anonymous inner classes or anything else.

Interface Segregation Principle + Dependency Inversion Principle

You're not using any interfaces at all, so this is not SOLID. (I personally think it's a bit overkill to use a whole bunch of interfaces for this simple program though).

Other comments

You're currently doing:

private void addActionListeners(){
    for (JButton knob : view.knobs) {
        knob.addActionListener(this);
        view.newGame.addActionListener(this);
    }
}

You're doing view.newGame.addActionListener(this); nine times (because it is inside the loop), as you're doing it inside the loop. Do it outside the loop instead.

What you should do:

private void addActionListeners(){
    for (JButton knob : view.knobs) {
        knob.addActionListener(this);
    }
    view.newGame.addActionListener(this);
}

---

Your checkBoard method has a nested loop which would be better like this:

    for (int i = 0; i <= 2; i++) {
        int sumColumns = 0;
        int sumVerses = 0;
        for (int j = 0; j <= 2; j++) {
            sumColumns += board[j][i];
            sumVerses += board[i][j];

        }
        if (Math.abs(sumColumns) == 3 || Math.abs(sumVerses) == 3) {
            return true;
        }
    }

---

Three is a magic number. I'd recommend using a constant for that, also using < 3 instead of <= 2 at places.

---

this.knobs = new JButton[9];

if (Arrays.asList(view.knobs).contains(e.getSource())

Why not use an ArrayList right from the start? There's not really a need to convert the array to a list every single time (although that operation is fast).