Java MVC pattern

Question

I recently posted a question about modeling a system that contains elements that can perform actions (here). The model runs in cycles and parts perform their action every cycle. The model should have some kind of visual representation displaying changes in model state. In my test code the model updated on timer events and the user sees the changes through observed state changes. The user can start or stop the model through the controller.

I wonder if the MVC pattern is suitable for this setup and if I got the basics of the MVC pattern right.

import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.Observable;
import java.util.Observer;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.Timer;

public class Main {

    public static void main(String[] args) {

        Model model = new Model();
        final View view = new View();
        Controller controller = new Controller(model, view);
        view.addController(controller);

        javax.swing.SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                view.createAndShowGUI();
            }
        });
    }

}

class Controller implements ActionListener {

    Model model;
    View view;

    public Controller(Model m, View v) {
        model = m;
        view = v;
        model.addObserver(v);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        if (model.isRunning()) {
            model.stop();
            view.changeCommandText("Start");
        } else {
            model.start();
            view.changeCommandText("Stop");
        }
    }

    public void addModel(Model m) {
        model = m;
    }

    public void addView(View v) {
        view = v;
    }

}

class Model extends Observable {

    private Integer modelState;
    private boolean running;

    private final Timer timer;
    ActionListener taskPerformer;

    public Model() {
        this.running = false;
        modelState = 0;

        taskPerformer = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent evt) {
                changeState();
            }
        };
        timer = new Timer(1000, taskPerformer);
    }

    public boolean isRunning() {
        return running;
    }

    public void changeState() {
        modelState++;
        setChanged();
        notifyObservers(modelState);
    }

    public String getState() {
        return modelState.toString();
    }

    public void start() {
        timer.start();
        running = true;
    }

    public void stop() {
        timer.stop();
        running = false;
    }

}

class View implements Observer {

    private final JLabel output = new JLabel();
    private final JButton button = new JButton("Start");

    public void createAndShowGUI() {
        JFrame frame = new JFrame("MVC test");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        JPanel panel = new JPanel();
        panel.setLayout(null);
        panel.setPreferredSize(new Dimension(240, 50));

        frame.setContentPane(panel);

        output.setSize(100, 30);
        output.setLocation(10, 10);
        panel.add(output);

        button.setSize(100, 30);
        button.setLocation(120, 10);
        panel.add(button);

        frame.pack();
        frame.setVisible(true);
    }

    @Override
    public void update(Observable obs, Object obj) {
        output.setText(((Model) obs).getState());
    }

    public void addController(ActionListener controller) {
        button.addActionListener(controller);
    }

    public void changeCommandText(String command) {
        button.setText(command);
    }

}

Answer 1

It looks quite good, some, mostly minor notes:

1. class Controller implements ActionListener {
   

   Having an actionPerformed method in the controller smells a little bit for me. I'm not sure about that but I'd rename the controller's method to changeModelState() (without any parameter) and use an inner class to call it:

   public void addController(final Controller controller) {
       button.addActionListener(new ActionListener() {
           @Override
           public void actionPerformed(ActionEvent e) {
               controller.changeModelState();
           }
       });
   }
   

   It would eliminate the following imports from the Controller.java, therefore the controller would not depend on Swing and it could be used with other view implementations without importing Swing classes:

   import java.awt.event.ActionEvent;
   import java.awt.event.ActionListener;
   

2. The code contains a Start string in the view:

   private final JButton button = new JButton("Start");
   

   and in the controller as well:

   view.changeCommandText("Start");
   

   I guess the controller should not know about that how the view presents this information. There could be another view which shows pictures/icons instead of a button with a string. It would be loosely coupled if the controller just set a state, for example, with an enum or separate methods for separate states.

3. On bigger projects I'd consider using something else than Observers and Observables. Péter Török's answer on SO has a good explanation about its disadvantages:

   They are not used, because their design is flawed: they are not type safe. You can attach any object that implements Observer to any Observable, which can result in subtle bugs down the line.

   [...]

   Alternative to Java's Observable class?

4.  Model model;
    View view;
   

   These fields could be private. (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

5. public void addModel(Model m) {
       model = m;
   }
   
   public void addView(View v) {
       view = v;
   }
   

   setModule and setView would be more descriptive. add is usually used with collections and implies that the method adds the parameter to a list/set/etc. and it still stores the former value too.

6. This field is only accessed in the constructor:

   ActionListener taskPerformer;
   

   It could be a local variable. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

7. Using this. is unnecessary here:

   this.running = false;
   modelState = 0;