5
\$\begingroup\$

I'm trying to make a good MVC model using annotations, which I will use it in a large scale GUI project. I want to respect the maximum of rules and guidelines, and be able to decorrelate every parts of the MVC. The code must be beautiful, testable and maintainable. I wrote a sample project to show you my model. What do you think about it?

The launcher:

package example;

import javax.swing.SwingUtilities;

/**
 * Test class.
 * 
 * @author AwaX
 * @date Feb 10, 2015
 */
public class Example_MVC {

    public static void main (String[] args) {
        SwingUtilities.invokeLater(new Runnable() {

            @Override
            public void run () {
                final Model model = new Model();
                final Controller controller = new Controller(model);
                controller.showGui();
            }
        });
    }
}

The data model:

package example;

import java.util.Observable;

/**
 * Data model.
 * 
 * @author AwaX
 * @date Feb 10, 2015
 */
public class Model extends Observable {

    private int counter;

    /**
     * Instanciate a default data model.
     */
    public Model () {
        super();
        this.counter = 0;
    }

    /**
     * Notify data model observers with new update.
     * 
     * @param arg
     *            Allow to specify a parameter for the observer.
     */
    public final void notifyChanged (Object... arg) {
        setChanged();
        if (arg.length == 0) {
            notifyObservers();
        } else if (arg.length == 1 && arg[0] != null) {
            notifyObservers(arg[0]);
        } else {
            throw new IllegalArgumentException("Only one argument allowed");
        }
        clearChanged();
    }

    public void inc () {
        this.counter++;
        notifyChanged(this.counter);
    }

    public int getCounter () {
        return counter;
    }

    public void setCounter (int c) {
        this.counter = c;
        notifyChanged(this.counter);
    }
}

The Controller:

package example;

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import example.View.ActionCallback;

/**
 * UI controller.
 * 
 * @author AwaX
 * @date Feb 10, 2015
 */
public class Controller {

    private final Model model;
    private final View view;
    private final ScheduledExecutorService scheduler;

    /**
     * Instanciate the controller.
     * 
     * @param model
     *            Data model.
     */
    public Controller (final Model model) {
        this.model = model;
        this.view = new View(model);
        this.view.addListener(this);
        this.model.addObserver(this.view);
        this.scheduler = Executors.newScheduledThreadPool(1);
        this.scheduler.scheduleAtFixedRate(new IncrementTask(), 0, 1000, TimeUnit.MILLISECONDS);
    }

    /**
     * Show the GUI.
     */
    public void showGui () {
        this.view.showGui();
    }

    @ActionCallback (event = "action1")
    public void action1 () {
        System.out.println("# Action 1");
    }

    @ActionCallback (event = "action2")
    public void action2 () {
        System.out.println("# Action 2");
    }

    /**
     * Increment the data model periodically.
     * 
     * @author AwaX
     * @date Feb 10, 2015
     */
    private class IncrementTask implements Runnable {

        @Override
        public void run () {
            model.inc();
        }
    }
}

The View:

package example;

import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.lang.annotation.Annotation;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Observable;
import java.util.Observer;

import javax.swing.AbstractAction;
import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JTextField;

/**
 * User interface.
 * 
 * @author AwaX
 * @date Feb 10, 2015
 */
public class View extends JFrame implements Observer {

    private static final long serialVersionUID = 4240474354742402647L;

    @Retention (RetentionPolicy.RUNTIME)
    @Target (ElementType.METHOD)
    public @interface ActionCallback {
        String event();
    }

    private final ArrayList<Object> listeners;

    private JButton btn1;
    private JButton btn2;
    private JTextField tf;

    /**
     * Instanciate the user interface.
     * 
     * @param model
     *            Data model.
     */
    public View (final Model model) {
        super("MVC Example");
        this.listeners = new ArrayList<>();
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        createGui();
    }

    /**
     * Show the GUI.
     */
    public void showGui () {
        setMinimumSize(new Dimension(300, 200));
        pack();
        setLocationRelativeTo(null);
        setVisible(true);
    }

    /**
     * Allow an object to subscribe for UI events.
     * 
     * @param o
     *            The subscriber object.
     */
    public void addListener (Object o) {
        if (!this.listeners.contains(o)) {
            this.listeners.add(o);
        }
    }

    /**
     * Create the GUI and add listeners.
     */
    private void createGui () {
        this.btn1 = new JButton("Button 1");
        this.btn2 = new JButton("Button 2");
        this.tf = new JTextField();

        setLayout(new BoxLayout(getContentPane(), BoxLayout.PAGE_AXIS));
        add(this.btn1);
        add(this.btn2);
        add(this.tf);

        this.btn1.setAction(new ButtonAction("action1"));
        this.btn2.setAction(new ButtonAction("action2"));
    }

    @Override
    public void update (Observable o, Object arg) {
        if (o instanceof Model) {
            Model model = (Model) o;
            this.tf.setText("" + model.getCounter());
        }
    }

    /**
     * Listener on button actions.
     * 
     * @author AwaX
     * @date Feb 10, 2015
     */
    private class ButtonAction extends AbstractAction {

        private static final long serialVersionUID = 9077483825287720181L;

        public ButtonAction (final String name) {
            super(name);
        }

        @Override
        public void actionPerformed (ActionEvent e) {
            for (Object o : listeners) {
                for (Method m : o.getClass().getMethods()) {
                    for (Annotation a : m.getAnnotations()) {
                        if (a instanceof ActionCallback) {
                            ActionCallback callback = (ActionCallback) a;
                            if (callback.event().equals(e.getActionCommand())) {
                                try {
                                    m.invoke(o);
                                } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e1) {
                                    e1.printStackTrace();
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I don't consider the code presented stub-code. While the code presented actually is an example, I think it's implemented in the best knowledge and can be seen as a Proof of Concept implementation of this Annotation-Driven event dispatching. I think it's giving sufficient context for a review and is close enough to production code. \$\endgroup\$ – Vogel612 Feb 11 '15 at 10:19
6
\$\begingroup\$

Wow. The idea is nice, but I'll say it clearly: You failed at creating a beautiful, testable and maintainable architecture. Miserably :(

But let's start with first things first:

import javax.swing.SwingUtilities;

This seems to be the first error you've made: Chose your technologies.

Swing has officially reached end-of-life status:

6. Is JavaFX replacing Swing as the new client UI library for Java SE?

Yes. [...] While we recommend developers to leverage JavaFX APIs as much as possible when building new applications, it is possible to extend a Swing application with JavaFX, allowing for a smoother transition.

This seems to be your first and probably most grave mistake in this application. But let's keep going.

Your goals were testable & maintainable code. (the beautiful is a different story).
For that you need to rigorously separate classes from each other to allow proper unit-testing, which should be the first step to take.

Your classes on the other hand are tightly coupled. Changing the implementation of one of View, Model or Controller will almost automatically require changes in at least one of the others, which in turn will require more changes. Simple refactorings quickly become maintenance nightmares that way.

But this wouldn't be CodeReview if there was no solution for this. Java supplies a convenient way to allow you to specify behavior of a class without actually implementing a class: Interfaces

There's two simple benefits to interfaces:

  1. You can change implementation details in classes without having to worry all that much about other files, as long as you keep the specified contract intact.
  2. You can easily mock them for clean unit-tests

This provides a much better testability and maintainability.

Which brings me to the next point on my list. You're using standard Java's Observable. I have a personal dislike against Java's Observable, since it's use is extremely limited.

For starters anything abstract you'd want to make an Observable has to be an abstract class, since interfaces can't inherit from classes, this in turn limits you as Programmer when trying to properly decouple your code for testing and clean maintainable code.

Luckily it's not that hard to find a way around this: Roll your own observable implementation and if you have the spare slot, just extend a DefaultObservable of your own making.

Now let's see what that would do to your code:

public interface Model implements MyObservable {
     // methods shared across all models
}

public interface View implements MyObserver {
    void showGui();
    void addListener();
    // maybe removal of listeners, other things all your views do
}

public interface Controller {
    void showGui();
}

and now your classes are free to be properly named and unit-tested.

But since this is mostly a design review I still have some minor and major nitpicks about the actual code you've written:

  • Program against interfaces, the listeners in your gui shouldn't be forced into an ArrayList<>. What if you'd wanted to switch for a CopyOnWriteArrayList<>? Additionally a List seldom makes sense for listeners. There's no listener that would require to be notified twice about an event. Choose the right collection for the right job and use a Set instead. It intrinsically doesn't allow duplicate entries.

  • try to avoid pack(). It can be quite the performance eater and significantly slow down your GUI loading. Instead properly lay out your components using an inherently resizable-friendly LayoutManager (if you insist on using Swing), or switch to JavaFX altogether which is somewhat more forgiving on that matter

  • Try to avoid deeply nested loops wherever possible (and try to avoid needless Reflection usage). Deeply nested loops can be significantly simplified by extracting the relevant parts into separate methods, which gives greater reusability

  • Your createGui()'s Javadoc is a lie. You don't register listeners, you register ButtonActions. Generally your javadoc is rather meager (well it's an example implementation mostly)

  • ButtonAction as well as @ActionCallback should have their own classfiles. This allows greater reusability and easier taking over into an actual project.

  • You could have simplified ActionCallback usage by using the magic-attribute of interfaces (namely value()), which allows the following:

    @ActionCallback("action1")
    public void action1() {
        // code here
    }
    
  • Your scheduler is never cleaned up properly, you let the JFrame's EXIT_ON_CLOSE call System.exit and are happy. You shouldn't be.

  • You should strongly consider going for Java 8 as technology. It removes lots of boilerplate, especially with the Runnables.

\$\endgroup\$
  • \$\begingroup\$ Ouch ^^ Thanks for the advices. So you think it's a good idea to use reflection in this case ? My main motivation here is to be able to make the view completely independant. I want to switch to JavaFx and JDK8 but unfortunately my company is really slow and JDK7 is the most you can have :) For the loops part, I don't know how to invoke methods in another way :/ \$\endgroup\$ – AwaX Feb 11 '15 at 11:47
  • 1
    \$\begingroup\$ you might want to take a deeper look at MethodHandles if you want to avoid Reflection. For what it's worth, Reflection is IMO often overly complicating simple things. I personally have no experience with MethodHandles yet, but they seem to be a really clean alternative (even works in Java 7;)) Reflection should always be the very last resort IMO, but that's just my 2 cents \$\endgroup\$ – Vogel612 Feb 11 '15 at 11:54
4
\$\begingroup\$

Is it beautiful? I don't know, but it's very readable for the most part. You have a pension for single letter variable names though. I don't care for that. Yes, yes... naming is the hardest thing in programming, but we that just means we have to try hard at it. Names like o and a don't tell me anything about what the variable actually is and represents. As a maintainer, I have to keep looking around the implementation details to remind myself.

The other thing I notice is this method.

/**
 * Listener on button actions.
 * 
 * @author AwaX
 * @date Feb 10, 2015
 */
private class ButtonAction extends AbstractAction {

    private static final long serialVersionUID = 9077483825287720181L;

    public ButtonAction (final String name) {
        super(name);
    }

    @Override
    public void actionPerformed (ActionEvent e) {
        for (Object o : listeners) {
            for (Method m : o.getClass().getMethods()) {
                for (Annotation a : m.getAnnotations()) {
                    if (a instanceof ActionCallback) {
                        ActionCallback callback = (ActionCallback) a;
                        if (callback.event().equals(e.getActionCommand())) {
                            try {
                                m.invoke(o);
                            } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e1) {
                                e1.printStackTrace();
                            }
                        }
                    }
                }
            }
        }
    }
}

Holy arrow code batman! =)

You might want to try to reduce the levels of nesting here. There's are several approaches you can take to do that, but neither do I know Java well enough nor have I studied your code hard enough to suggest how.

\$\endgroup\$
  • \$\begingroup\$ Usually I'm good at finding variable names but there is one case where I don't make the effort : loops, but promise I will :) I agree with you, the nested loops are ugly and have to be optimized but I don't know how to invoke the annotated method in an easier way. You have to iterate over all the methods to get the associated annotation and still be able to invoke the specific method after :/ \$\endgroup\$ – AwaX Feb 12 '15 at 9:52

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.