Is this a stoopid way of doing abstract / concrete logic?

Lorne Kates

So I've got this object, right? Thing. It's main function is to DoStuff. Cool.

But now I have lots of different types of Thing. I've got NewThing and DingyThing and JohnCarpentersTheThing and of course ClobberingTimeThing. They all do the same basic DoStuff, with only a minor difference. But that minor difference is in the middle of Stuff.

The first thought is to just make Thing abstract, inherit it in each of those Things, and rewrite DoStuff each time as:

function DoStuff()
{
   base.DoStuff()
   // my own stuff here
}

Like I said, though, the custom logic is in the middle of DoStuff, with standardized stuff before and after. I could break those into separate functions, but then each class would have to know to call SetupStuff(), DoStuff() and then CleanupStuff().

My solution has always been this, and I don't know if it's a stupid thing:

public abstract class Thing
{
      public void KickItOff()
      {
            // stuff
            DoStuff();
      }
      private void DoStuff()
      {
            // Bunch of crap
            DoABunchOfSetupThatEveryThingNeeds();
            StuffResult = DoStuffConcrete();
           DoABunchOfCommonProcessingOn(StuffResult);
            // even more crap
      }

      protected abstract StuffResult DoStuffConcrete();
}

public class NewThing : Thing
{
      protected StuffResult DoStuffConcrete()
      {
            // do a little bit of code that only NewThing does
            return StuffResult;
      }
}

So anyone can inherit from Thing. They put custom logic in the DoStuffConcrete() function that will always be called. They don't have to worry about any of the setup or teardown, because the base Thing class does that. The DoStuff() function is private, so no one is going to fuck it up. And no matter what the type of Thing is, you can do:

Thing t = ThingFactory.GetThing(type);
t.KickItOff();

iz I a dumb?

boomzilla

As a programmer with a non-formal CS background I can say that I do stuff like that (hah!) all the time without problem.

dcon

I'm sure there are other weird ways of doing it using super advanced language features. But I'm a firm believer in KISS. (Or, for some people KISA.)

This is simple, obvious, and more importantly, maintainable!

Unperverted Vixen

@Lorne-Kates While I'm not a fan of the ThingFactory, the rest seems reasonable.

Lorne Kates

@Unperverted-Vixen said in Is this a stoopid way of doing abstract / concrete logic?:

@Lorne-Kates While I'm not a fan of the ThingFactory, the rest seems reasonable.

In reality, it's usually a Case statement of some sorts that instantiates the proper Thing, does anything special with it, etc.

Which, really, is just another type of factory.

anonymous234

That does seem like the proper way to do it using only standard OOP stuff.

Now if you were open to more esoteric programming methods, I'm sure we could find some slightly cleaner way.

ObjectMike

@Lorne-Kates said in Is this a stoopid way of doing abstract / concrete logic?:

@Unperverted-Vixen said in Is this a stoopid way of doing abstract / concrete logic?:

@Lorne-Kates While I'm not a fan of the ThingFactory, the rest seems reasonable.

In reality, it's usually a Case statement of some sorts that instantiates the proper Thing, does anything special with it, etc.

Which, really, is just another type of factory.

Is there any reason for the factory? Would the factory be doing anything besides the switch statement? If it's just a switch, just have people create things themselves.

Or it may be better to have more concrete factory methods. That way you can expose more concrete concepts without having to expose all of your concrete types.

I know this wasn't the main point of your thread but since somebody latched onto that I figured I'd chime in too.

To your original question, I've used this pattern before, it can work well. Assuming all possible and foreseen implementations would want things in the same order it's fine. You wouldn't want to use the pattern for things where some want to skip init and/or some want to skip clean up. Or I suppose those could just override the "real" methods.

DogsB

Looks fine on my end. To be real OO though you need a snazzy name. Like Template!

On a more serious note that's about standard for solving that problem with OO.

Lorne Kates

@mikehurley said in Is this a stoopid way of doing abstract / concrete logic?:

Is there any reason for the factory?

Mainly just as an example because everyone knows what they are.

@mikehurley said in Is this a stoopid way of doing abstract / concrete logic?:

Would the factory be doing anything besides the switch statement? If it's just a switch, just have people create things themselves.

In one case, it was for payment providers. They all do the same basic thing-- load settings, validate user input, instantiate API, populate request, send request, handle response, populate payment object, return.

There is a switch statement where we take the payment provider type (string). If "paypal" do this, if "bambora" do that, etc. Each of those providers has their own connector in our system, doing nearly the exact same thing. So I wanted to unify them into a base class to standardize API logging, exception handling, obeying http timeout limits, etc.

Each of the Switch statements does have some custom code; ie: Bambora setup has to have a Country Code set, or Bambora will barf. But everything each of those blocks does can easily be put into a "Validate Information" function to the same effect.

If it was up to me, the code would be:

BasePaymentProvider pp = null;
PaymentResult result = null;

switch PaymentProviderSetup.ProviderCompany
{
case "Paypal"
   pp = new PaypalPaymentProvider();
case "Bambora"
  pp = new BamboraPaymentProvider();
etc
}

if (pp != null)
{
   result = pp.DoThePayment(OrderInformation);
}

return result;

I could get even fancier if I wanted, and add a column to the PaymentProviders database table called "DotNetClass". Then I could do:

if (IsInTheCorrectNamespace(PaymentProviderSetup.DotNetClass))
{
  pp = Activator.Create(OurNamespace + "." + PaymentProviderSetup.DotNotClass);
}

if(pp != null)
{
  pp.DoItInTheButt();
}

Then I'd never have to change that switch statement each time we bring a new payment provider into the solution.

topspin

@Lorne-Kates If I understand correctly, this is the non-virtual interface pattern, which is not unusual.

JBert

This works, as long as you don't need inheritance in other strange ways.

Most of the time I would look at the Strategy pattern though because it might offer better unit testing support.

And if that case shows up more than twice I would for sure stuff it into a factory, passing the PaymentProviderSetup.ProviderCompany value to it.

Gribnit

Needs more factories.

Lorne Kates

@Gribnit said in Is this a stoopid way of doing abstract / concrete logic?:

Needs more factories.

Dim factoryFactory as FactoryFactory(of Factory) = new FactoryFactory(of Factory)(factory)

Tsaukpaetra

@Lorne-Kates bookmarking this for tomorrow. I'm adding more cloud providers to Master Server and the switch statement is getting stupid...

Zecc

@mikehurley said in Is this a stoopid way of doing abstract / concrete logic?:

Would the factory be doing anything besides the switch statement? If it's just a switch, just have people create things themselves.

Do you hate abstraction and the single responsibility principle?

Zecc

@Lorne-Kates said in Is this a stoopid way of doing abstract / concrete logic?:

There is a switch statement where we take the payment provider type (string). If "paypal" do this, if "bambora" do that, etc. Each of those providers has their own connector in our system, doing nearly the exact same thing. So I wanted to unify them into a base class to standardize API logging, exception handling, obeying http timeout limits, etc.

I can relate to this, except we have many more methods. Switches everywhere!

Lots of the switches don't consider providers that were added at a later point in time.
Interfaces force you to implement every method, and that's a good thing.

MrL

Looks fine to me.

Gąska

Seconding the strategy pattern. It works virtually the same, but composition > inheritance.

Anyway, your code is fine, no WTF in there (at least not in this little part you've shown to us).

dkf

@Lorne-Kates said in Is this a stoopid way of doing abstract / concrete logic?:

My solution has always been this, and I don't know if it's a stupid thing:

It doesn't look stupid to me. It's exactly the sort of thing why you can make protected abstract methods.

KattMan

@Lorne-Kates Personally I think this is just a little wrong. Does it perform the task? Yes. Does it work easily? Yes. Is it easy to understand and Maintain? Yes. So where do I think it is wrong? This is a classic example of where you really don't want multiple sub-classes to perform this but rather what you want is a visitor into this one class to perform the specific modification of your process that can be injected into this class prior to executing.

So really create your thing, inject a visitor, execute DoStuff().
Or possibly Create your thing call DoStuff(IVisitor dingyVisitor) so that the process will execute the visitor at the proper location and the visitor can be as simple or complex as you need it to be.

Gąska

@KattMan yeah, there's also visitor pattern. Although I'm 99% sure it doesn't quite fit the OP's problem, and strategy will be better here.

Captain

@Lorne-Kates Yeah, what you did is called the "template pattern", for somewhat obvious reasons. Not stupid.

In fact, it's somewhat "very common" in large scale Haskell systems, where you'd define abstract "type classes" with mixed methods, like:

class Template t where
    -- these functions aren't defined by default, so each instance needs its own definition
    heading :: t -> Html
    body :: t -> Html
    footer :: t -> Html

    -- this is a "template" for a "page".  It has a default definition defined in terms of the
    -- "abstract" functions ( and `<>` means append)
    page :: t -> Html
    page t = (heading t) <> (body t) <> (footer t)

pie_flavor

Yes.
*proceeds to read post*

JazzyJosh

Alternatively, pass the implementation into the base Thing, optionally with DI instead of a switch.

interface ZhuLi {
    void doTheThing(Thing thing);
}

class Thing {
    private final ZhuLi zhuLi;
    
    @Inject
    public Thing(final ZhuLi zhuLi) {
        this.zhuLi = zhuLi;
    }

    public void doTheThing() {
        getReadyToDoTheThing();
        zhuLi.doTheThing(this);
        theThingIsDone();
    }
}

And then you can unit test the specific logic that MyCustomStuffDoer does easily.

IOW KattMan's answer.

Also, ideally you're working on something other than this I'm just lazy.

dkf

@JazzyJosh said in Is this a stoopid way of doing abstract / concrete logic?:

Alternatively, pass the implementation into the base Thing, optionally with DI instead of a switch.

What makes most sense depends on context, whether you're mainly using a bit of functionality from over there or filling out the incomplete bits of over here. Of course, you're doing a bit of both, but context determines which aspect is the dominant one when thinking about the code.

JazzyJosh

@dkf Agreed

xaade

@Lorne-Kates said in Is this a stoopid way of doing abstract / concrete logic?:

@Gribnit said in Is this a stoopid way of doing abstract / concrete logic?:

Needs more factories.

Dim factoryFactory as FactoryFactory(of Factory) = new FactoryFactory(of Factory)(factory)

        public class Factory
        {
            public object Factory { get { return new Factory(); } }
        }

        public class Main
        {
            public void Do() 
            {
                Factory factory = new Factory();
              
                factory = (Factory)((Factory)(factory.Factory)).Factory; // ad infinum
            }
        }

xaade

@Gąska said in Is this a stoopid way of doing abstract / concrete logic?:

composition > inheritance.

The biggest problem I have with composition is that I'm lazy, and there's no efficient way to make passthrough methods.
It would be nice if there was a way to tell the compiler that certain members' methods will generate pass through methods on the parent type.

Something that allowed us to mix wrapped calls with passthrough calls.

public class Child {
public void DoOneThing();
public void DoAnotherThing();
public void DoAwesomeThing();
}

public class Parent {
[Passthrough("DoOneThing", "DoAnotherThing")]
public Child Child { get; }
public void DoAwesomeThing() {
// do parent specific awesome thing
Child.DoAwesomeThing();
}
}

Parent parent = new Parent();
parent.DoOneThing();
parent.DoAwesomeThing();

dkf

@xaade said in Is this a stoopid way of doing abstract / concrete logic?:

The biggest problem I have with composition is that I'm lazy, and there's no efficient way to make passthrough methods.

That depends on the object system, but it's not particularly common.

pie_flavor

@xaade I like Kotlin.

open class MyThing(private val wrapped: MutableList<String>) : MutableList<String> by wrapped

JBert

@pie_flavor You are doing a "BenL" here. What does that do?

pie_flavor

@JBert When you say by (expr) after something you extend, it means to implement the interface by forwarding all the methods to the object produced by the expression (which must implement the interface). Here I am creating a class named MyThing which takes a list in its constructor and assigns it to a private read-only field, and implements the same list interface by forwarding all the methods to that field. Note that I chose to make it a constructor parameter and a field; there is no requirement that it be either.

Jaloopa

@Lorne-Kates said in Is this a stoopid way of doing abstract / concrete logic?:

@Gribnit said in Is this a stoopid way of doing abstract / concrete logic?:

Needs more factories.

Dim factoryFactory as FactoryFactory(of Factory) = new FactoryFactory(of Factory)(factory)

I found a SomethingFactoryFactory in some code today. Stared at it for a good minute before deciding I didn't want to go down that rabbit hole

Captain

@dkf Yep, a language with a message-passing/actor model would make it easy.

This was actually one of the nicer things about Ruby.