Rolling my own Configuration with UI

Question

The Rubberduck Saga continues as I find a need to roll my own configuration. Since the program is really a *.dll and available to several host applications, using app.config is not an option. I decided to leverage XML serialization to allow a user to modify which comments get picked up in the Task List. I feel like everything started out really well and then went south when I built the UI. I tried to separate the concerns of displaying the tokens and the actual idea of a token, but failed miserably. I also have a public static class that I'm not real happy with. It smells like an anti-pattern to me.

First I created a root Configuration class.

using System;
using System.IO;
using System.Xml.Serialization;
using System.Runtime.InteropServices;

namespace Rubberduck.Config
{
    [ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    [XmlRootAttribute(Namespace = "", IsNullable = false)]
    public class Configuration
    {
        public UserSettings UserSettings { get; set; }

        public Configuration()
        {
            //default constructor required for serialization
        }

        public Configuration(UserSettings userSettings)
        {
            this.UserSettings = userSettings;
        }
    }
}

And UserSettings is similarly straight forward.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Xml.Serialization;
using System.Runtime.InteropServices;

namespace Rubberduck.Config
{
    [ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    public class UserSettings
    {
        public ToDoListSettings ToDoListSettings { get; set; }
        public CodeInspectionSettings CodeInspectinSettings { get; set; }

        public UserSettings()
        {
            //default constructor required for serialization
        }

        public UserSettings(ToDoListSettings todoSettings, CodeInspectionSettings codeInspectionSettings)
        {
            this.ToDoListSettings = todoSettings;
            this.CodeInspectinSettings = codeInspectionSettings;
        }
    }
}

As is TodoListSettings

using System.Xml.Serialization;
using System.Runtime.InteropServices;

namespace Rubberduck.Config
{
    interface IToDoListSettings
    {
        ToDoMarker[] ToDoMarkers { get; set; }
    }

    [ComVisible(false)]
    [XmlTypeAttribute(AnonymousType = true)]
    public class ToDoListSettings : IToDoListSettings
    {
        [XmlArrayItemAttribute("ToDoMarker", IsNullable = false)]
        public ToDoMarker[] ToDoMarkers { get; set; }

        public ToDoListSettings()
        {
            //empty constructor needed for serialization
        }

        public ToDoListSettings(ToDoMarker[] markers)
        {
            this.ToDoMarkers = markers;
        }
    }
}

And finally, the class that actually represents the string tokens, TodoMarker.

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;
        }
    }
}

I'm open to comments on these classes, but they're really just here for context. What I'm really curious about is my method of loading the Configuration into the UI for manipulation. I'm using a static ConfigurationLoader class to serialize the XML file into a Configuration object and then injecting that into a TodoModel.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Xml.Serialization;
using System.Runtime.InteropServices;
using System.IO;
using Rubberduck.Inspections;
using System.Reflection;
using Rubberduck.VBA.Parser.Grammar;

namespace Rubberduck.Config
{
    [ComVisible(false)]
    public static class ConfigurationLoader
    {
        private static string configFile = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) + @"\Rubberduck\rubberduck.config";

        /// <summary>   Saves a Configuration to Rubberduck.config XML file via Serialization.</summary>
        public static void SaveConfiguration<T>(T toSerialize)
        {
            XmlSerializer xmlSerializer = new XmlSerializer(toSerialize.GetType());
            using (TextWriter textWriter = new StreamWriter(configFile))
            {
                xmlSerializer.Serialize(textWriter, toSerialize);
            }
        }

        /// <summary>   Loads the configuration from Rubberduck.config xml file. </summary>
        /// <remarks> If an IOException occurs returns a default configuration.</remarks>
        public static Configuration LoadConfiguration()
        {
            try
            {
                using (StreamReader reader = new StreamReader(configFile))
                {
                    var deserializer = new XmlSerializer(typeof(Configuration));
                    return (Configuration)deserializer.Deserialize(reader);
                }
            }
            catch (IOException)
            {
                return GetDefaultConfiguration();
            }
        }

        public static Configuration GetDefaultConfiguration()
        {
            var userSettings = new UserSettings(
                                    new ToDoListSettings(GetDefaultTodoMarkers()),
                                    new CodeInspectionSettings(GetDefaultCodeInspections())
                               );

            return new Configuration(userSettings);
        }

        public static ToDoMarker[] GetDefaultTodoMarkers()
        {
            var note = new ToDoMarker("NOTE:", TodoPriority.Low);
            var todo = new ToDoMarker("TODO:", TodoPriority.Normal);
            var bug = new ToDoMarker("BUG:", TodoPriority.High);

            return new ToDoMarker[] { note, todo, bug };
        }

        // omitted some reflection methods for a different set of configs for brevity
    }
}

In TodoModel I originally injected just the markers, but it turns out that it needs to be able to serialize the object into XML. So, it now calls the public static ConfigurationLoader and I smell a smell.

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 TodoSettingModel
    {
        private BindingList<ToDoMarker> _markers;
        public BindingList<ToDoMarker> Markers { get { return _markers; } }

        public TodoSettingModel(List<ToDoMarker> markers)
        {
            _markers = new BindingList<ToDoMarker>(markers);
        }

        public void Save()
        {
            var settings = new ToDoListSettings(_markers.ToArray());
            var config = ConfigurationLoader.LoadConfiguration();
            config.UserSettings.ToDoListSettings = settings;

            ConfigurationLoader.SaveConfiguration<Configuration>(config);
        }
    }
}

Last, but not least. Here is the code behind of my TodoSettingsControl which gets embedded into a Form.

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 TodoListSettingsControl : UserControl
    {
        private TodoSettingModel _model;
        private IToDoMarker _activeMarker;

        /// <summary>   Parameterless Constructor is to enable design view only. DO NOT USE. </summary>
        public TodoListSettingsControl()
        {
            InitializeComponent();
        }

        public TodoListSettingsControl(TodoSettingModel model):this()
        {
            _model = model;
            this.tokenListBox.DataSource = _model.Markers;
            this.tokenListBox.SelectedIndex = 0;
            this.priorityComboBox.DataSource = Enum.GetValues(typeof(Config.TodoPriority));

            SetActiveMarker();
        }

        private void SetActiveMarker()
        {
            _activeMarker = (IToDoMarker)this.tokenListBox.SelectedItem;
            if (_activeMarker != null && this.priorityComboBox.Items.Count > 0)
            {
                this.priorityComboBox.SelectedIndex = (int)_activeMarker.Priority;
            }

            this.tokenTextBox.Text = _activeMarker.Text;
        }

        private void tokenListBox_SelectedIndexChanged(object sender, EventArgs e)
        {
            SetActiveMarker();
        }

        private void saveChangesButton_Click(object sender, EventArgs e)
        {
            var index = this.tokenListBox.SelectedIndex;
            _model.Markers[index].Text = tokenTextBox.Text;
            _model.Markers[index].Priority = (TodoPriority)priorityComboBox.SelectedIndex;
            _model.Save();
        }

        private void tokenTextBox_TextChanged(object sender, EventArgs e)
        {
            this.saveChangesButton.Enabled = true;
        }

        private void addButton_Click(object sender, EventArgs e)
        {
            var marker = new ToDoMarker(this.tokenTextBox.Text, (TodoPriority)this.priorityComboBox.SelectedIndex);
            _model.Markers.Add(marker);
            _model.Save();

            this.tokenListBox.DataSource = _model.Markers;
        }

        private void removeButton_Click(object sender, EventArgs e)
        {
            _model.Markers.RemoveAt(this.tokenListBox.SelectedIndex);
            _model.Save();

            this.tokenListBox.DataSource = _model.Markers;
        }

    }
}

Which all ends up looking like this.

Answer 1

I think a couple of Role interfaces would really help here. You need to do two things with your configuration

• load it
• save it

so:

public interface IConfigurationReader
{
    Configuration ReadConfiguration();
}

public interface IConfigurationWriter
{
    void WriteConfiguration<T>(T configuration);
}

Obviously those names could be improved. You can then create one class which implements both:

public class XmlConfigurationStore : IConfigurationReader, IConfigurationWriter
{
    // Implementation as before
}

You can also add in the stuff to get default values but not sure whether this makes sense on an interface or perhaps as part of a ConfigurationStore base class.

You'd want to compose these services in your control:

public partial class TodoListSettingsControl : UserControl
{
    private readonly IConfigurationReader configurationReader;
    private readonly IConfigurationWriter configurationWriter;

    public TodoListSettingsControl()
    {
        var configStore = new XmlConfigurationStore;
        configurationReader = configurationWriter = configStore;
        // Anything else.
    }
}

I really disagree with the view model saving itself - it reminds me of active record (which I've never really liked). It gets the job done but I think your code would work better if the TodoListSettingsModel class was just a wrapper around some data and you had another service to have the responsibility of saving the model. This service would almost certainly depend on both IConfigurationReader and IConfigurationWriter.

Answer 2

I generally don't like to add a second answer but the magnitude of what I'm gong to suggest is a lot greater than what I proposed previously, which doesn't appear to be enough.

I pulled the entire source off of GitHub and what's really missing from your question is how you are "injecting" these dependencies. Specifically, you are newing a lot of stuff up in your form's constructor and going from there. Then later on down the page, in a form event, there's this:

var markers = new List<ToDoMarker>(_config.UserSettings.ToDoListSettings.ToDoMarkers);
controlToActivate = new TodoListSettingsControl(new TodoSettingModel(markers));

Several things stick out to me:

1. You're not really injecting dependencies correctly if your model is getting wired up in different places around a form. If you want to inject dependencies, you have several choices to make based on how much of a purist you are and how intrusively you would like to make changes:

   1. Subscribe to the "pure dependency injection" approach and roll your own DI in Main(), attempting to resolve the forms themselves and pass dependencies into their constructor (not recommended).

   2. compromise by using a container as a service locator, register your object graph in the container and resolve your top-level dependency as early as possible in the form (or in the constructor).

   3. new everything required in the form's constructor, letting it act as a per-form bootstrapper.

2. Generally speaking, you should really only have a need to have a data structure as a dependency if the object you're passing it into is also a data structure. Doing so means you have to have a top level orchestrator acquire the data structure in a meaningful way and pass it in. The best way to do this is to pass the object that generates or contains the data structure directly in to the object. In this case, pass the settings directly into the model.

3. Why the fascination with arrays? If it's required for serialization then so be it, but there is a lot of conversion between types going on. For example there's a method that converts all of the IInspections to an array that then gets iterated over so that each item is used to instantiate a CodeInspection. You can change the method signature of the inner method to just return an IEnumerable and you can use .Select() to project that to a new collection, defining the instantiation in the lambda. Then, if you must, you can cast to a list.

4. On that note, why are you passing an interface into a concrete class that is ostensibly the same thing? I see the concrete class is used all the way up to the App class. Why not just make it easier on yourself by using the interface all the way up? The benefits of your interface are pretty much negated if you have to convert it to a concrete class as it goes up the layers.

5. Your settings dependencies are way too broken up. You probably don't even need to have three different classes, one class containing an instance of 2 other classes for settings. Just have one settings object with all of the properties. If you absolutely must have 2 separate dependency classes, then your topmost settings object should be composed of the properties of its dependencies and not directly expose its dependencies. You should map the properties of the 2 dependency objects onto the settings object's properties and have the consumer of the settings deal with those props. There is absolutely no need for the consumer to have to dig into an object hierarchy unless there were a hundred properties...which in that case should be organized with a separate layer of objects that compose the settings.

6. You mentioned you wanted to separate your concerns of displaying the tokens versus the idea of the tokens themselves. The model is a good abstraction. If you wanted to go further, you could take a page from the MVVM book and have the model act like a viewmodel. The model has a property that defines a delegate event that should fire when the markers collection changes. The form sets that when it instantiates the model to a delegate that encapsulates the logic that assigns the datasource of the control to the list of markers. Then either the model exposes the collection as readonly and has its own methods to modify the collection that fire the delegate, or you can leave your code the way it is and have your model's save method call the delegate. It may or may not be necessary to go this far. The added complexity does come with its benefits--you do have the ability to test more of the logic and the delegate is defined once, passed to the viewmodel and forgotten from there.

If that was too long, I can keep it simple: merge methods and objects back together because you have broken way too many out, tighten up your dependency graph and have it be wired up in one place, as close to the instantiation of the form as possible, and don't upcast/convert interfaces into concrete types as you get closer to the UI.

Answer 3

Well, I'm convinced the model is the right place for saving the configuration. I think your cause for concern comes from the static nature of the ConfigurationLoader class, which I looked over and didn't see any design problems with.

What led me to my recommendation was thinking about how to do unit testing. You have a tradeoff. On one hand, you can test the model much easier if you don't have to execute the ConfigurationLoader's SaveConfiguration every time by changing your code to inject an instance of it along with the markers in the model constructor and mocking the SaveConfiguration method. On the other hand, ConfigurationLoader ends up with behavior more indicative of an internal class and if you don't make internals visible to your test project you lose the granularity of being able to run tests directly on the methods.

My recommendation is to make the ConfigurationLoader an internal class and have your thing doing the injecting manage one instance of it for the lifetime of the application. Does that work for you?

Answer 4

NitPicking

• inconsistent use of var

• you shouldn't name a property like the type it represents

  public ToDoListSettings ToDoListSettings { get; set; }  
  

• you misspelled the property name

  public CodeInspectionSettings CodeInspectinSettings { get; set; }  
  

• for the input parameter ToDoListSettings todoSettings you are not consistent in the naming. For the class/type you say ToDo are two words, but for the parameter name you say it is one word.

ConfigurationLoader

• you should always use System.IO.Path.Combine() to combine a path/filename.

  private static string configFile = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) + @"\Rubberduck\rubberduck.config";  
  

• a SaveConfiguration() method inside an object named ConfigurationLoader is a big smell.

TodoSettingModel.Save()

• you first need to load before you can save. This smells for me.