Refactoring code using Strategy Pattern

问题:

I have a GiftCouponPayment class. It has a business strategy logic which can change frequently - GetCouponValue(). At present the logic is “The coupon value should be considered as zero when the Coupon Number is less than 2000”. In a future business strategy it may change as “The coupon value should be considered as zero when the Coupon Issued Date is less than 1/1/2000”. It can change to any such strategies based on the managing department of the company.

How can we refactor the GiftCouponPayment class using Strategy pattern so that the class need not be changed when the strategy for GetCouponValue method?

UPDATE: After analyzing the responsibilities, I feel, "GiftCoupon" will be a better name for "GiftCouponPayment" class.

C# CODE

    public int GetCouponValue()
    {
        int effectiveValue = -1;
        if (CouponNumber < 2000)
        {
            effectiveValue = 0;
        }
        else
        {
            effectiveValue = CouponValue;
        }

        return effectiveValue;
    }

READING

1. Strategy Pattern - multiple return types/values

回答1:

GiftCouponPayment class should pass GiftCoupon to different strategy classes. So your strategy interface (CouponValueStrategy) should contain a method:

int getCouponValue(GiftCoupon giftCoupon)

Since each Concrete strategy implementing CouponValueStrategy has access to GiftCoupon, each can implement an algorithm based on Coupon number or Coupon date etc.

回答2:

You can inject a "coupon value policy" into the coupon object itself and call upon it to compute the coupon value. In such cases, it is acceptable to pass this into the policy so that the policy can ask the coupon for its required attributes (such as coupon number):

public interface ICouponValuePolicy
{
  int ComputeCouponValue(GiftCouponPayment couponPayment);
}

public class GiftCouponPayment
{
  public ICouponValuePolicy CouponValuePolicy {
    get;
    set;
  }

  public int GetCouponValue()
  {
    return CouponValuePolicy.ComputeCouponValue(this);
  }
}

Also, it seems like your GiftCouponPayment is really responsible for two things (the payment and the gift coupon). It might make sense to extract a GiftCoupon class that contains CouponNumber, CouponValue and GetCouponValue(), and refer to this from the GiftCouponPayment.

回答3:

When your business - logic changes, it's quite natural that your code will have to change as well.

You could perhaps opt to move the expiration-detection logic into a specification class:

    public class CouponIsExpiredBasedOnNumber : ICouponIsExpiredSpecification
    {

        public bool IsExpired( Coupon c )
        {
             if( c.CouponNumber < 2000 )
                 return true;
             else
                  return false;
        }
    }

    public class CouponIsExpiredBasedOnDate : ICouponIsExpiredSpecification
    {
       public readonly DateTime expirationDate = new DateTime (2000, 1, 1);

       public bool IsExpired( Coupon c )
        {
             if( c.Date < expirationDate )
                 return true;
             else
                  return false;
        }
    }

public class Coupon
{
     public int GetCouponValue()
     {
        ICouponIsExpiredSpecification expirationRule = GetExpirationRule();

        if( expirationRule.IsExpired(this) ) 
           return 0;
        else
           return this.Value;

     }
}

The question you should ask yourself: is it necessary to make it this complex right now ? Can't you make it as simple as possible to satisfy current needs, and refactor it later, when the expiration-rule indeed changes ?

回答4:

The behavior that you wish to be dynamic is the coupon calculation - which can dependent on any number of things: coupon date, coupon number, etc. I think that a provider pattern would be more appropriate, to inject a service class which calculates the coupon value.

The essence of this is moving the business logic outside of the GiftCouponPayment class, and using a class I'll call "CouponCalculator" to encapsulate the business logic. This class uses an interface.

interface ICouponCalculator
{
    int Calculate (GiftCouponPayment payment);
}

public class CouponCalculator : ICouponCalculator
{
   public int Calculate (GiftCouponPayment payment)
   {
      if (payment.CouponNumber < 2000)
      {
         return 0;
      }
      else
      {
         return payment.CouponValue;
      }
   }
}

Now that you have this interface and class, add a property to the GiftCouponPayment class, then modify your original GetCouponValue() method:

public class GiftCouponPayment
{
   public int CouponNumber;
   public int CouponValue;

   public ICouponCalculator Calculator { get; set; }

   public int GetCouponValue()
   {
      return Calculator.Calculate(this);
   }
}

When you construct the GiftCouponPayment class, you will assign the Calculator property:

var payment = new GiftCouponPayment() { Calculator = new CouponCalculator(); }
var val = payment.GetCouponValue(); // uses CouponCalculator class to get value

If this seems like a lot of work just to move the calculation logic outside of the GiftCouponPayment class, well, it is! But if this is your requirement, it does provide several things:

1. You won't need to change the GiftCouponPayment class to adjust the calculation logic.

2. You could create additional classes that implement ICalculator, and a factory pattern to decide which class to inject into GiftCouponPayment when it is constructed. This speaks more to your original desire for a "strategy" pattern - as this would be useful if the logic becomes very complex.