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:
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:
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).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).
neweverything required in the form's constructor, letting it act as a per-form bootstrapper.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.
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 the method that converts all of the
IInspectionsto an array that then gets iterated over each item is added to a list can just return anIEnumerableand you use the select method, defining the instantiation in the lambda. Then, if you must, you can cast to a list.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.
Your settings dependencies are way too broken up. You probably don't even need to have the 2 subclasses for settings at all. Just have one settings object with all of the properties. If you absolutely must have 2 separate classes, then your 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 dependencies settings 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.
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.
