2

I have a app using Laravel framework and there are some conditional rules that I dont know what is the best way to code and maintain.

Use case: conditionally apply promotion code

  • promo code can be applied within specific date or date range
  • promo code can be applied on order >= $100
  • promo code can be applied for specific item
  • ...

Basic solution is to write multiple IF ELSE statements to check 1 by 1. For example:

if ($promo->specific_date) {

} 
elseif ($promo->date_range >= 'date' && $promo->specific_date <= 'date') {

}

if ($totalAmount < 100) {
    // Dont allow
}

if (! $promo->allowed_items) {
    // Dont allow
}

// More conditions ...

I can see that code will be problematic in testing and maintaining.

So Im wondering if there is a better way to handle this? E.g. using a OOP way?

P/S: To clarify my use case:

  • I need all rules to pass to make a promo valid
  • Im thinking of creating a Rule model so that I can have a CRUD to manage them, and in the backend, I can run a query to get all rules, then call a class to pipe and check each rules... (not sure if this is good or bad idea)

Thanks,

Louis L
  • 81
  • 3
  • 9
  • 1
    As far as I know `switch` is faster than `if` – Nipun Tharuksha Feb 06 '20 at 07:37
  • Are the rules different for different promo codes (and by that I mean, do you have multiple promo codes)? I'm assuming yes, but just in case. – Jeto Feb 06 '20 at 07:40
  • 1
    @NipunTharuksha [I'm doubtful of that.](https://stackoverflow.com/questions/10773047/which-is-faster-and-better-switch-case-or-if-else-if) In any case, performance should not matter in deciding between them. – Jeto Feb 06 '20 at 07:59
  • @Jeto thanks for pointing me for a better understand.Could you please check this [switch vs if else](https://www.geeksforgeeks.org/switch-vs-else/) – Nipun Tharuksha Feb 06 '20 at 08:27
  • Well, the point is using inline IF and/or SWITCH is hard to read and digest; and that is what Im trying to avoid. – Louis L Feb 06 '20 at 09:37
  • Using `switch` you can't do a test based on a range like `$date>= $my_date` – Med.ZAIRI Feb 06 '20 at 10:40

3 Answers3

0

use switch statement here because it faster then if

by default set promo_code flag false becuase now only want to my any condition it true then set promo_code flag true ..

if you want to used if statement in one by one then it's good because easy to readable and maintainable

$promo_code_flag = false;

if ($promo->specific_date) {
    $promo_code_flag = true;
} 
elseif ($promo->date_range >= 'date' && $promo->specific_date <= 'date') {
     $promo_code_flag = true;
}

if ($totalAmount > 100) {
    $promo_code_flag = true;
}

if ( $promo->allowed_items) {
    $promo_code_flag = true;
}

if($promo_code_flag) { 
 //allow promo code
}//other wise it will not allowe
Jignesh Joisar
  • 13,720
  • 5
  • 57
  • 57
  • This is wrong, as it will allow the promotion code if *any* of the rules is verified, as opposed to all of them. Also, you don't need to check all the rules all the time. – Jeto Feb 06 '20 at 08:01
  • @Jeto he only want if any condition verified than promo code will allow so i give that condition based on answer – Jignesh Joisar Feb 06 '20 at 08:06
  • He's testing for invalid rules first, so to me he clearly intends to validate a promo only if all of these rules pass. – Jeto Feb 06 '20 at 08:08
  • Thanks guys, my apologies, I was not clear at the begining. The provided snippet is just a quick example. My case is that I need all rules to pass to make it valid. – Louis L Feb 06 '20 at 09:41
0

You could define the promotion properties in your Promotion model (which means they should probably be stored somewhere in your database), and then have a normalized validator that you can call for any promo.

Here's some sample/pseudo code to explain the process (it's PHP 7.4, simply remove the property types to make it work for previous versions):

final class Promotion
{
  private DateTime $minDate;
  private DateTime $maxDate;
  private DateTime $minAmount;
  private array $allowedItems;

  public function getMinDate(): DateTime
  {
    return $this->minDate;
  }

  public function getMaxDate(): DateTime
  {
    return $this->maxDate;
  }

  public function getMinAmount(): DateTime
  {
    return $this->minAmount;
  }

  public function getAllowedItems(): array
  {
    return $this->allowedItems;
  }
}

final class PromotionValidator
{
  public function isPromotionValid(Promotion $promo, array $purchasedItems, int $totalAmount): bool
  {
    $now = new \DateTime();

    if ($now < $promo->getMinDate() || $now > $promo->getMaxDate()) {
      return false;
    }

    if ($totalAmount < $promo->getMinAmount()) {
      return false;
    }

    if (count(array_intersect($purchasedItems, $promo->getAllowedItems())) !== count($promo->getAllowedItems())) {
      return false;
    }

    return true;
  }
}

Usage:

$promotionValidator = new PromotionValidator();
$promoIsValid = $promotionValidator->isPromotionValid($promo, $cartItems, $cartAmount);
Jeto
  • 14,596
  • 2
  • 32
  • 46
  • Thanks @Jeto, that is what I initially had in mind, create a service to validate. However, my concern is that I want to avoid using nested inline if statements to check (like what you have in the isPromotionValid() method) because it will be hard to maintain and extend in the future (e.g. adding more rules, refactor rules...) – Louis L Feb 06 '20 at 09:45
  • What about a Rule model then, which a promo can be bound to any number of? The `isPromotionValid` code would then be something like `foreach ($promo->getRules() as $rule) { if (!$rule->verify($promo, ...) return false; } return true;`. Would that help? At some point you're gonna have to write the code that checks them though (unless you want to use something like [ExpressionLanguage](https://symfony.com/doc/current/components/expression_language.html)). – Jeto Feb 06 '20 at 09:51
0

You could leverage Laravel pipelines to apply some kind of checks on your order.

Imagine that you could pluck the constraints configuration correctly from the database and build up an array (or something like a ConstraintBag instance) which contains all the constraints that you need to check:

configuration
$constraints = [
    DateRangeConstraint::class,
    TotalAmountConstraint::class,
    AllowedItemsContraint::class,
];

Each constraint may adhere to the same interface (Constraint in this PoC) which will define a single handle method:

use Closure;

class DateRangeConstraint implements Constraint
{
    public function handle($order, Closure $next)
    {
        if ($order->promo->start_date >= 'date' || $order->promo->end_date <= 'date') {
            throw new PromotionConstraintException($this);
        }

        return $next($order);
    }
}

Then in your controller/service method you could use this array of rules in a pipeline and pass the order object (or an object which contains all the parts you need for validating all the constraints) though the constraints. If any of this fails, you could trigger a custom exception (maybe one per category of constraint/one per constraint) and return the resulting outcome of the validation process:

// Do not forget to add the use statement
use Illuminate\Pipeline\Pipeline;

class PromotionValidationService
{
    protected $constraints;

    // Pass in the constraints array you have already built
    public function __construct($constraints)
    {
        $this->constraints = $constraints;
    }

    // Start the validation process and cycle through all the constraints
    // I would pass in the order object as you might need to access the total
    // order amount and/or the items in the order
    public function validate($order)
    {
        try {
            app(Pipeline::class)
                ->send($order)
                ->through($this->constraints);
        } catch (PromotionConstraintException $exception) {
            // Handle the exception and return false or rethrow
            // the exception for further handling from the caller.
            return false;
        }

        return true;
    }
}

Obviously, this is still a proof of concepts and would require more study and architectural planning to handle the various constraints you might need to check (eg: passing the whole $order object might not be the best idea, or it might not be available yet when checking the promotion constraints). However, this could be a flexible alternative to a fixed sequence of if/elses that needs to be edited for each change.

mdexp
  • 3,492
  • 2
  • 9
  • 20