Best design pattern for refactoring a class that does calculation based on many parameters

I'm refactoring a set of classes as below 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; }}

 开发者_JAVA百科   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 to implement CalculatePrice() in SpecialParcel & ExpressParcel classes?

public interface IPriceCalculator {
    decimal CalculatePrice();
}

EDIT: the above was only a big picture of all system, there is also other consideration that in comments, we discus about them, and i write them here again for clearing things .

there is BulkDiscount for all of ParcelTypes. bulk post happens when customer send more than 10 parcels(or any threshold), also there is special discount when one customer send more than 10 parcel to a unique destination(there is only one receiver). now this type of discounts are managed in each parcel type's CalculatePrice(). even there are discount for blinds for under 7Kg parcels.

also right now there are 3 parceltype, i show only 2 of them here. but we need to add other type in future(TNT & DHL support). each type has many services that customer can select and pay for it. for example, sms service or email service & so on.

---

Personally, while others might say that a Parcel shouldn't know how to calculate its own shipping cost, I disagree. Your design already identifies that there are three different kinds of parcel with three different calculations, so to my (naive?) eyes it's entirely appropriate that the object should have a method e.g. CalculatePrice().

If you really want to go that way, then you'd need two implementations of IParcelPriceCalculator (or whatever you call it) and an abstract factory method on the GeneralParcel to create the concrete ExpressParcelPriceCalculator or SpecialParcelPriceCalculator classes. Which, personally, I'd consider overkill, not least as that code will then be tightly coupled to each GeneralParcel implementation anyway.

I would however consider making the static collections of City and Province public static properties of City and Province respectively. That's just tidier, and it's where I'd expect to find them if I were maintaining the code. StatesNeighbourliness should probably go into Province, or it might even justify its own class.

---

The way in which you calculate a price for a given parcel is a responsibility that shouldn't belong to a data object.

Given what you've told me, here is how I would implement, to try and account for future considerations:

public interface IParcel {
    int SourceCode { get; set; }
    int DesinationCode { get; set; }
    int Weight { get; set; }
}

public class PricingCondition {
    //some conditions that you care about for calculations, maybe the amount of bulk or the date of the sale
    //might possibly be just an enum depending on complexity
}

public static class PricingConditions {
    public static readonly PricingCondition FourthOfJulyPricingCondition = new PricingCondition();
    public static readonly PricingCondition BulkOrderPricingCondition = new PricingCondition();
    //these could alternatively come from a database depending on your requirements
}

public interface IParcelBasePriceLookupService {
    decimal GetBasePrice(IParcel parcel);
    //probably some sort of caching
}

public class ParcelPriceCalculator {
    IParcelBasePriceLookupService _basePriceLookupService;

    decimal CalculatePrice(IParcel parcel, IEnumerable<PricingCondition> pricingConditions = new List<PricingCondition>()) {
        //do some stuff
    }
    decimal CalculatePrice(IEnumerable<IParcel> parcels, IEnumerable<PricingCondition> pricingConditions = new List<PricingCondition>()) {
        //do some stuff, probably a loop calling the first method
    }
}

---

The IPriceCalculator would be best practice for Single Responsibility Principle.

But change the method's signature to decimal CalculatePrice(IParcel parcel); The method is calling IParcel's CalculatePrice() method to get base price for each parcel.

---

The advice I'd offer would depend to a degree on how you go about generating and using the Parcel polymorphs. What I mean is, we can't see what criteria are used to determine whether something is "Express" or "Special" and whether those criteria have to do with the properties of the parcel itself or some external factor.

Having said that, I think your intuition is a good one about separating the price calculation from the Parcel object itself. As kmkemp pointed out, a parcel shouldn't be figuring out how to calculate the price of a parcel depending on what type of parcel it is. A parcel is a data transfer/POCO type object, at least as indicated in your giving it a weight, source, etc.

However, I'm not clear on why you're using these interfaces. Don't get me wrong -- interfacing is nice for decoupling and testability, but why is there a parcel interface in addition to an abstract base class with an abstract method. Personally, going on just the information that I have, I'd do this:

public class Parcel
{
    int SourceCode { get; set; }
    int DestinationCode { get; set; }
    int weight { get; set; }
}

public abstract class GeneralCalculator
{
    //Statics go here, or you can inject them as instance variables
    //and they make sense here, since this is presumably data for price calculation
    protected static ReadOnlyDictionary<int, List<int>> _States_neighboureness;
    protected static ReadOnlyCollection<City> _Citieslist;
    protected static ReadOnlyCollection<Province> _Provinceslist;
    //.... etc

    public abstract Decimal CalculatePrice(Parcel parcel);
}

public class ExpressCalculator : GeneralCalculator
{
    public override decimal CalculatePrice(Parcel parcel)
    {
        return 0.0M;
    }
}

public class SpecialCalculator : GeneralCalculator
{
    public override decimal CalculatePrice(Parcel parcel)
    {
        return 0.0M;
    }
}

But, again, I don't know how the parcels are actually being processed. You may need some kind of modification to this concept depending on how you generate and then process the parcels. For instance, if the kind of parcel it is depends on the property values of the parcel, you might want to define a factory method or class that takes a parcel and returns an appropriate instance of the calculator.

But, however you modify it, I'd definitely cast my vote for your thought of decoupling the definition of the parcel from the scheme for calculating its price. Data storage and data processing are separate concerns. I'd also not vote in favor of a static class somewhere containing global settings, but that's my own personal taste -- that kind of thing too easily acquires a setter and becomes a global variable down the road.

---

Like you say, those static properties aren't really part of the GeneralParcel class. Move them to a static "ListsOfThings" class.

Then you can use code that refers to ListsOfThings.ProvincesList, etc.