I've written a system that calculates discounts for a shopping cart based on a set of rules. Each rule is implemented with the following interface (C#):

interface IRule
   bool IsActive();
   bool IsApplicableTo(IShoppingCart cart);
   decimal CalculateDiscount(IShoppingCart cart);

A concrete implementation might look something like this:

class Rule : IRule
   bool IsActive() { return true; }

   bool IsApplicableTo(IShoppingCart cart)
       if (!IsActive())
          return false;

       return TestIfApplicable(cart);

   decimal CalculateDiscount(IShoppingCart cart)
       if (!IsApplicableTo(cart))
          return 0m;

       return CalculateStuff(cart);

Now I'm trying to introduce some logging to the system. To keep things clean, I don't want to jam it into the Rule class. So I thought I'd make a Decorator for IRule, which handles the logging:

class LoggingRule : IRule
    IRule decoratedRule;

    bool IsActive()

        return decoratedRule.IsActive();

    //And so on.

The issue that I'm running into here, is that the Rule class actually calls the IsActive and IsApplicableTo methods itself. When that happens, the decorator is simply passed by, and nothing is being logged.

I suppose I could make the LoggingRule a subclass of Rule. It would mean that I would have to make a LoggingRule variant for every IRule implementation. That could potentially cause a lot of duplication.

I could also put the logging into a base class, and use the Null object pattern when I don't want to log anything. But then again, 99 out of 100 times I don't want to log anything. And when these methods are called many many times, is it worth the potential overhead?

So neither solution seems particularly elegant, and I'm struggling to find a better alternative... Any suggestions?

  • Why not an abstract base class again? Then you could have a protected abstract method that "does the work" while the public form of the discount could check if it's active and applicable (though applicable should probably just return 0 rather than calling similar logic twice) – Telastyn Sep 1 '14 at 13:25

Can a Rule be active but not apply to a shopping cart? Yes, right? Can a Rule apply to a shopping cart and not be active? Again, yes. So as I see it, IsApplicableTo method is (or at least should be thought of as ) its own property independent of IsActive. If IsApplicableTo calls IsActive, fine, though as a general rule, if it is public, it is meant to be called, and therefore it will be called twice.

In this way, the one who handles IRule should be calling both, thus the responsibility is not up to IRule to return false if IsApplicableTo would otherwise return true and IsActive returns false. Likewise, don't feel like you need to call IsApplicableTo from within CalculateDiscount, since it is the caller's responsibility. The caller would likely have something similar to the following:

for each rule:
    if rule.IsActive() and rule.IsApplicableTo(shoppingCart):
        total *= (1.0 - rule.CalculateDiscount(shoppingCart));

Should IsApplicableTo return false because IsApplicableTo is calling IsActive and IsActive returns false, the result is ultimately the same. In this way, LoggingRule can decorate another rule, only derive from IRule, and it would work perfectly fine. Just don't make the mistake of adding LoggingRule and the encapsulated rule itself in the same list.

Hope that helps!

  • I follow your reasoning, and I agree with it. I wonder if the IRule interface on its own communicates this clearly enough though? – PJanssen Sep 1 '14 at 15:26
  • @PJanssen Interfaces should convey what methods are readily available to call. If they shouldn't be called, they really shouldn't be in the interface. While it's not given, it strongly implies that they will be called, and all implementations should be aware of that. There's nothing stopping you from calling them within your implementation, but the ultimate responsibility is left with the caller, so yes, in my humble opinion, IRule interface communicates this clearly enough. – Neil Sep 2 '14 at 9:28

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.