Return to Answer

• 1

• 30.3k
• 9
• 82
• 157

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;

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

1. 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?

1.

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

1.

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.

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

1. Using this. is unnecessary here:

this.running = false;
modelState = 0;