Doubts about the quality of an API designed for use with minimal effort

Question

I'll try to provide exactly the amount of context that is necessary to understand the construct.

I built an API (well, a small part of one) that works and is kind of usable, but rather unpretty and badly testable, and most of all unpretty. Testability is also an issue.

The problem domain has the concept of an 'image' in the sense of a logical image of a database record. This image can be extracted from an execution context passed into the logic from the calling application, based on certain rules that need to have defaults but also be configurable on different levels.

The goal is to be able to just take a dependency in an interface called IFullImageSource anywhere in the application (I am using an IoC container) and call its parameterless GetImage() method, having the implementation handle everything else.

The IFullImageSource implementation:

public class SelectingFullImageSource : IFullImageSource
{
    private readonly IImageRulesSet _rules;
    private readonly IPluginExecutionContext _context;

    public SelectingFullImageSource(IImageRulesSet rules, IPluginExecutionContext context)
    {
        this._rules = rules;
        this._context = context;
    }

    public Entity GetImage()
    {
        StepStage stage = this._context.GetStepStage();
        IImageRule rule = this._rules.GetRule(stage);
        return rule.GetImage(this._context);
    }
}

Everything needed to determine how to extract the image is contained within this part of the object graph; the rest of the application does not need to care at all.

Now let's go to the other end and start with the small parts.

IImageRulesSet encapsulates a collection of IImageRule objects keyed by StepStage values (StepStage has overridden equality members); I will show that in a moment. The smallest components here are the IImageRule implementations. They generally look like this:

public class TargetImageRule : IImageRule
{
    public Entity GetImage(IPluginExecutionContext context)
    {
        return ((Entity)context.InputParameters["Target"]).Clone();
    }
}

or

public class PreImageRule : IImageRule
{
    private readonly string _imageName;

    public PreImageRule() { }

    public PreImageRule(string imageName)
    {
        this._imageName = imageName;
    }

    public Entity GetImage(IPluginExecutionContext context)
    {
        string imageName = this._imageName ?? context.PrimaryEntityName;
        return context.PreEntityImages[imageName];
    }
}

These don't include the StepStage part because while that is determined solely from the context as well, the same IImageRule implementation can apply to several StepStage values.

The default rules set is provided as a dictionary of StepStage and IImageRule:

public class DefaultImageRules : Dictionary<StepStage, IImageRule>, IDefaultImageRules
{
    public DefaultImageRules() : this(null, null) { }

    public DefaultImageRules(string preImageName, string postImageName)
        : base(new Dictionary<StepStage, IImageRule>()
               {
                   {new StepStage("Create", StepStage.PreStage), new TargetImageRule()},
                   {new StepStage("Create", StepStage.PostStage), new TargetImageRule()},
                   {new StepStage("Update", StepStage.PreStage), new MergeImageRule(new PreImageRule(preImageName), new TargetImageRule())},
                   {new StepStage("Update", StepStage.PostStage), new PostImageRule(postImageName)},
                   {new StepStage("Delete", StepStage.PreStage),  new PreImageRule(preImageName)},
                   {new StepStage("Delete", StepStage.PostStage), new PreImageRule(preImageName)},
               }) { }
}

This is where the unprettiness starts. The constructor "fallthrough" with null parameters (of all things!) seems rather sloppy; the same goes for the rule implementations - although local defaults are always available.

That means the following: Simply creating a DefaultImageRules object will pass null for all name parameters, but through

container.Register<IDefaultImageRules>(new DefaultImageRules("preImage", null));

the developer can rename one of the images while the rule structure itself stays in place.

(Note: The IDefaultImageRules interface is only an empty marker interface deriving from IDictionary<StepStage, IImageRule> for IoC and mockability; I can probably do away with this and use a derived class for unit tests that has

this.Clear();

in its constructor before setting it up for the test again.)

This is also not testable very well. I can easily check whether the correct IImageRule implementation is returned for a StepStage value, but I can only test proper passing of the image names by creating an IPluginExecutionContext stub and setting it up with the expected image, then passing it into the IImageRule instance returned by the dictionary. This is actually testing two things at once - but then again that might not be an issue because the IImageRule implementations are individually tested anyway and "guaranteed" to work correctly.

Still, the whole thing seems a bit flaky.

This is being used in the following class that fulfills the IImageRulesSet dependency of the very first one I showed (yes, all those names are also still an issue):

public class ImageRulesSet : IImageRulesSet
{
    private readonly IDictionary<StepStage, IImageRule> _rules;
    private readonly IDictionary<StepStage, IImageRule> _customRules;
    private readonly IMergeImageRulesDictionaries _mergeImageRulesDictionaries;

    public ImageRulesSet(IDefaultImageRules rules, ICustomImageRules customRules, IMergeImageRuleDictionaries mergeImageRulesDictionaries)
    {
        this._rules = rules;
        this._customRules = customRules;
        this._mergeImageRulesDictionaries = mergeImageRulesDictionaries;
    }

    private bool _rulesHaveBeenMerged = false;

    private IDictionary<StepStage, IImageRule> Rules
    {
        get
        {
            if (!this._rulesHaveBeenMerged)
            {
                this._mergeImageRulesDictionaries.Merge(this._rules, this._customRules);
                this._rulesHaveBeenMerged = true;
            }

            return this._rules;
        }
    }

    public IImageRule GetRule(StepStage stepStage)
    {
        return this.Rules[stepStage];
    }
}

The ICustomImageRules/CustomImageRules combination is exactly the same as the DefaultImageRules thing, except that the dictionary is empty by default. The IMergeRuleDictionaries simply merges both, giving precedence to the 'custom' entries for identical StepStage values to allow for overriding default rules in addition to adding new ones.

That is about it; the object graph in total looks like this:

SelectingFullImageSource : IFullImageSource
    ImageRulesSet : IImageRulesSet
        DefaultImageRules : IDefaultImageRules
            IImageRule implementations
        CustomImageRules : ICustomImageRules
            IImageRule implementations
        DefaultMergeRulesDictionaries : IMergeRulesDictionaries
    IPluginExecutionContext (from calling application)

The configuration effort is the minimum possible, I think - registering instances of IDefaultImageRules and ICustomImageRules in the IoC container; the values for the custom rules can be written as a collection initializer for the dictionary.

What I'm looking for are opinions on the quality of this solution with respect to the SOLID principles and testability, as I currently think it is less than ideal, as well as (first and foremost) a more elegant solution. The only things I really need to retain are the outermost interface (IFullImageSource) and the degree of simplicity in configuring the rules - the exact way of doing that can be entirely different.

Oh, and suggestions about naming are also always welcome.

Answer 1

1. Why can't a StepStage contain a collection of IImageRule? Then there can be a collection of StepStage. This provides an abstraction level that I think is missing.
2. Have StepStage implement IEquatable. Now the StepStageCollection can easily Add, Contains, etc.
3. this._mergeImageRulesDictionaries.Merge(this._rules, this._customRules) Seems to me there could be duplicates in these two sets and so rules could get dropped. This "client" method has to know internal stuff to merge what it's supposed to be using.
4. ImageRulesSet is the client code? Then it has no business merging rule sets.
5. Can "Crete", "Update", "Delete" be an enum?
6. Seems to me the client could/should be a template method(s) that pulls rules from the collections in the desired execution order. Therefore you must build up the abstractions more than you have now.

Structure and Encapsulating Functionality

1. Default/empty constructions of all classes.
2. Optional and default method parameters.
3. Implementing IEquatable is key to making StepStage behave like a key. We can ensure uniqueness in a collection.
4. Lots of encapsulation with Key objects internally referencing it's "value objects" and vice versa. No more exposing "core" dictionary stuff to clients.
5. Put all the construction grunt work into factories (not shown). I'm imagining an Abstract Factory because of the complexity I'm seeing.
6. Make sub-factories. Can use them separately for testing, and composite them in an Abstract Factory.
7. More Testable due to all of the above. Importantly, decoupled from other stuff by the Factory. Looks like SelectingFullImageSource will have a factory and will pass in that context thing. The factory will have separate/independent references for the stuff below to build whatever.

public enum CRUD {Undefined, Create, Update, Delete}
public enum Stage {Undefined, pre, post}

public class StepStage : IEquatable<StepStage> {
    protected CRUD Step {get; set;}
    protected Stage Stage {get; set;}
    protected ImageRuleCollection Images {get; set}

    public StepStage(CRUD step, Stage stage, ImageRuleCollection imageRules = null) {
        this.Step = step;
        this.Stage = stage;

        ImageRules = imageRules ?? new ImageRuleCollection(this);
    }

    // default constructor
    public StepStage() {
        this.Step = CRUD.Undefined;
        this.Stage = Stage.Undefined;
    }

    public bool Equals(StepStage other) {
        return (this.Step == other.Step && this.Stage == other.Stage);
    }

    // MUST ALSO OVERRIDE OBJECT.EQUALS & GETHASHCODE
    public override bool Equals(object obj) {
        // cast obj and call the IEquatable.Equals(). easy peasy
    }
 }

 public class StepStageCollection : List<StepStage> {

     public override bool Add(StepStage anotherSS) {
         if (!this.Contains(anotherSS)) {  // no duplicate "keys"
             Add(anotherSS);
             return true;
         }

         return false;
     }

     // implement GetKeys(), GetValues() if needed
 }

public abstract class ImageRule : IEquatable<ImageRule> {
    protected StepStage myKey;

    // put the original IImageRule stuff in here.
    // I think we don't need an "I"interface. This abstract class
    // will do the job.

    public Image (StepStage theKey = null) {
        myKey = theKey ?? new StepStage;
    }

    public bool Equals(Image other) {
        return myKey.Equals(other.myKey);
    }

    public void SetKey(StepStage newKey){
        if(newKey == null) // throw exception
        myKey = newKey;
    }
}

public class ImageRuleCollection : List<ImageRule> {
  // msdn says one should inherit Collection<T>. whatever.

    protected StepStage ourKey {get; set;}

    public ImageRuleCollection (StepStage theKey = null, params[] ImageRule imageRules = null) {
         ourkey = theKey ?? new StepStage();

         if (imageRules != null) {
             foreach(var imageRule in imageRules) {
                 imageRule.SetKey(ourKey);
                 Add(imageRule);
             }
          }
    }

    public void SetKey(StepStage newKey) {
        if(newKey == null) // throw exception

        ourKey = newKey;
        foreach(ImageRule imageRule in this) {
           imageRule.SetKey(ourKey);
        }
    }
}