Refactoring my Todo List was on my Todo List. How's my MVC?

Question

This is a follow up to Rolling my own Configuration with UI. I got a lot of great advice about the Configuration system I created, but no one touched on how all of my logic for the UI was in the code behind of the control. I decided that it was a bad idea, and took a stab at the Model-View-Controller pattern. Mostly, because I didn't like that none of that code was testable.

The code I would like to have reviewed is responsible for displaying a list of tokens to the user and allow the user to add/remove the tokens, as well as adjust their priority. (If you're curious, the tokens are used by another process as markers to be found in text searches.)

Did I get the pattern correct? Is this MVC? Are there any other code smells here?

---

My Form looks like this.

(The treeview is out of scope and stored in a different user control.) Red text gives the control names.

ITodoSettingsView:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Rubberduck.Config;
using System.ComponentModel;
using System.Windows.Forms;

namespace Rubberduck.UI.Settings
{
    public interface ITodoSettingsView
    {
        TodoPriority ActiveMarkerPriority { get; set; }
        string ActiveMarkerText { get; set; }
        BindingList<ToDoMarker> TodoMarkers { get; set; }
        int SelectedIndex { get; set; }
        bool SaveEnabled { get; set; }

        event EventHandler RemoveMarker;
        event EventHandler AddMarker;
        event EventHandler SaveMarker;
        event EventHandler SelectionChanged;
        event EventHandler TextChanged;
        event EventHandler PriorityChanged;

    }
}

TodoListSettingsUserControl (the view):

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Drawing;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using Rubberduck.Config;

namespace Rubberduck.UI.Settings
{
    public partial class TodoListSettingsUserControl : UserControl, ITodoSettingsView
    {
        /// <summary>   Parameterless Constructor is to enable design view only. DO NOT USE. </summary>
        public TodoListSettingsUserControl()
        {
            InitializeComponent();
        }

        public TodoListSettingsUserControl(List<ToDoMarker> markers)
            : this()
        {
            this.tokenListBox.DataSource = new BindingList<ToDoMarker>(markers);
            this.tokenListBox.SelectedIndex = 0;
            this.priorityComboBox.DataSource = Enum.GetValues(typeof(Config.TodoPriority));
        }

        public int SelectedIndex
        {
            get { return this.tokenListBox.SelectedIndex; }
            set { this.tokenListBox.SelectedIndex = value; }
        }

        public bool SaveEnabled
        {
            get { return this.saveChangesButton.Enabled; }
            set { this.saveChangesButton.Enabled = value; }
        }

        public TodoPriority ActiveMarkerPriority
        {
            get { return (TodoPriority)this.priorityComboBox.SelectedIndex; }
            set { this.priorityComboBox.SelectedIndex = (int)value; }
        }

        public string ActiveMarkerText 
        {
            get { return this.tokenTextBox.Text; }
            set { this.tokenTextBox.Text = value; }
        }

        public BindingList<ToDoMarker> TodoMarkers
        {
            get { return (BindingList<ToDoMarker>)this.tokenListBox.DataSource; }
            set { this.tokenListBox.DataSource = value; }
        }

        public event EventHandler SelectionChanged;
        private void tokenListBox_SelectedIndexChanged(object sender, EventArgs e)
        {
            RaiseEvent(this, e, SelectionChanged);
        }

        public event EventHandler SaveMarker;
        private void saveChangesButton_Click(object sender, EventArgs e)
        {
            RaiseEvent(this, e, SaveMarker);
        }

        public event EventHandler TextChanged;
        private void tokenTextBox_TextChanged(object sender, EventArgs e)
        {
            RaiseEvent(this, e, TextChanged);
        }

        public event EventHandler PriorityChanged;
        private void priorityComboBox_SelectedIndexChanged(object sender, EventArgs e)
        {
            RaiseEvent(this, e, PriorityChanged);
        }

        public event EventHandler AddMarker;
        private void addButton_Click(object sender, EventArgs e)
        {
            RaiseEvent(this, e, AddMarker);
        }

        public event EventHandler RemoveMarker;
        private void removeButton_Click(object sender, EventArgs e)
        {
            RaiseEvent(this, e, RemoveMarker);
        }

        private void RaiseEvent(object sender, EventArgs e, EventHandler handler)
        {
            if (handler != null)
            {
                handler(this, e);
            }
        }
    }
}

TodoSettingsController:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Rubberduck.Config;
using System.ComponentModel;

namespace Rubberduck.UI.Settings
{
    public class TodoSettingController
    {
        private ITodoSettingsView _view;

        public ToDoMarker ActiveMarker
        {
            get { return _view.TodoMarkers[_view.SelectedIndex]; }
        }

        public TodoSettingController(ITodoSettingsView view)
        {
            _view = view;

            if (_view.TodoMarkers != null)
            {
                _view.ActiveMarkerText = _view.TodoMarkers[0].Text;
                _view.ActiveMarkerPriority = _view.TodoMarkers[0].Priority;
            }

            _view.SelectionChanged += SelectionChanged;
            _view.TextChanged += TextChanged;
            _view.AddMarker += AddMarker;
            _view.RemoveMarker += RemoveMarker;
            _view.SaveMarker += SaveMarker;
            _view.PriorityChanged += PriorityChanged;
        }

        private void SaveMarker(object sender, EventArgs e)
        {
            //todo: add test; How? I can't click the save button. Code smell here.
            var index = _view.SelectedIndex;
            _view.TodoMarkers[index].Text = _view.ActiveMarkerText;
            _view.TodoMarkers[index].Priority = _view.ActiveMarkerPriority;
        }

        private void RemoveMarker(object sender, EventArgs e)
        {
            _view.TodoMarkers.RemoveAt(_view.SelectedIndex);
        }

        private void AddMarker(object sender, EventArgs e)
        {
            var marker = new ToDoMarker(_view.ActiveMarkerText, _view.ActiveMarkerPriority);
            _view.TodoMarkers.Add(marker);

            _view.SelectedIndex = _view.TodoMarkers.Count - 1;
        }

        private void TextChanged(object sender, EventArgs e)
        {
            _view.SaveEnabled = true;
        }

        private void PriorityChanged(object sender, EventArgs e)
        {
            _view.SaveEnabled = true;
        }

        private void SelectionChanged(object sender, EventArgs e)
        {
            _view.ActiveMarkerPriority = this.ActiveMarker.Priority;
            _view.ActiveMarkerText = this.ActiveMarker.Text;

            _view.SaveEnabled = false;
        }

        public void SetActiveItem(int index)
        {
            _view.SelectedIndex = index;
        }
    }
}

TodoMarkers.cs:

using System.Xml.Serialization;
using System.Runtime.InteropServices;
using Rubberduck.VBA.Parser;

namespace Rubberduck.Config
{
    [ComVisible(false)]
    public enum TodoPriority
    {
        Low, 
        Normal,
        High
    }

    [ComVisible(false)]
    public interface IToDoMarker
    {
        TodoPriority Priority { get; set; }
        string Text { get; set; }
    }

    [ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    public class ToDoMarker : IToDoMarker
    {
        //either the code can be properly case, or the XML can be, but the xml attributes must here *exactly* match the xml
        [XmlAttribute]
        public string Text { get; set; }

        [XmlAttribute]
        public TodoPriority Priority { get; set; }

        /// <summary>   Default constructor is required for serialization. DO NOT USE. </summary>
        public ToDoMarker()
        {
            // default constructor required for serialization
        }

        public ToDoMarker(string text, TodoPriority priority)
        {
            Text = text;
            Priority = priority;
        }

        /// <summary>   Convert this object into a string representation. Over-riden for easy databinding.</summary>
        /// <returns>   The Text property. </returns>
        public override string ToString()
        {
            return this.Text;
        }
    }
}

Here's an example call:

this.config = this.configService.LoadConfiguration();

var markers = this.config.UserSettings.ToDoListSettings.ToDoMarkers.ToList();

this.todoView = new TodoListSettingsUserControl(markers);
this.todoController = new TodoSettingController(this.todoView);

and everything from there is pretty much automagicly handled by the controller responding to view events except for this method to save the markers back to the Config file.

private void SaveConfig()
{
    this.config.UserSettings.ToDoListSettings.ToDoMarkers = this.todoView.TodoMarkers.ToArray();
    this.configService.SaveConfiguration<Configuration>(_config);
}

Unit Tests are available if requested, but this post was already quite long.

Answer 1

MVC

Quoting from the provided wikipedia link

Interactions In addition to dividing the application into three kinds of components, the model–view–controller design defines the interactions between them.

• A controller can send commands to the model to update the model's state (e.g., editing a document). It can also send commands to its associated view to change the view's presentation of the model (e.g., by scrolling through a document).

• A model notifies its associated views and controllers when there has been a change in its state. This notification allows the views to produce updated output, and the controllers to change the available set of commands. In some cases an MVC implementation might instead be "passive," so that other components must poll the model for updates rather than being notified.

• A view requests information from the model that it uses to generate an output representation to the user.

• basically you don't have MVC because you don't have a model.
• your view has events to which the controller subscribes. This could be the other way around but the view should subscribe to the model events.

Your view should react on state changes triggered by the controller and/or the model.

How to do MVC

The image on wikipedia next to "Interactions" shows how it should be done.

The user interacts by clicking on a button or selecting some item in a drop down etc. with the controller. The controller then changes the state of the model which can trigger an update of the view usually by raising an event to which the view had subscribed.

So let us start with the model from the controller perspective

public interface IModel
{
   bool Update(IToDoMarker marker);
   bool Add(string text, TodoPriority priority);
   bool Remove(IToDoMarker marker);
   bool RequestMarkerList();
   IToDoMarker GetMarker(int index);
}

the controller will also need methods to be called by the view and e.g an event stating an error.

public interface IController
{
   void Update(int index, TodoPriority priority);
   void Update(int index, string text);
   void Update(int index, string text, TodoPriority priority);
   void Add(string text, TodoPriority priority);
   void Remove(int index);
   void RequestMarkerList();
   event EventHandler<FailedCallEventArgs> FailedCall;
}

and the model from the view perspective

public interface IViewModel
{
    event EventHandler<MarkerListUpdatedEventArgs> MarkerListUpdated;
    event EventHandler<MarkerUpdatedEventArgs> MarkerUpdated;
    event EventHandler<MarkerAddedEventArgs> MarkerAdded;
    event EventHandler<MarkerRemovedEventArgs> MarkerRemoved;
}

and the combined IToDoModel

public interface IToDoModel : IModel, IViewModel 
{ }

Partial implemented IController

public class ToDoController : IController
{
    private IModel _model;
    public ToDoController(IModel model)
    {
        _model = model;
    }

    public void Update(int index, TodoPriority priority)
    {
        IToDoMarker marker = GetMarker(index);
        if (marker == null) { OnFailedCall("Marker with given index not found"); }

        marker.Priority = priority;

        if (_model.Update(marker)) { return; }

        OnFailedCall("Update failed");
    }

    private IToDoMarker GetMarker(int index)
    {
        return _model.GetMarker(index);
    }

    private void OnFailedCall(String reason)
    {
        // create a FailedCallEventArgs object
        // and call the overloaded OnFailCall(FailedCallEventArgs e)
    }

    protected void OnFailCall(FailedCallEventArgs e)
    {
        // raise the FailedCall event
    }
    public event EventHandler<FailedCallEventArgs> FailedCall;
}

Partial implemented IToDoModel

public class ToDoModel : IToDoModel
{
    public bool Update(IToDoMarker marker)
    {
        bool updateSuccess = UpdateTheMarker(marker);
        if (updateSuccess)
        {
            // raise the MarkerUpdated event
        }
        return updateSuccess;
    }

    private bool UpdateTheMarker(IToDoMarker marker)
    {
        return true;
    }

    public event EventHandler<MarkerUpdatedEventArgs> MarkerUpdated;

}

You would then add a constructor to your usercontrol which takes an IController and an IViewModel as parameter.

public TodoListSettingsUserControl(IController controller, IViewModel model)
    : this()
{
    // subscribe to the model events and store in a variable
    // subscribe to the controller events and store in a variable
}  

and call the whole thing like

IToDoModell model = new ToDoModel();
IController controller = new ToDoController((IModel)model);  
TodoListSettingsUserControl ctrl= new TodoListSettingsUserControl(controller, (IViewModel)model);