5

I am designing BC for Promo Codes. They work like this: Admin can create promo code by specifying

  1. Details (like Code and Description)
  2. Benefit (interface for ValueObjects, for example MoneyDiscountBenefit)
  3. Usage Restrictions - collection of AbstractUsageRestriction, each restricting use of promocode, for example:

    • PromoCode can be used only by sigle user
    • Promocode can be used on max 3 Order Items by each user
    • Promo Code can be used on max 10 Order Items
    • Promo Code can be used only to some OrderItems (some specific type, or where price is above some limit etc.)

Customer can apply this promo code on his Order (some VO in this BC), then it is applied on all OrderItems fulfilling all restrictions. Then on OrderPaid event, PromoCode is "used" on each Order Item filfulling restrictions. "Used", because this BC from customer perspective just handles that promo code is marked as applied/used and then next restriction checking (by anyone on this promocode) while applying/using can be affected by this use (if desired), for example with UniqueUsageRestriction if some user use promocode, another can't anymore - restriction throws exception.

Now, my entity looks like this:

<?php

class PromoCode
{

    /** @var PromoCodeId */
    private $id;

    /** @var Details */
    private $details;

    /** @var Benefit */
    private $benefit;

    /** @var UsageRestrictions */
    private $usageRestrictions;

    /** @var PromoCodeUsesCollection */
    private $promoCodeUsesCollection;

    /** @var PromoCodeAppliesCollection */
    private $promoCodeAppliesCollection;

    public function __construct(PromoCodeId $id, Details $details, Benefit $benefit, ?UsageRestrictions $usageRestrictions = null)
    {
        $this->id                         = $id;
        $this->details                    = $details;
        $this->benefit                    = $benefit;
        $this->usageRestrictions          = $usageRestrictions ?: new UsageRestrictions();
        $this->promoCodeUsesCollection    = new PromoCodeUsesCollection();
        $this->promoCodeAppliesCollection = new PromoCodeAppliesCollection();
    }

    public function getId(): PromoCodeId
    {
        return $this->id;
    }

    public function getBenefit(): Benefit
    {
        return $this->benefit;
    }

    public function getOrderItemIdsOnWhichWasAppliedWith(OrderId $orderId){
        $this->promoCodeAppliesCollection->getOrderItemIdsWith($orderId);
    }

    public function applyFor(Order $order): void
    {
        $this->checkApplicabilityForOrder($order);

        foreach ($order->getOrderItems() as $orderItem) {
            $this->checkApplicabilityForOrderItem($order, $orderItem);
            $promoCodeApply = new PromoCodeApply($order, $orderItem);
            $this->promoCodeAppliesCollection->addApply($promoCodeApply);
        }
    }

    public function useFor(Order $order): void
    {
        $this->checkApplicabilityForOrder($order);

        foreach ($order->getOrderItems() as $orderItem) {
            $this->checkApplicabilityForOrderItem($order, $orderItem);
            $promoCodeUse = new PromoCodeUse($order, $orderItem);
            $this->promoCodeUsesCollection->addUse($promoCodeUse);
        }
    }

    public function countAppliesForOrder(OrderId $orderId): int
    {
        return $this->promoCodeAppliesCollection->countAppliesForOrder($orderId);
    }

    public function countOrdersWithUseByUser(UserId $userId): int
    {
        return $this->promoCodeUsesCollection->countOrdersWithUseByUser($userId);
    }

    public function canBeUniqueForUser(UserId $userId): bool
    {
        return $this->promoCodeUsesCollection->canBeUniqueFor($userId);
    }

    public function hasCode($code)
    {
        return $this->details->getCode()->getValue() === $code;
    }

    private function checkApplicabilityForOrder(Order $order): void
    {
        $this->usageRestrictions->checkApplicabilityForOrder($this, $order);
    }

    private function checkApplicabilityForOrderItem(Order $order, OrderItem $orderItem): void
    {
        $this->usageRestrictions->checkApplicabilityForOrderItem($this, $order, $orderItem);
    }
}

I have feeling that it knows and does too much, but how to split that?

I dont like those use and apply functions somehow, maybe that should do some strategy? So I would refactor it to something like:

    public function applyFor(Order $order): void
    {
        $this->applyPromoCodeStrategy->applyForOrder($this, $order, $this->usageRestrictions, $this->promoCodeAppliesCollection)
    }

But I dont like that I am passing there a) four parameters, b) That strategy now does something with state of promoCodeAppliesCollection property without PromoCode knowing.

It is somehow weird that PromoCode answers questions like countOrdersWithUseByUser or canBeUniqueForUser etc. Just to fillfull questions of some specific restrictions in UsageRestrictions. I would need to make some mutable object inside PromoCode entity (with those collections with applies and usages) that would not have any id, and would handle all those things around using and checking restrictions separately, but I didn't see something like this anywhere.

What approaches I can use to improve this design? Or can you name some design smell with possible solutions here?

  • 1
    Why the difference between applyPromoCode and usePromoCode? Isn't the used state for a PromoCode entirely determined by the state of its Order? – guillaume31 May 2 '17 at 12:42
  • 1
    Okay, makes sense. It don't find it shocking that PromoCode knows about the number of times it was used by a user in an Order. That's in line with its role as an Aggregate Root in actions involving promo codes. – guillaume31 May 2 '17 at 13:37
  • 1
    The only thing I see is that a checkoutOrder() transaction will sometimes span across multiple aggregates (Order and PromoCode) but there is probably not a lot of contention between clients to modify a given PromoCode, so it might be a non problem. – guillaume31 May 2 '17 at 13:41
  • 1
    1) yes, splitting an AR into smaller, cohesive entities is a good idea. – guillaume31 May 2 '17 at 14:47
  • 1
    2) That's good news. Theoretically does not keep the same malicious user from using same code twice in a very short time, but you can detect this and compensate after. – guillaume31 May 2 '17 at 14:52
1

For me the smell is the various functions which just pass the call down the chain to the sub objects, useageRestrictions, uses and applies collection.

This obviously expands the number of functions and hence responsibility of your object.

Consider a more functional design where the data entity for the promotion is separate from the object (or objects) which evaluates the price of a basket.

Although this may seem less OO in fact you have these objects already hidden by your entity. If you expose them and give them better names, Till? CheckOut? you may find the design is clearer

| improve this answer | |
  • In this BC i dont have anything like basket or price. There is Order VO, but just to check restrictions. Can you elaborate what you mean by "more functional design"? BTW i think hiding those objects by entity is ok, because they are not used anywhere else and are something like "private" for entity. – Tom Apr 29 '17 at 13:13
  • more functional as in less OO. sounds like you need to relook at your domain if you have order and promotions but no pricing. – Ewan Apr 29 '17 at 15:12
  • Why prices should play any more important role? Applied PromoCode actually does not need to affect price at all. You can have PromoCode that in the end just adds some other than discount bonus to Order Item. But that is handled in another BC - shop. PromoCode BC just takes responsibility for creating promocodes, applying them and using them (while handling restrictions) which in the end results in some Events that says Promocode was applied/used on certain item with some bonus XXX. Shop BC then somehow reacts to those events (apply bonuses to order which results maybe in discounted price). – Tom Apr 30 '17 at 0:36
  • yup sounds wrong to me. of course I dont know the full situatiom – Ewan Apr 30 '17 at 5:20
  • But that is domain specification. Promocodes are not just discounts, they can translate to some other type of bonus/benefit. There is no problem that I am dealing with. – Tom May 1 '17 at 19:04
1

Ok, so it's not rocket science as I thought. The solution is to just introduce new entity (PromoCodeUsage) that is hidden in PromoCode, is it's implementation detail and taking care for responsibility of how promo code is used and applied :). So now it looks nice and clean and shiny:

<?php declare(strict_types = 1);

class PromoCode
{

    /** @var PromoCodeId */
    private $id;

    /** @var Details */
    private $details;

    /** @var Benefit */
    private $benefit;

    /** @var PromoCodeUsage */
    private $promoCodeUsage;

    public function __construct(PromoCodeId $id, Details $details, Benefit $benefit, ?UsageRestrictions $usageRestrictions = null)
    {
        $this->id             = $id;
        $this->details        = $details;
        $this->benefit        = $benefit;
        $this->promoCodeUsage = new PromoCodeUsage($usageRestrictions);
    }

    public function getId(): PromoCodeId
    {
        return $this->id;
    }

    public function getBenefit(): Benefit
    {
        return $this->benefit;
    }

    public function getCode(): Code
    {
        return $this->details->getCode();
    }

    public function hasCode($code)
    {
        return $this->details->getCode()->getValue() === $code;
    }

    public function applyFor(Order $order): void
    {
        $this->promoCodeUsage->applyFor($order);
    }

    public function useFor(Order $order): void
    {
        $this->promoCodeUsage->useFor($order);
    }

    public function countAppliesForOrder(OrderId $orderId): int
    {
        return $this->promoCodeUsage->countAppliesForOrder($orderId);
    }

    public function countOrdersWithUseByUser(UserId $userId): int
    {
        return $this->promoCodeUsage->countOrdersWithUseByUser($userId);
    }

}
| improve this answer | |

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.