8
\$\begingroup\$

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();
}
\$\endgroup\$
13
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Long, beautiful and Complete. Many thanks, let test and report results. Need special attention. \$\endgroup\$ – ahmad molaie Oct 5 '11 at 22:52
  • \$\begingroup\$ Great answer, but if I can make an obvious comment that with the original solution (in the question), it is far easier to "understand" instantly what is going on by glancing quickly at the code. This proposed solution is far more complex (and yet also far more powerful). If there is actually only a limited set of parcel types/calcs, then the original (simpler) solution may be fine as is. \$\endgroup\$ – dodgy_coder Oct 6 '11 at 5:02
  • \$\begingroup\$ @dodgy_coder - I know, which is why Solution 1 and 2 are there. They give a little more flexiblility, without being all that (much) more complex. But the adaptability of solution 4... was just too much to pass up; with a component based IParcel I can add a HolidayWrappingFee to every parcel type in under 5 minutes (by implementing the new calculation, and adding the component to the parcel), with the existing solution I have to actually modify everything. \$\endgroup\$ – Clockwork-Muse Oct 6 '11 at 15:33
  • \$\begingroup\$ I myself prefer solution 4. the project right now is very complex and there are many more development ahead. for example we need to add TNT & DHL support. also we have discount for bulk parcels, there are even discounts for blinds(for under 7Kg parcels), for each type of parcels there are extra service(sending sms | email | ...) & many more things, that i don't mention here, because of simplicity. one important thing is that this project is porting to android after base work done here. \$\endgroup\$ – ahmad molaie Oct 6 '11 at 16:39

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.