A Model View Controller Pattern implementation. Do I get it right?

Question

I know there has been many questions on the Model-View-Controller pattern, but this question is a follow up on a previous question I asked regarding the implementation of a download manager:

Get files downloaded asynchronously after double clicking on list item (C++)

One of the reviewers suggested that I implement (besides the threading aspect, which was the core of the question) the whole thing using an MVC pattern. As I had never done that before but was extremely interested in finally learning about this, I decided to address improvements to the code by first tackling the MVC implementation. I have been extremely impressed by the quality of the answers I received to my first question. So would like to progress in that way.

Now again I know they are many posts/questions about this topic, but I am particularly interested in the implementation I came up with.

• First, the issue with everything that is out there is that there are in fact, many different solutions provided to the problem, and most articles insist on the fact that there's no "definite" right way of implementing this pattern.
• As I have no experience, I have tried to learn how this is done by studying Chrome's source code. The code base is too complicated for someone of my level but I have been basing my observations on the very first version published so the complexity remains acceptable:

https://github.com/chromium/chromium/tree/3.0.195.25

I have been mostly looking at the ItemShelfDialog which is a view that implements a table view and a table model. In the case of this implementation, this is would be a list of URLs (and the table model would essentially be a list of URLs).

I did my best to re-implement as well as I could a simplified version of that code (see below).

I did a diagram to show (if I got it right) the relations between the components. Whereas (in Google's implementation) the colors represent the files in which the different classes would be implemented (e.g. class TableModel and class ToableModelObserver are defined in the same file)

https://github.com/chromium/chromium/blob/3.0.195.25/app/table_model.h

Here is my understanding of a possible "correct" implementation:

• The model is the central piece. It holds well... the model (the data).

• The view (TableView) observes the model (I am the table model observer).

• The "controller" (not named controller in this particular example, but I understand the class ShelfItemDialog acts as the controller here) observes the view (I am the table view observer).

• The ShelfItemDialog (the wrapper view) holds the model and the view as member variables (it is itself a view so that it can be placed into the UI framework). When it is created, it creates the model creates the view, set itself (the controller) as the view's observer, and set the view as the model's observer.

Use cases:

• At creation, the model is loaded (Reload()), which causes the view to be "signaled" by the model through OnModelChange(). The view can now display the list of items from the table (a list of files, in my case).
• The view receives events (double click) which causes the "controller" to be "signaled" through OnDoubleClick. The controller can then download the files (that was my original goal - design as an exercise, a program that downloads files). OnDoubleClick not implemented in the code below.

Questions:

• Would like any general comments about the code itself of course (disclaimer: it's not because I based this on Google's code - also rather old - that mine is good. I may have incorrectly interpreted the original author's intent).
• More importantly, I want to understand if I got the pattern right (in terms of how it should be coded) and, if not, what I need to fix.
• Since the View has access to the model, can it queries the data from the model to display a list of say files (like in my case) or should it maintain its own internal std::vector<std::pair<int, std::string>> where the int would be an index in the model's data, and string the name of the file as displayed in the view?
• If a user deletes an item from the file list (say delete key), the view will receive this event. But then should the view call the model to say, "hey remove item at index X from your model data" or should the view call the controller (RemoveItemByIndex(X))that in turn will call the model (RemoveItemByIndex(X)), which in turn will call the view (update your view, the model changed). Looking at Google's code:

void TableView::OnKeyDown(unsigned short virtual_keycode) {
  if (!ignore_listview_change_ && table_view_observer_) {
    table_view_observer_->OnKeyDown(virtual_keycode);
  }
}

It calls the observer (the controller), which would then process the key (which can be a del key). Is the latter then the correct way? (though in Google's impl, OnKeyDown() is not overwritten - but irrelevant - it might as well had been). In general, should every event received by the view always be sent to the controller and never directly to the model? I know stupid question, since it would break the MVC pattern in the first place, yet, I am curious to see if this would sometimes be ok.

The code:

#include <vector>
#include <memory>
#include <string>
#include <iostream>

// MODELS

class TableModelObserver { // table_model_observer
public:
   virtual void OnModelChanged() = 0;
   //virtual void OnItemsChanged(int start, int length) = 0;
   //virtual void OnItemsAdded(int start, int length) = 0;
   //virtual void OnItemsRemoved(int start, int length) = 0;
};

class TableModel { // table_model
public:
   struct Group {
       std::wstring title;
       int id;
   };
   
   typedef std::vector<Group> Groups;
   
   virtual void SetObserver(TableModelObserver* observer) = 0;
};

class PossibleURLModel : public TableModel {
public:
   PossibleURLModel() {        
   }

   void Reload() {
       // create some fake results
       struct SomeTableResultType {
           std::wstring filename;
           std::wstring title;
       };
       std::vector<SomeTableResultType> results = {{L"path/foo.bin", L"foo"}, {L"path/bar.bin", L"bar"}};  
       
       results_.resize(results.size());
       for (size_t i = 0; i < results.size(); ++i) {
           results_[i].filename = results[i].filename;
           results_[i].title = results[i].title;
           results_[i].index = i;
       }
       
       if (observer_)
           observer_->OnModelChanged();
   }

   virtual void SetObserver(TableModelObserver* observer) {
       observer_ = observer;
   }

   struct Result {
       std::wstring filename;
       std::wstring title;
       size_t index{0};
   };

   // Results we are showing
   std::vector<Result> results_;

   // Our observer
   TableModelObserver* observer_;
};

// VIEWS

class View {
   
};

class TableViewObserver {
public:
   //virtual void OnSelectionChanged() = 0;
   virtual void OnDoubleClick() = 0;
};

class TableView : public View, TableModelObserver {
public:
   TableView(TableModel* model) 
       : model_(model) {
       model_->SetObserver(this); // the view is observing the model. If the model changes, table view shall be notified
   }

   void SetObserver(TableViewObserver* observer) {
       table_view_observer_ = observer;
   }

   virtual void OnModelChanged() { std::cerr << "TableView::OnModelChanged()" << std::endl; }
   
   TableModel* model_;
   TableViewObserver* table_view_observer_{nullptr};
};

// CONTROLLER

class ShelfItemDialog : public View, TableViewObserver { // shelf_item_dialog
public:
   ShelfItemDialog() {
       url_table_model_.reset(new PossibleURLModel());
       
       url_table_ = new TableView(url_table_model_.get());
       url_table_->SetObserver(this);
   }
   ~ShelfItemDialog() = default;
   
   void Show() {
       url_table_model_->Reload();
   }
   
   void OnDoubleClick() {
       //int selection = url_table_->FirstSelectedRow();
       //if (selection >= 0 && selection < url_table_model_->RowCount()) {
       //  OnSelectionChanged();
       //  PerformModelChange();
       //}
   }
   
   TableView* url_table_;
   std::unique_ptr<PossibleURLModel> url_table_model_;
};

int main() {
   ShelfItemDialog a;
   a.Show();
   return 0;
}

Answer 1

Answers to your questions

• Would like any general comments about the code itself […]

See below.

• More importantly, I want to understand if I got the pattern right (in terms of how it should be coded) and, if not, what I need to fix.

As you already mentioned, there is no one right way to implement the pattern. It's just a guide to show you how to separate a system into a model, view and controller, so that you reduce coupling between those components, and make it easier to swap out say, a GUI view, with a command line view, without having to modify your model.

You already noticed that the controller is in the ShelfItemDialog(), which is also the view. This often happens in GUI code. However, as long as the controller's action doesn't directly change the view, but lets the view only update based on changes to the model, it is fine.

• Since the View has access to the model, can it queries the data from the model to display a list of say files (like in my case) or should it maintain its own internal std::vector<std::pair<int, std::string>> where the int would be an index in the model's data, and string the name of the file as displayed in the view?

It's perfectly fine to query the data from the model. This avoids double bookkeeping and all the issues associated with it. Of course it depends on the situation. If your model is stored on a slow tape drive, you might want your view to cache the data in RAM.

• If a user deletes an item from the file list (say delete key), the view will receive this event.

You will have registered a handler for delete key press events. That handler is part of the controller in the MVC pattern. Don't let the fact that your GUI library doesn't explicitly mark things as being "view" or "controller" confuse you.

Again, because the view and the controller are often closely related, you will implement them together. Sometimes that's just better than to force them to be completely separated in your code.

Overuse of classes

You don't need classes for everything in C++. There are some classes which are unnecessary in your code. For example, class View is used as a base class, but the base itself is never used. It can't be used anyway, since View is an empty class. I would just remove it entirely.

The Observer classes are have a use, but there are other ways to do this that don't rely on classes, and are a bit more flexible. You can use std::function<> to hold references to functions you can call. For example:

class TableModel {
public:
    …
    std::function<void()> onModelChanged;
};

class PossibleURLModel : public TableModel {
    …
    void Reload() {
        …
        if (onModelChanged)
            onModelChanged();
    }
};

class TableView {
public:
   TableView(TableModel* model) 
       : model_(model) {
       model_->onModelChanged = [&]{ OnModelChanged(); };
   }

   void OnModelChanged() {
       …
   }
   …
};

Note how there is no longer a need for a TableModelObserver class in the above code. Also, the function you register for onModelChanged can be anything; it doesn't have to be calling a member function of TableView, it could call a member function of another class or call a free function as well. This improves the decoupling between components (which is one of the reasons for having the model-view-controller pattern in the first place).

In most GUI libraries, you can register multiple callbacks for a single event, using signals and signal slots. A commonly used library to implement this is libsigc++.

Unnecessary heap allocations

Often you can avoid using new and delete in C++; containers and smart pointers will take care of that for you. But even better than using a smart pointer is to avoid needing any kind of pointer. In class ShelfItemDialog you allocate memory for a PossibleURLModel using std::unique_ptr, and for a TableModel using a raw pointer. The latter is a memory leak, since there is no corresponding delete. But you could just have stored these objects by value:

class ShelfItemDialog : TableViewObserver {
public:
    ShelfItemDialog() {
        url_table_.SetObserver(this);
    }
   
    void Show() {
        url_table_model_.Reload();
    }
    …   
    PossibleURLModel url_table_model_;
    TableView url_table_{&url_table_model_};
};