-1

I'll try word this as succinctly as possible.

I currently have an object called a StoresAdaptor which inherits from a base class ERPAdaptor, that takes a Stores object and pushes it into a 3rd party ERP system via an adaptor design pattern.

Now, there are multiple types of these Stores movements. Each has their own implementation, but the creation is nigh on identical. The issue arises from the fact that this Stores object has the "To" location, and the Line object has the "From" location - however, when making a Movement, we need to know both the "To" and "From".

public class StoresAdaptor : ERPAdaptor
{
    protected Stores storesRequest
    {
        get { return (Stores) this.GenericObject; }
    }
    public StoresAdaptor(Data_Controller DCO, GenericObject storesRequest, ERPConnection conn)
        : base(DCO, storesRequest, conn)
    { }
    private class ReturnMiscMaterialAdaptor
    {
        //This here feels wrong...
        private StoresAdaptor _storesAdaptor;
        private Stores.Line _storesLine;

        public ReturnMiscMaterialAdaptor(StoresAdaptor storesAdaptorRef, Stores.Line storesLine)
        {
            _storesAdaptor= storesAdaptorRef;
            _storesLine= storesLine;
        }
     // task to bay
        public Update()
        {
            ERPSystem.ToLocation = _storesAdaptor.storesRequest.ToLocation;
            ERPSystem.FromLocation = _storesLine.FromLocation;
            //Return specific functionality
        }
    }
    private class IssueMiscMaterialAdaptor
    {
        //As well as here... there has to be a better way to do this...
        private StoresAdaptor _storesAdaptor;
        private Stores.Line _storesLine;

        public IssueMiscMaterialAdaptor(StoresAdaptor storesAdaptorRef, Stores.Line storesLine)
        {
            _storesAdaptor= storesAdaptorRef;
            _storesLine= storesLine;
        }
        public Update()
        {
            ERPSystem.ToLocation = _storesAdaptor.storesRequest.ToLocation;
            ERPSystem.FromLocation = _storesLine.FromLocation;
            //Issue specific functionality
        }
        // bay to task
    }

In essence, we need to keep the header object (Stores) exposed to all the sub-classes, whilst passing in the line (Line) to the separate sub-classes to be actioned appropriately.

What I've done so far is passed in a reference to the object in the sub-class constructor, but I find I'm repeating the exact same fields and ctor declaration (changing the name to match of course) which after the 3rd time of hitting "Paste" made me stop, and take a step back and realise I'm doing something wrong.

I've also considered making these 'sub-classes' : StoresAdaptor but the base StoresAdaptor inherits from an abstract class with no parameterless constructor - and as such I would need to duplicate the constructor of the base class over and over.

Can anyone suggest which pattern would work best in this scenario? I will keep working on it but if I feel something is wrong with the pattern, there must be something VERY wrong with the design.

1
  • Please do let me know why the down-vote, rather than just down voting and leaving. I cannot learn if I am not given an opportunity to do so. Cheers.
    – DeeKayy90
    Jan 10, 2017 at 1:04

2 Answers 2

1

ReturnMiscMaterialAdaptor and IssueMiscMaterialAdaptor appear to be inner classes. Is that right? Instead, make these subclasses to an abstract class StoresAdaptor. abstract classes allow for inherited implementation, including constructors. Either or both of ERPAdaptor and StoresAdaptor could be abstract - I don't know enough about the design to be more specific.


Given the above you should not need to pass Stores.Lines parameter


Stores.Line appears to be a component inside of Stores so in Stores wrap that:

class Stores {
   public string FromLocation { get { return this.Line.FromLocation; }} 
}

Alternatively, but not preferred, you can wrap Store properties in the adapter class. Note, this is probably moot if inheritance, as suggested, is used.

BEFORE:

public Update()
    {
        ERPSystem.ToLocation = _storesAdaptor.storesRequest.ToLocation;
        ERPSystem.FromLocation = _storesLine.FromLocation;
        //Return specific functionality
    }

AFTER:

public ReturnMiscMaterialAdaptor(StoresAdaptor storesAdaptorRef, Stores.Line storesLine)
    {
        _storesAdaptor= storesAdaptorRef;
        _storesLine= storesLine;
        this.ToLocation = _storesAdaptor.storesRequest.ToLocation;
        this.FromLocation = _storesLine.FromLocation;
    }    
public Update()
    {
        ERPSystem.ToLocation = this.ToLocation;
        ERPSystem.FromLocation = this.FromLocation;
        //Return specific functionality
    }

In general, I'm saying apply the least knowledge principle in the adaptee and adaptor classes.


I disagree with the notion stated in another answer that the adapter classes could be static. How can you expose an API for a stateful object and guarantee its consistency? What happens when the client needs a second adapted object?


P.S. adaptor is misspelled, should be adapter.

1
  • I like this response. The design is simple yet perfect for our requirements. And it removes the need for unnecessarily complicated code! As for the spelling of Adaptor/Adapter - I am more familiar with the latter also, but for whatever reason, we've chosen to go with the former spelling.
    – DeeKayy90
    Jan 10, 2017 at 1:09
1

I think the problem is easily solved once you realize that one does not need a StoresAdaptor and a Stores.Line in order to build a ReturnMiscMaterialAdaptor or a IssueMiscMaterialAdaptor, nor the class needs to keep them in the internal state in order to function. Base on your code, they seem not needed until the Update method is called, so they should be parameters of that call, instead.

In fact, it almost looks like the Adaptor classes are good candidates for static classes -- of course not currently possible because they are subclasses of ERPAdaptor.

A possible alternative solution:

private class IssueMiscMaterialAdaptor
{
    public IssueMiscMaterialAdaptor() { }

    public Update(StoresAdaptor storesAdaptorRef, Stores.Line storesLine)
    {
        var _storesAdaptor= storesAdaptorRef;
        var _storesLine= storesLine;
        ERPSystem.ToLocation = _storesAdaptor.storesRequest.ToLocation;
        ERPSystem.FromLocation = _storesLine.FromLocation;
        //Issue specific functionality
    }
    // bay to task
}

or even

private class IssueMiscMaterialAdaptor
{
    public IssueMiscMaterialAdaptor() { }

    public Update(Location toLocation, Location fromLocation)
    {
        ERPSystem.ToLocation = toLocation;
        ERPSystem.FromLocation = fromLocation;
        //Issue specific functionality
    }
    // bay to task
}
1
  • I like this response also! I am partial to the static classes idea, but the alternatives provided are great ideas - both to provide the required fields or the objects into the method themselves. If I could mark this as an answer also, I would as it does answer the question. As it is said, "there are many ways to skin a cat" - and this is most definitely a good way to do so!
    – DeeKayy90
    Jan 10, 2017 at 1:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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