Decorating a class that calls its own public methods

Question

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()
    {
        LogImportantThings();

        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?

Answer 1

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!