MVC: Should my domain model look like this?

Question

Consider the following GUI screen (Java Swing) with a list of persons where the user can select a person and delete it.

According to MVC, the view observes (observer pattern) the model and updates itself. Business logic and domain data in MVC goes to model (as far as I am aware of). My question is, should the model be like a service and have a personList.delete(Person p) method or have a selectedPerson field and be personList.delete().

I have two alternate versions of this GUI and I want to know which one is "more MVC".

The one is:

public class Example {

    public static void main(String[] args) {
        SwingUtilities.invokeLater(Example::runGui);
    }

    public static void runGui() {

        Persons persons = new Persons();
        DefaultListModel<Person> personsSwingModel = new DefaultListModel<>();
        JList<Person> personsJList = new JList<>(personsSwingModel);
        personsJList.setVisibleRowCount(15);
        personsJList.setCellRenderer(new DefaultListCellRenderer());

        persons.addPropertyChangeListener("data", e -> {
            syncListData(personsSwingModel, persons);
        });
        syncListData(personsSwingModel, persons);

        personsJList.getSelectionModel().addListSelectionListener(e -> {
            persons.setSelectedPerson(personsJList.getSelectedValue());
        });

        persons.addPropertyChangeListener("selection", e -> {
            personsJList.setSelectedValue(persons.getSelectedPerson().orElse(null), true);
        });

        JButton deleteButton = new JButton("Delete selected");

        deleteButton.addActionListener(e -> {
            persons.delete();
        });

        JFrame frame = new JFrame("Example");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLayout(new BorderLayout());

        frame.add(new JScrollPane(personsJList), BorderLayout.CENTER);
        frame.add(deleteButton, BorderLayout.PAGE_END);

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

    }

    private static void syncListData(DefaultListModel<Person> personsSwingModel, Persons persons) {
        personsSwingModel.removeAllElements();
        persons.getPersons().forEach(personsSwingModel::addElement);
    }

    private static class Persons {
        private List<Person> persons;
        private SwingPropertyChangeSupport listeners;
        private Person selectedPerson;

        public Persons() {
            persons = new ArrayList<>();
            persons.add(new Person("Mike", "Random"));
            persons.add(new Person("Alice", "Something"));
            persons.add(new Person("John", "Rambo"));
            persons.add(new Person("Jack", "Sparrow"));
            listeners = new SwingPropertyChangeSupport(this);
        }

        public List<Person> getPersons() {
            return persons;
        }

        public void delete() {
            if (selectedPerson == null)
                return;

            persons.remove(selectedPerson);
            listeners.firePropertyChange("data", null, null);
        }

        public void setSelectedPerson(Person selectedPerson) {
            this.selectedPerson = selectedPerson;
            listeners.firePropertyChange("selection", null, null);
        }

        public Optional<Person> getSelectedPerson() {
            return Optional.ofNullable(selectedPerson);
        }

        public void addPropertyChangeListener(String property, PropertyChangeListener listener) {
            listeners.addPropertyChangeListener(property, listener);
        }
    }

    private static class Person {
        String firstName;
        String lastName;

        public Person(String first, String last) {
            this.firstName = first;
            this.lastName = last;
        }

        @Override
        public String toString() {
            return firstName + " - " + lastName;
        }
    }
}

And the other:

public class Example {

    public static void main(String[] args) {
        SwingUtilities.invokeLater(Example::runGui);
    }

    public static void runGui() {

        Persons persons = new Persons();
        DefaultListModel<Person> personsSwingModel = new DefaultListModel<>();
        JList<Person> personsJList = new JList<>(personsSwingModel);
        personsJList.setVisibleRowCount(15);
        personsJList.setCellRenderer(new DefaultListCellRenderer());

        persons.addPropertyChangeListener("data", e -> {
            syncListData(personsSwingModel, persons);
        });
        syncListData(personsSwingModel, persons);

        JButton deleteButton = new JButton("Delete selected");

        deleteButton.addActionListener(e -> {
            persons.delete(personsJList.getSelectedValue());
        });

        JFrame frame = new JFrame("Example");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLayout(new BorderLayout());

        frame.add(new JScrollPane(personsJList), BorderLayout.CENTER);
        frame.add(deleteButton, BorderLayout.PAGE_END);

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

    }

    private static void syncListData(DefaultListModel<Person> personsSwingModel, Persons persons) {
        personsSwingModel.removeAllElements();
        persons.getPersons().forEach(personsSwingModel::addElement);
    }

    private static class Persons {
        private List<Person> persons;
        private SwingPropertyChangeSupport listeners;

        public Persons() {
            persons = new ArrayList<>();
            persons.add(new Person("Mike", "Random"));
            persons.add(new Person("Alice", "Something"));
            persons.add(new Person("John", "Rambo"));
            persons.add(new Person("Jack", "Sparrow"));
            listeners = new SwingPropertyChangeSupport(this);
        }

        public List<Person> getPersons() {
            return persons;
        }

        public void delete(Person p) {
            persons.remove(p);
            listeners.firePropertyChange("data", null, null);
        }

        public void addPropertyChangeListener(String property, PropertyChangeListener listener) {
            listeners.addPropertyChangeListener(property, listener);
        }
    }

    private static class Person {
        String firstName;
        String lastName;

        public Person(String first, String last) {
            this.firstName = first;
            this.lastName = last;
        }

        @Override
        public String toString() {
            return firstName + " - " + lastName;
        }
    }
}

The two examples differ on "selectedPerson" property. At first, the first approach seems to be more appropriate since other views can listen (observe) to this selection as well. However, what I find weird in a real scenario is that if I follow this approach, I end up with too much "gui properties" in my models. Should my domain models look like this? Should selection be part of my domain model? And if selection goes in this model, how is that different from a Presentation Model? (ignoring the data binding difference). Finally, the first approach, I can say that makes the use case more clear ("the user can delete the selected person").

The second approach, the model looks like a service and it is definitely different from a Presentation Model. Zero GUI concerns. The model fetches the data from the database, and handles the use case "user can delete the selected person" with the controller providing the selected person. However, I do not know what happens when other views want to observe the selection. (Technically they can by accessing the Swing's list selection model, but yikes).

Now, say I am adding a search field at the top to let user search fast for persons:

If the first approach is taken, the domain model must be touched and have a "searchText" property and sync with the view. But should it be? While in the second approach, only view and the controllers are edited. The controller just asks getPersons filter them based on search text and shows the result.

I do not want to follow something like "oh, if other views want to observe the X property, it goes in a model, otherwise the state of X stays as a GUI concern".

Answer 1

There are different ideas what MVC means and where to put code. There is in my opionion an old view that describes that your main logic should be in the Controller, and your model should be thin, merely a way to access your data.

A newer idea is that your model should contain all your business logic and should be fat. That's why this approach is called fat model. Your Controller is the layer that communicates with your user and updates the view accordingly.

Let's describe it more general.

View
Contains everything for your View. In your example it's Swing. In more general you could say, its how your program represents data.

Model
All your business logic goes in here.

Controller
So what does the controller?

---

To better understand where to put code, you should make a thought experiment. This also makes it clearer, what the Controller does.

The thought experiment is simple. You not only want to create a desktop application for your buisness, you also want to create, let's say a Web Fronted and a CLI Tool. The goal is to achieve maximum code reuse.

The model in this experiment is a library that can be reused in your different applications. Let's say you want the typical CRUD operations for a Person. Then your model contains all those operations in a high-level idea to support it. Like

model.createPerson(...)
model.fetchPersonById(...)
model.searchPersons(...)
model.updatePerson(...)
model.deletePersonById(...)

It should not just be something like a database connection, that you query in your controller. And sure, you also add all other kind of functions that you need for your application.

The controller is just dump. It only determines what a user wants to do and then delegates it to the right model method/function.

You can think of the Controller as the thing, that communicates with the user. It handles whatever the user typed in, clicked in the UI, and so on.

Let's say you write a CLI program. Then your view would be some code that turns your model result into formated text for the user or turns it into something like JSON, CVS, whatever ...

The controller would be what parses the input or the command line arguments. Then call the model to do your thing, and then printing the result back to the user. Tell him the operation was completed, etc.

In a web application. The View is whatever you use to create your HTML.

The Controller would be something that maps a specific URL to a logic. For example that the url "/user/delete/id/1204" deletes the user with the id "1204" from your database.

In every of those examples. The model contains the logic of connecting to the database. Abstracting away your database. The model is not just data. Its what all your domain is about. And providing all the functionality you need in your application.

In every application there should be a simple line like

model.deleteUser(id)

that deletes the user. However it works. Back to your questions.

Should the Model contain a selectedPerson? The answer is no. Because in a model, there is no such thing as selection. Selection is what the view does. Not the model.

Your UI application can select something. But not the model.

If you want a different UI Element that somehow needs to know what the selected user is. Then yes. Also your new UI Element should watch on changes on the selection e.g. watch your Swing elements for changes. You also can create a selectedPerson property. But it should be part of the controller, never of the model.

But in my experience, you should fetch data directly. In my opinion there is no point in providing something like selectedPerson. Technically it is just a cache, that can be invalidated, and causes problems. If you want to know what is selected, then ask directly.

If it is much code to do this, then create a helper function/method that returns the selected User, but don't save it somewehere.

---

You can think of MVC the following:

1. A User interacts with the application
2. The Controller determines what to do. Calling the Model.
3. The Model executes what needs to be done.
4. The View updates accordingly to the changes of the Model
5. Back to 1.

---

Summary

So in my opinion. Think of the model as a library that you can re-use in different applications.

For example, if i would write a command-line tool that i can call like: myapp --delete-user --id 1204

then something like selectedPerson makes no sense at all in a model, because there is no such thing as selection in a CLI. This makes it obvious that selection is from the view, not from the model.

If you want to search, then your model should also not contain a search text field. Provide something like

let results = model.search(searchPattern)

Your search method does all kind of searching and gives you a result, that you then can use to fill your UI. In a CLI you would turn the result into text and so on.

Answer 2

I think it's easier to answer this kind of question by making a clear separation in the code between the Model, the View, and the Controller. In this case, you have the Controller and the View code intertwined, so the distinction isn't as clear.

So I've split the creation of the View into a separate method called buildView(). Now the layout code is all in one spot, and it's purely layout.

The Model should be, as much as possible, just observable data and should only hold enough code to manage the data itself. No application logic at all. This means that the delete() method should just have the code that does the actual deletion and handles the observable aspect of the data.

What if you had a button on the View called "Delete First Person"? So that wouldn't be the selected Person. What if the action was to delete everyone except the selected person? Putting that logic into your View builds a dependency in the Model to the View - which you don't want to do.

So the deletion logic belongs in the Controller. But the Controller can't have a dependency on the View either, what if it isn't a JButton or anything that triggers an ActionListener? Maybe a keypress? So the controller passes a Runnable to the layout builder, and the layout builder uses it in an appropriate event handler.

Loading data into the Model, even test data, is the Controller's job. So I've moved the data load out of the constructor of the Model, and into a method called by the Controller.

Then it became clear that the "selection" listener wasn't used or needed at all. So it was deleted.

The logic that decides to delete the element based on the currently selected item is defined in the Controller - where it should be. So now the Model only contains data and methods that manipulate the data in an observable manner and it has no knowledge or dependencies on anything external. The View only has a dependency on the Model and a Runnable from the Controller. The controller only has dependencies on the Model and has no knowledge or dependency on the structure or the functionality of the View - both of which are declared in the View constructor.

Also the naming of the model and the data element "persons" was confusing to me. So I renamed the Model class to "Model".

public class Example {

    public static final String DATA_PROPERTY = "data";

    public static void main(String[] args) {
        SwingUtilities.invokeLater(Example::runGui);
    }

    public static void runGui() {
        Model model = new Model();
        JFrame frame = buildView(model, () -> model.getSelectedPerson().ifPresent(model::delete));
        loadPeople(model);
        frame.pack();
        frame.setLocationByPlatform(true);
        frame.setVisible(true);
    }

    private static void loadPeople(Model model) {
        List<Person> persons = new ArrayList<>();
        persons.add(new Person("Mike", "Random"));
        persons.add(new Person("Alice", "Something"));
        persons.add(new Person("John", "Rambo"));
        persons.add(new Person("Jack", "Sparrow"));
        model.setPersons(persons);
    }

    private static JFrame buildView(Model model, Runnable deletionHandler) {
        DefaultListModel<Person> personsSwingModel = new DefaultListModel<>();
        JList<Person> personsJList = new JList<>(personsSwingModel);
        personsJList.setVisibleRowCount(15);
        personsJList.setCellRenderer(new DefaultListCellRenderer());

        model.addPropertyChangeListener(DATA_PROPERTY, e -> syncListData(personsSwingModel, model));
        syncListData(personsSwingModel, model);

        personsJList.getSelectionModel().addListSelectionListener(e -> model.setSelectedPerson(personsJList.getSelectedValue()));

        JButton deleteButton = new JButton("Delete selected");

        deleteButton.addActionListener(e -> deletionHandler.run());

        JFrame frame = new JFrame("Example");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLayout(new BorderLayout());

        frame.add(new JScrollPane(personsJList), BorderLayout.CENTER);
        frame.add(deleteButton, BorderLayout.PAGE_END);
        return frame;
    }


    private static void syncListData(DefaultListModel<Person> personsSwingModel, Model model) {
        personsSwingModel.removeAllElements();
        model.getPersons().forEach(personsSwingModel::addElement);
    }

    private static class Model {
        private final List<Person> persons = new ArrayList<>();
        private final SwingPropertyChangeSupport listeners;
        private Person selectedPerson;

        public Model() {
            listeners = new SwingPropertyChangeSupport(this);
        }

        public void setPersons(List<Person> persons) {
            this.persons.clear();
            this.persons.addAll(persons);
            listeners.firePropertyChange(DATA_PROPERTY, null, null);
        }

        public List<Person> getPersons() {
            return persons;
        }

        public void delete(Person personToRemove) {
            persons.remove(personToRemove);
            listeners.firePropertyChange(DATA_PROPERTY, null, null);
        }

        public void setSelectedPerson(Person selectedPerson) {
            this.selectedPerson = selectedPerson;
        }

        public Optional<Person> getSelectedPerson() {
            return Optional.ofNullable(selectedPerson);
        }

        public void addPropertyChangeListener(String property, PropertyChangeListener listener) {
            listeners.addPropertyChangeListener(property, listener);
        }
    }

    private static class Person {
        String firstName;
        String lastName;

        public Person(String first, String last) {
            this.firstName = first;
            this.lastName = last;
        }

        @Override
        public String toString() {
            return firstName + " - " + lastName;
        }
    }
}

The last part of the question is interesting. Does the selected Person property even belong in the Model? It would be possible to write the View such that it accepted a Consumer<Person> and then extracted the selected item from the JList and passed it to the Consumer when the button was clicked. BUT...that would be putting some of the deletion logic into the View. The button isn't just a button that invokes some unknown external logic, it now has to know to go and extract some data from another screen element and pass it on.

It's probably best to keep the selected Person property in the Model. The View already has a dependency on the structure of the Model, and it's reasonable to expect that any View design would support the selected Person property in that Model. Then the Controller needs nothing more than the Model in order to perform the deletion logic.