Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm building an application for a bonded-warehouse. They have services calculated by Unit price like "Container surveillance duty = containers number x storage days x unit price" and they have Container storage duty that its price is variant according to time intervals like:

  • 1 to 3 days price1
  • 4 to 10 days price2
  • up to 11 days price3

I wanted to apply sort of OCP and keep price calculation methods implementation open and easy as I'm using Entity Framework code first.(I know that sounds a bit crazy).

I built this model using inheritance but I'm actually not self convinced with the result, so I did post it to check whether it's correct. My aim is to have a pricing scale that contains all the services with different calculation methods.

Is this model correct?

public class Service
{
    public int ServiceId { get; set; }
    public string Name { get; set; }
}
public class Pricing
{
    public int PricingId { get; set; }
    public int ServiceId { get; set; }
}  

public class UnitaryPricing : Pricing
{
    public decimal UnitPrice { get; set; }
}

public class StoragePricing : Pricing
{
    public List<TimeIntervalPricing> TimeIntervalPricings { get; set; }
}
public class TimeIntervalPricing
{
    public int TimeIntervalPricingId { get; set; }
    public int IntervalStart { get; set; }
    public int IntervalEnd { get; set; }
    public decimal Price { get; set; }

}
share|improve this question

closed as off-topic by Jamal Feb 14 at 7:45

If this question can be reworded to fit the rules in the help center, please edit the question.

    
This question appears to be off-topic because it is about correctness of a design and not a full code review. –  Jamal Feb 14 at 7:45

1 Answer 1

As I mentioned in my comment (that was mysteriously deleted), I think the pricing should be a property of the service, because you select the service you want and then need to get its pricing; you don't select a pricing and then look what service you get for that. The way you wrote it, after selecting a service, you would have to look up which pricing links to this service instead of having its ID already. (This of course changes when a service can have several pricing records associated with it.)

Also, I think TimeIntervalPricing might qualify as a descendant of UnitaryPricing - as long as the interval price is not fixed for the whole interval, but rather by day (so "1 to 3 days - price1" means *price1 * days*). Otherwise I would at least derive it from Pricing, because that's what it is.

That would modify your model to this:

public class Service
{
    public int ServiceId { get; set; }
    public string Name { get; set; }
    public int PricingId { get; set; }
}

public class Pricing
{
    public int PricingId { get; set; }
}  

public class UnitaryPricing : Pricing
{
    public decimal UnitPrice { get; set; }
}

public class StoragePricing : Pricing
{
    public IList<TimeIntervalPricing> TimeIntervalPricings { get; set; }
}

public class TimeIntervalPricing : (*Pricing or UnitaryPricing*)
{
    public int IntervalStart { get; set; }
    public int IntervalEnd { get; set; }
    public decimal Price { get; set; }
}

This would be my way of thinking about it, but it may well be that not knowing EF renders my approach not viable in this case.

Also, I miss the actual calculation methods; where would those go?

share|improve this answer
    
Thanks, though, I'm a bit stuck with calculations methods I think they will be implemented in classes that implement a common interface.This goes with OCP I think right? –  HichemSeeSharp Mar 5 '13 at 7:00
    
The OCP is about extensibility and exchangeability of functionality in the first place. The problem I see with the calculation methods is that I think they might have quite different parameters. If you could post the raw code for each of the calculations, it would be easier to see how they could be made to fit into a common structure. –  TeaDrivenDev Mar 5 '13 at 11:03

Not the answer you're looking for? Browse other questions tagged or ask your own question.