fritzthecat-blog: Java MVC Example Review

We are living in the age of hand-written software. Source codes must be regarded as individual problem solutions, created by people with different motivations and experiences. Would you give three developers the same problem to solve, the outcome would be three different implementations, all of them differently readable and maintainable. It is possible that developer A has significant problems to maintain the solution of developer B, although both had solved the same problem before.

Futurologists predict the end of hand-written software since decades now. Believing them would mean that customers by themselves put together the applications they need. Generic computer programs will generate bug-free source code for them. Programming is like weaving, and the weavers died out.

There is no doubt that things will go into this direction, and it would be silly to be against such automation. But how will the programs that generate bug-free source come to life? Hand-written:-? It does not matter how we solve these problems, hand-written software will always behind the solution.

In future we will do more generic programming, we will write software that carries out the same things that generated source code would have done. I see it becoming more abstract, and more reusable and reused. Nobody is writing sort-algorithms any more (although this might be a nice hobby:-)

To transform source code to average shapes and concepts that everybody would understand, we need code reviews. In this Blog I would like to review the MVC Swing Example of my latest Blog. Did it conform to MVC? Does it contain code duplications, does it violate best practices?

Critics

It may be useful to open another browser-window and put the source code of the example beside this page, because I will repeat just excerpts of it here.

Model

The first thing that comes to my mind when I see TemperatureModel is that it is composed of three classes:

listener support
properties exposition (getters, setters)
business logic (Celsius-Fahrenheit conversion)

So, shouldn't there be three separate classes, because a class should have just one single responsibility?

Good idea. In practice, you would have a reusable AbstractModel that supports a generic observer notification like it was done via modelChanged(). Property setters of sub-classes will need to call fireChanged().

package mvc;

import java.util.*;

public class AbstractModel
{
    public interface Listener
    {
        public void modelChanged();
    }

    private final List<Listener> listeners = new ArrayList<>();

    public void addListener(Listener view) {
        removeListener(view);
        listeners.add(view);
    }

    public void removeListener(Listener view) {
        listeners.remove(view);
    }

    protected final void fireChanged() {
        for (Listener listener : listeners)
            listener.modelChanged();
    }
}

In a sub-class we could define the properties and the business logic:

package mvc.model2views;

import mvc.AbstractModel;

public class TemperatureModel extends AbstractModel
{
    private Integer temperatureInCelsius;
    
    public Integer getTemperatureInCelsius() {
        return temperatureInCelsius;
    }

    public void setTemperatureInCelsius(Integer temperatureInCelsius) {
        this.temperatureInCelsius = temperatureInCelsius;
        fireChanged();
    }

    public Integer getTemperatureInFahrenheit() {
        return toFahrenheit(getTemperatureInCelsius());
    }

    public void setTemperatureInFahrenheit(Integer temperatureInFahrenheit) {
        setTemperatureInCelsius(toCelsius(temperatureInFahrenheit));
    }

    private Integer toFahrenheit(Integer valueInCelsius) {
        if (valueInCelsius == null)
            return null;

        return Integer.valueOf((int) Math.round(valueInCelsius * 1.8) + 32);
    }

    private Integer toCelsius(Integer valueInFahrenheit) {
        if (valueInFahrenheit == null)
            return null;

        return Integer.valueOf((int) Math.round((valueInFahrenheit - 32) / 1.8));
    }
}

About the third class containing business logic:

it does not make sense to separate the business logic from properties, because that logic is about the properties! Separating data and methods working on them is opposite to the object-oriented idea.

View

I won't review the Swing view implementations here. These are just the TemperatureView interface implementation, and thus not part of the MVC mechanisms.

Questionable is the genericity of getAddableComponent(), returning Object instead of the windowing-system specific type.

public interface TemperatureView
{
    ....

    Object getAddableComponent();

    ....
}

I can fix this by introducing a generic W type for the addable component used by the windowing system:

public interface TemperatureView<W>
{
    ....

    W getAddableComponent();

    ....
}

This must be declared and used as return by the concrete implementation:

public class SwingTemperatureView implements TemperatureView<JComponent>
{
    ....

    @Override
    public JComponent getAddableComponent() {
        return view;
    }

    ....

Now the Demo application does not need to cast the return any more:

        ....

        final SwingTemperatureView view = new SwingTemperatureView();
        ....

        frame.getContentPane().add(view.getAddableComponent());
        ....

Controller

The controller being just a few lines of code, can there be design flaws?

The controller always implements the view-listener interface. You may remember that I defined the TemperatureView.Listener interface to contain two notification methods:

public interface TemperatureView
{
    public interface Listener
    {
        void inputCelsius(Integer newCelsius);
        
        void inputFahrenheit(Integer newFahrenheit);
    }
    
    void addListener(Listener controller);

    ....

}

This Listener is part of the data-binding. When we want to bind fields unbuffered to the model (no "Save" button), we need exactly one view-listener method for each of these fields.

As it is programmed in the example, the model-property is bound just once to the view-field, as to be seen in call-hierarchy of model.getTemperatureInCelsius(). But the view-field is bound two times, once to the controller's listener-method belonging to that property, and from there to the model-property.

View to controller:

package mvc.model2views.viewimpl;

import mvc.model2views.TemperatureView.Listener;

class SwingCelsiusField extends SwingTemperatureField
{
    ....
    
    @Override
    protected void input(Integer celsius, Listener listener) {
        listener.inputCelsius(celsius);
    }

Controller to model:

package mvc.model2views;

public class TemperatureController implements TemperatureView.Listener
{
    ....

    @Override
    public void inputCelsius(Integer newTemperatureInCelsius) {
        if (validate(newTemperatureInCelsius))
            model.setTemperatureInCelsius(newTemperatureInCelsius);
    }

In other words, the inputCelsius() call in SwingCelsiusField can not ensure that the receiving controller really calls model.setTemperatureInCelsius() and not model.setTemperatureInFahrenheit(), which would be a typical data-binding bug.

Motivation for this "double binding" is to let the controller perform presentation-logic upon the input, e.g. data-validation.

We can not get rid of this. Seems to be one of the vulnerabilities of MVC. When we would listen to a "Commit" button instead of field-events, the binding would, according to the MVC concept, also happen in the controller. That means the controller would read from the view, perform presentation logic, then write to the model. We would need a TemperatureView.getTemperatureInCelsius() method instead of the TemperatureView.Listener.inputCelsius(). No relief from double binding.

Resume

Code reviews make sense. I came out with a reusable model abstraction. I avoided the cast operator for the view's addable component. Finally I detected a weakness in MVC data-binding.

The controller has more than one responsibility. It performs actions like input field changes, or pushing buttons to add, delete, cut, copy, paste. It handles compound actions like drag & drop that cascade into sub-actions like hovering items. It does data-binding when writing to the model. It is also some kind of MVC-facade, accepting new models from outside, and exposing the addable view component to be part of other views. To improve MVC we must improve the controller. This is the reason why there are so many other concepts like MVP, MVC2, MVVM, MVW etc. It is the controller that gets replaced in all these concepts.

The MVC example I reviewed here may not be representative for all problem areas that have to be solved by MVC. Action-buttons were missing, rendering graphs of model domain-objects in a view was missing, complex view structures like lazy-loading tabs, tables, trees and their editing were missing. Nevertheless the example showed the basic minimal structure of an MVC implementation.

In my next Blog I will try to find out whether MVP improves MVC.

fritzthecat-blog

Blog-Archiv

Samstag, 11. Februar 2017

Java MVC Example Review

Critics

Model

View

Controller

Resume

Keine Kommentare:

Über mich