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);
 }
}

• Joeblade: didn't dive into your MVC split, but I do see you work a lot with "X" and "O" while you also have Value.X and Value.O why not banish all use of those strings and replace them with Value.X etc? "X" (the string) is a view thing that should be replaced by your inner representation asap and ideally is never really used. Secondly you use matrixvalue 1, -1 and null . why not use Value.X/ Value.O and Value.EMPTY?

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).

• apex39: Yes, View definitely should't have any non-view related methods, so reverseValue's no use here. Does "Interface Segregation Principle + Dependency Inversion Principle" mean to start writing an application from creating interfaces, instead of classes? Why view.newGame.addActionListener(this); should have separate 9 lines instead of 3 lines of loop? Last 3 comments are totally clear, whole answer is truly helpful and informative. Big thanks.
• Simon Forsberg: @apex39 Edited my answer to clarify. Please look up on wikipedia what SOLID really means, if you don't know about those concepts.