best design pattern for refactoring a set of classes that do calculation base on many parameters

Question

I'm refactoring a set of classes which does some price calculations. the calculation is done based on many parameters. The code is :

public interface IParcel {
    int SourceCode { get; set; }
    int DestinationCode { get; set; }
    int weight{get;set;}
    decimal CalculatePrice();
}

public abstract class GeneralParcel : IParcel {
    //implementation of inteface properties
    //these properties set with in SourceCode & DestinationCode 
    //and are used in CalculatePrice() inside classes that inherit from GeneralParcel 
    protected SourceProvinceCode{get; protected set;}
    protected DestinationProvinceCode{get;protected set;}

    //private variables we need for calculations
    private static ReadOnlyDictionary<int, List<int>> _States_neighboureness;
    private static ReadOnlyCollection<City> _Citieslist;
    private static ReadOnlyCollection<Province> _Provinceslist;

    protected ReadOnlyCollection<City> Citieslist {get { return _Citieslist; }}

    protected ReadOnlyCollection<Province> ProvincesList {get { return _Provinceslist; }}

    protected ReadOnlyDictionary<int, List<int>> StatesNeighboureness {get {return _States_neighboureness; }}

    //constructor code that initializes the static variables

    //implementation is in concrete classes
    public abstract decimal CalculatePrice();
}

public ExpressParcel : GeneralParcel {
    public decimal CalculatePrice() {
        //use of those three static variables in calculations
        // plus other properties & parameters
        // for calculating prices 
    }
}


public SpecialParcel : GeneralParcel {
    public decimal CalculatePrice() {
        //use of those three static variables in calculations
        // plus other properties & parameters
        // for calculating prices 
    }
}

Right now, the code uses "Strategy pattern" efficiently.

my question is that those three static properties, really are not part of parcel object, they are need only for price calculations, so which design pattern or wrapping(refactoring), is suggested?

Is having another interface as below necessary (& then wrap those static properties inside it?, even make static that class, because it is basically only some calculations), then how to connect it to IParcel? Doing so, how best to implement CalculatePrice() in SpecialParcel & ExpressParcel classes?

public interface IPriceCalculator {
    decimal CalculatePrice();
}

Answer 1

Depending on the rest of your design, having a Parcel know how to calculate it's ownworth is perfectly acceptable behaviour. That said, the method of calculation is now tied directly to the parcel class itself (what happens if your local government decides that ExpressParcels under a specific weight should be charged like a GeneralParcel - or worse, some other subclass?).

Also, the three dictionaries likely do not belong in the parcel class at all. I can't imagine that is the only location that type of information is relevant. Shipping/truck routing procedures also come to mind. You should see if this is the case, or if the logic/data is indeed unique.

With that stated, here's some possibilities for refactoring in this situation. You're on the right track with IPriceCalculator; you should modify it to take an IParcel object.

1) Implement IPriceCalculator, store as class (static) variable in IParcel subclasses.

public class GeneralParcel : IParcel {

    private static IPriceCalculator calc = new IPriceCalculator() {

                                               public decimal Calculate(IParcel parcel) {
                                                  return weight * .1;
                                               }
                                           }
}

This allows price calculators to be shared between classes, looked up from a static factory, etc. However, it doesn't allow easy change of the calculator used for a specific instance of that class (such as, say, applying coupon rules). And you still have to ask the parcel how much it is.

2) Implement IPriceCalculator, store as instance variable in IParcel subclasses.

public class GeneralParcel : IParcel {

    private IPriceCalculator calc;

    public decimal Price {
       get {
          return calc.Calculate(this);
       }
    }

    public GeneralParcel(IPriceCalculator calc) {
       this.calc = calc;
    }

}

This allows prices calculators to be shared and swapped out as necessary. The catch is that an individual parcel can't know all the possible restrictions to make the choice, and a calculator shouldn't know about any of the others. Parcel still knows how much it is.

3) Implement IPriceCalculator, lookup from stored cache, store result in IParcel subclasses.

public class GeneralParcel : IParcel {

    private decimal price = -1;

    public decimal Price {
       get {
          if (price < 0) {
              price = CalculatorLookup.find(this).calculate(this);
          }
          return price;
        }
    }

    public GeneralParcel() {

    }
}

This of course caches out the information, preventing costly recalcs. However, getting line-item charges may be dificult, and some of the Lookup rules might be interesting.

---

Finally, my favorite, and the most flexible;

4) Create (and implement) a series of *PriceCalculation*s, store IPriceCalculation in IParcel subclass instances.

public interface IPriceCalculation {
   public decimal Calculate {get;}

   public IPriceCalculation AddRule(IPriceCalculation calc, IParcel parcel);
}

impl

public struct BaseFeeCalculation {
   private readonly decimal price;

   public decimal Calculate {
      get {
         return price;
      }
   }

   private BaseFeeCalculation (IPriceCalculation calc) {
      if (calc == null) {
          price = 5;
      } else {
          price = 5 + calc.Calculate;
      }
   }

   public IPriceCalculation AddRule(IPriceCalculation calc, IParcel parcel) {
      return new BaseFeeCalculation(calc);
   }

}

I recommend that the calculations be structs, and immutable. You're probably going to want to add other things to them, like the ability to report their individual price (for line items), and not just the total. Additionally, while immutablility will help avoid circular references, you may want to add something to check for existing calculations (probably shouldn't have two BaseFees, after all).

Parcel basically stays the same; just add a property that returns the IPriceCalculation used.

When adding rules, try something along the lines of the following:

public struct CalculationBuilder {

   // You'll need to initialize this somehow, obviously.
   private static readonly IList<IPriceCalculation> calculations;

   private CalculationBuilder() {}

   private static IPriceCalculation Build(IParcel parcel) {
      IPriceCalculation calc;  

      foreach(IPriceCalculation calcer : calculations) {
         calc = calc.AddRule(calcer, parcel);
      }   

      return calc;
   }
 }

This works well for anything short of dependent rules (although can be adapted fairly easily for that). Usually, the list of calculations can/should be initialized through a DI/Autowiring framework (like Spring).

Oh, and thank you for using decimal, and not double for your price calculation.

... that was a bit long, wasn't it.