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

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 con

• 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.

• Have StepStage implement IEquatable. Now the StepStageCollection can easily Add, Contains, etc.

• 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.

• ImageRulesSet is the client code? Then it has no business merging rule sets.

• Can "Crete", "Update", "Delete" be an enum?

• 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

• Default/empty constructions of all classes.

• Optional and default method parameters.

• Implementing IEquatable is key to making StepStage behave like a key. We can ensure uniqueness in a collection.

• Lots of encapsulation with Key objects internally referencing it's "value objects" and vice versa. No more exposing "core" dictionary stuff to clients.

• Put all the construction grunt work into factories (not shown). I'm imagining an Abstract Factory because of the complexity I'm seeing.

• Make sub-factories. Can use them separately for testing, and composite them in an Abstract Factory.

• 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 {
    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 {

     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 {
    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 {
  // msdn says one should inherit Collection. 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);
        }
    }
}

StackExchange Code Review Q#23196, answer score: 2