1

I need to make a sponsorship system with complex business requirements. Basically, after a user makes a payment, the system should get triggered. There are many different types of sponsoring, so I found abstractions to make my life simpler now and in the future.

To use polymorphism, I first created an abstract class that every sponsoring system would inherit from, and implement each abstract method needed.

Here's the class :

abstract class SponsorshipService
{
    /** @var UserKang */
    protected $user;
    /** @var UserKang */
    protected $sponsor;
    /** @var SponsorUser */
    protected $sponsorship;
    /** @var int */
    protected $amount;
    /** @var BillingMicroService */
    protected $billingMicroService;
    /** @var MailMicroService */
    protected $mailMicroService;

    public function __construct(BillingMicroService $billingMicroService, MailMicroService $mailMicroService)
    {
        $this->billingMicroService = $billingMicroService;
        $this->mailMicroService = $mailMicroService;
    }

    public function apply(UserKang $user, int $amount): bool
    {
        $this->user = $user;
        $this->amount = $amount;

        if (!$this->user->isSponsored()) {
            return false;
        }

        $this->sponsor = $this->user->getSponsor();
        $this->sponsorship = $this->user->getSponsorship();

        if (!$this->checkSponsorshipAppliance()) {
            return false;
        }

        $offeredCash = $this->getOfferedCash();
        $isCreditOffered = $this->offerCreditSponsorship($offeredCash);

        if (!$isCreditOffered) {
            return false;
        }

        $this->sendEmail();
        $this->updateSponsorship();
    }

    abstract protected function checkSponsorshipAppliance(): bool;

    abstract protected function getOfferedCash(): int;

    abstract protected function offerCreditSponsorship(): bool;

    abstract protected function sendEmail(): void;

    abstract protected function updateSponsorship(): void;
}

This is all good so far, but I'd like to unit tests each of these abstract methods for each SponsorshipService implementation I'll make. But I made these protected, because it doesn't seem like they have any reason to be public, and I think the less public methods a class has, the easier it is to use, and the safer it is too.

What's wrong in this design ? Clearly I don't want to unit test apply in every possible way for every implementation as it would be a nightmare as much now as it'd be in the future.

  • 1
    related: Testing private methods as protected – gnat Dec 9 '18 at 13:36
  • @gnat I don't understand how this is related ? Isn't that answer talking about private vs protected ? I feel like something is wrong because I'm using protected, but I don't want to use public nor private. – Steve Chamaillard Dec 9 '18 at 14:14
  • the point is, one generally better abstains of weakening access restrictions for (incorrectly understood) testability considerations – gnat Dec 9 '18 at 14:37
  • 1
    If I don't go that far though and we accept protected methods, then still you should be able to cover all the test "gaps" for a particular class by thoroughly its public interface. Whatever use a particular subclass gains from its protected interface is still just part of properly ensuring that its public interface functions correctly. To put it another way, theoretically you might have a protected method which functions incorrectly in some peculiar edge use case. But if it's never used that way by the subclass, then for all practical purposes to the outside world, the class works as intended. – Dragon Energy Dec 9 '18 at 15:03
  • 1
    @DragonEnergy Oh I see, so basically what you're saying is that since it's protected/private, it won't be used in any other way than the ways which are used/tested in the public interface, which means the edge cases are bound to the upper level of visibility ? – Steve Chamaillard Dec 9 '18 at 15:25
6

What's wrong in this design?

It is a subjective question, though here's some food for thought:


The constructor initializes two instance members (billingMicroService, mailMicroService), yet not all instance members — the others get initialized in apply.

When we have instance fields that can be grouped into two (or more) distinct sets of lifetimes within one class, it suggests that we have conflated what could be independent domain concepts/entities — and this suggests refactoring into two (or more) classes.  (Sometimes one set of instance fields could become parameters instead of another class.)


apply also conditionally initializes some fields suggesting possible further conflation.


Do you expect a single concrete subclass to implement all the features you are offering in the abstract base, like sending emails and offerings of cash as well as offerings of credit?

This feels like a violation of single responsibility, for one.

But for another, if you want to mix and match abilities to send emails, with different ways to offer cash, with yet differing ways of offering credit, you'll have to create a new subclass for each combination — this results in class explosion, and can be addressed by composition.

(While it is true that you can introduce abstract subclasses that offer partial functionality so you have more DRY (reuse) than re-implementing things in each concrete subclass, managing a class hierarchy to pick features that way is hard an unnecessary since this can be done more simply with composition instead.)


If you use composition instead of inheritance, I believe you'll find testing individual capabilities easier.

  • So I tried using a composition design, which leads to a significant increase in number of classes. Instead of having 3 different implementations extending the base abstract class, I now have 2 validating classes, 3 offered cash calculating classes, 2 classes to offer credits, 3 email sending classes, and 1 sponsorship repository to update the model. Doesn't it seem like a lot ? Although I understand it's a lot more flexible, is the complexity worth it ? Could it be that I'm having trouble finding the correct abstractions to use composition with less classes created ? – Steve Chamaillard Dec 9 '18 at 15:48
  • 2
    We need as many classes as we have independent concepts in the domain -- that is not artificial complexity, it is real world complexity -- the domain is rich. – Erik Eidt Dec 9 '18 at 15:54
  • 3
    So, I disagree that sheer class count represents increased complexity over inheritance (which has its own class count). The complexity introduced by conflating domain entities and their lifetimes, by inheritance and overrides, is higher in my book than having numerous simple(r) classes connected by composition & delegation. A simpler design, even with more classes, is less complex. – Erik Eidt Dec 9 '18 at 15:57
  • 3
    I can solve any computing problem with only one class. I also refuse to maintain systems that use only one class. :P – candied_orange Dec 9 '18 at 16:27

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.