13
\$\begingroup\$

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.

Rubberduck Todo List Settings Dialog

\$\endgroup\$
  • \$\begingroup\$ Note to self: I shouldn't necessarily be modeling ToDoMarkers, this form is concerned with Configuration \$\endgroup\$ – RubberDuck Jan 2 '15 at 10:56
4
+150
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I like this answer because it's much more to the point than mine and it hits the nail on the head when breaking it down into load/save responsibilities. I am a bit concerned about breaking it out of the model though if the goal is DI. Having the form act as the controller doesn't get you as tight of a dependency graph and logic that is moved out into form events becomes harder to test. \$\endgroup\$ – moarboilerplate Jan 2 '15 at 15:09
  • \$\begingroup\$ @moarboilerplate - I wasn't initially sure how the control was being instantiated so just newed the service up in the constructor. There are certainly better options, as you say in your second answer. \$\endgroup\$ – RobH Jan 2 '15 at 15:16
  • \$\begingroup\$ Awarding the bounty to this answer because it sent me down the path I ended up using. I opted not to go full bore reader/writer interfaces (it seemed like overkill), but I am now injecting an IConfigurationService (which isn't static) into the model which has made things a bit cleaner and more testable. \$\endgroup\$ – RubberDuck Jan 5 '15 at 12:14
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your time. I've read this several times, but I think, to really grasp it, I need to sit down with your answer and the code at the same time. I'll do that soon. \$\endgroup\$ – RubberDuck Jan 2 '15 at 1:56
  • 1
    \$\begingroup\$ Totally understand. There's a lot there, and it's quite dense. Sorry I couldn't provide more code examples but it's just that there are some key architectural points at play here and depending on how you decide to organize the app I could write code examples for the exact same thing that looked completely different. Trying to make my answer as terse as possible, I guess I could sum it up as "make your settings objects all one object and don't pass lists in constructors, pass the objects that hold those lists in to the constructors" \$\endgroup\$ – moarboilerplate Jan 2 '15 at 2:15
  • \$\begingroup\$ Also don't instantiate objects anywhere but constructors if you can \$\endgroup\$ – moarboilerplate Jan 2 '15 at 2:17
3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I was thinking the same thing. Thanks for kind of confirming that for me. I have a question though. What do you feel the advantage is in making ConfigurationLoader internal? \$\endgroup\$ – RubberDuck Dec 24 '14 at 17:56
  • 1
    \$\begingroup\$ If it was a standalone app, it probably wouldn't matter, but since it's a dll, you want to consider what you want to expose to a consuming app. The reason for an app to consume ConfigurationLoader would be because you would be implementing the same configuration scheme in that app, which it doesn't seem you'd be in any hurry to do. \$\endgroup\$ – moarboilerplate Dec 24 '14 at 19:20
  • \$\begingroup\$ That's a good point, and I'll keep it in mind when/if I'm looking to build a library. This is a *.dll just because that's how COM Add-Ins work though. Thanks again @moarboilerplate. \$\endgroup\$ – RubberDuck Dec 24 '14 at 19:24
3
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Do you think it would just be better to pass the model an entire config so it's not necessary to load before saving? Or would it make more sense to give the config object save and load methods? \$\endgroup\$ – RubberDuck Jan 2 '15 at 10:53
  • \$\begingroup\$ I would either pass a Configuration to the constructor or load the Configuration inside the constructor of the model and save it as reference. Then I could apply changes to the configuration and if I want to save I would save the changes. \$\endgroup\$ – Heslacher Jan 2 '15 at 10:59
  • \$\begingroup\$ Accepting this answer as it gave the most objective and technically correct advice. \$\endgroup\$ – RubberDuck Jan 5 '15 at 12:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.