0

I've come across something that I find decently frustrating while adding new functionality to our large existing code base.

Preface

We have a variety of classes (ItemA, ItemB, ItemC...) that inherit from a base class (TheBaseClass). We'll say there are around 10 Item classes. We hold a collection of mixed Item instances on the magnitude of a few hundred to a few thousand.

List<TheBaseClass> items = new List<TheBaseClass>()

This list is the output of a previous step that assembles the items based on some input data, usually some factory pattern stuff going on to get the Item. We then take this list and pass through a series of processing and calculation steps. Some of these methods modify values on existing items, others modify the collection itself (usually by inserting new items under a variety of conditions, this can include look-aheads and behinds while iterating). The steps sometimes just act on TheBaseClass, but many iterate through singling out a specific Item implementations, or act on different Item's in different ways.

Heres an example of a process step that acts on all types by using a dictionary with function pointers for each type:

public class ProcessingStep1()
{
    Dictionary<Type, CalculateDelegate> lookupTable = new Dictionary<Type, CalculateDelegate>();

    public ProcessingStep1()
    {
        lookupTable.Add(typeof(ItemA), ProcessItemA);
        lookupTable.Add(typeof(ItemB), ProcessItemB);
        // ...
    }

    public method CalculateAThing(IEnumerable<TheBaseClass> items)
    {
        foreach (TheBaseClass item in items)
        {
            // desired to throw an exception if the key is not found
            CalculateDelegate calculate = lookupTable[item.GetType()]; 
        }
    }

    private ProcessItemA(TheBaseClass item)
    {
        // need to cast to access a property from the derived type
        var something = (item as ItemA).PropertyOnItemA;
        item.BaseClassProperty = doMath(something);
    }

    private ProcessItemB(TheBaseClass item)
    {
        // no need to cast, but the behavior is different for ItemB than others
        item.BaseClassProperty = somethingSpecific;

    }

    // ...
}

Separating Code from Data

These processing steps vary based on settings and inputs into the super process, in addition to a few defined "routes" of steps to take, the steps themselves can be implementations of an interface and may vary individually.

This, along with the fact that the processing steps can modify the collection itself, is a strong argument to keep these methods as separate entities rather than members of the Item and TheBaseClass types, like this

public abstract class TheBaseClass
{
    public abstract void CalculateAThing();
}

Struggles

I've now had to add a new Item, ItemD. If ItemD had to implement an interface or abstract methods, I would get compiled time checks of the expected behavior that an Item is required to have. However here I need to rely on either my previous knowledge of our code base (if I have it) or a continuous development loop of trying to get a collection with ItemD's present through all of the possible branches that ProcessingSteps can take, relying on exceptions (from the dictionary above) or me catching and identifying runtime bugs of the output.

I'm locked into this architecture and code base, and I won't really identify my other limitations because I think it will limit the discussion, but I'm looking for how this is managed in other domains or even in functional languages so that I could avoid this in the future if a new design was heading in the same direction.

3

It appears to me that you have re-invented the wheel. Your list of delegates replaces the VMT (badly).

The proper OO way to do this is to store the "delegate" in the Virtual Method Table (VMT), which is done for you automatically when you use polymorphism, if you have set things up properly. Any class-specific behavior is decided via calls to virtual methods.

abstract class TheBaseClass
{
    public abstract void CalculateAThing();
}

class ItemA : TheBaseClass
{
    public int PropertyOnItemA { get; set; }

    public override void CalculateAThing()
    {
        this.BaseClassProperty = doMath(this.PropertyOnItemA);  //No casting needed because type is known
    }
}

class ItemB : TheBaseClass
{
    public override void CalculateAThing()
    {
        this.BaseClassProperty = somethingSpecific;
    }
}


public class ProcessingStep1()
{
    //Got rid of the list of delegates and the ctor that builds it
    //Any class-specific logic is held in the class itself, i.e. is encapsulated

    public method CalculateAThing(IEnumerable<TheBaseClass> items)
    {
        foreach (TheBaseClass item in items)
        {
            item.CalculateAThing();
        }
    }
}

When you need to add an ItemD or ItemE, you just need to override CalculateAThing and implement the item-specific logic as part of the class to which it applies. This couples the behavior with the data, and the type-specific behavior and data with the type, which is the whole idea behind encapsulation and polymorphism.

If you insist on keeping the code outside of TheBaseClass and in your ProcessingStep1 class, you can still keep the decision-making in TheBaseClass and the implementation in ProcessingStep1 by using a callback, like this:

abstract class TheBaseClass
{
    abstract public void DoTheProcessing(ProcessingStep1 processor);
}

class ItemA : TheBaseClass
{
    public override void DoTheProcessing(ProcessingStep1 processor)
    {
        processor.ProcessItemA(this);
    }
}

class ItemB : TheBaseClass
{
    public override void DoTheProcessing(ProcessingStep1 processor)
    {
        processor.ProcessItemB(this);
    }
}

Which will allow your type-specific methods to accept specific instances:

public method CalculateAThing(IEnumerable<TheBaseClass> items)
{
    foreach (TheBaseClass item in items)
    {
        item.DoTheProcessing(this);
    }
}

private ProcessItemA(ItemA item)
{
    item.BaseClassProperty = doMath(item.PropertyOnItemA);  //No cast needed
}

private ProcessItemB(ItemB item)
{
    item.BaseClassProperty = somethingSpecific;
}

Once you avoid that delegate list and all those unnecessary casts, the possibility of "missing" some methods goes way down, and can be detected at compile time.

  • Since the OP said that "the processing steps can modify the collection itself", the DoTheProcessing method should have a parameter for the collection also (or the processor may facilitate that). – Bernhard Hiller Jul 27 '17 at 9:34
0

Just to build on @John Wu's answer, if you have custom behaviors, each item should implement them via an interface.

Example:

class ItemA : TheBaseClass, ISpecialBehavior
{
    public override void DoTheProcessing(ProcessingStep1 processor)
    {
        processor.ProcessItemA(this);
    }

    public void ExexcuteSpecialBehavior()
    {
        //Implement Here
    }
}

Then in the ProcessingStep1 class, as one iterates through each item, check to see if that item implements the custom interface(s) and then call the custom implementation. Then each item defines it's own custom behavior. There is some linkage in the ProcessingStep1 class to know which custom behaviors to run that belong to that processing step via the interface, but it's responsibility to do know how to do the processing so that is to be expected.

  • Why not just do the special behavior as part of DoTheProcessing ? – John Wu Jul 26 '17 at 18:52
  • @JohnWu - Agreed, one could do that as well if that behavior was specialized to one specific item. I was thinking more along the lines of a defined custom behavior that each item instance could implement if needed. – Jon Raynor Jul 27 '17 at 13:39

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.