I've got a class with a property TotalCost
, which is calculated by some simple math of two other properties, CostOfFood
and NumberOfPeople
. The code works the way I want it to, but I was wondering if this is a satisfactory method in the long run of application development, a bad idea to have one property that depends on another all together (I'm pretty sure this is the case, but sometimes it makes sense to), or if the informed reader would deem it acceptable. Helpful hints are in the comments.
class DinnerParty
{
private int numberOfPeople;
public int NumberOfPeople
{
get { return numberOfPeople; }
set
{
numberOfPeople = value;
//TotalCost property is updated when more people are added to the party
TotalCost =CalculateFoodCost(value);
}
}
private decimal totalCost;
public decimal TotalCost
{
get { return totalCost; }
private set
{
totalCost = value;
}
}
private decimal costOfFood;
public decimal CostOfFood
{
get { return costOfFood; }
set
{
costOfFood = value;
//TotalCost property is updated when CostOfFood changes
//directly below line was my initial idea
//TotalCost = value * NumberOfPeople was my initial thought
//before overloading the CalculateFoodCost method
//this calls the CalculateFoodCost version that takes a decimal
TotalCost = CalculateFoodCost(value);
}
}
private decimal CalculateFoodCost(int costOfFood)
{
//the int coming in as a parameter is the 'value' of the NumberOfPeople property
return this.costOfFood * NumberOfPeople;
}
private decimal CalculateFoodCost(decimal costOfFood)
{
return this.costOfFood * NumberOfPeople;
}
public DinnerParty()
{
//so food cost is never 0
CostOfFood = 10;
}
}
Testing
DinnerParty d = new DinnerParty();
d.NumberOfPeople = 1;
Console.WriteLine(d.TotalCost);//output = 10
d.CostOfFood = 2;
Console.WriteLine(d.TotalCost);//CostOfFood changed, output =2
d.NumberOfPeople = 2;
Console.WriteLine(d.TotalCost);//output=4;
d.CostOfFood = 3;
Console.WriteLine(d.TotalCost); //output =6
2 Answers 2
The C# property model allows external classes to inspect (or set) a given member as though it were a public 'field', and the implementation details are left to the property's accessor and mutator. In your case, you want to expose TotalCost and hide the implementation details about how it is derived. And your code reflects best practices.
Following the comment from Clockwork-Muse, your implementation can be made more elegant by...
public decimal TotalCost
{
get { return CostOfFood * NumberOfPeople; }
}
This avoids the calculation penalty for setting either of the calculation ingredients and performs the calculation only when called upon to do so. It's also a bit more readable and transparent. In this particular case, there's no need for an asymmetric mutator, so it's been removed.
-
\$\begingroup\$ I wasn't aware that you could leave out the backing variable and still use a return statement in the get clause. I actually just returned to this question to find that you had the exact same idea as me. :) +1 \$\endgroup\$wootscootinboogie– wootscootinboogie2013年07月17日 18:57:17 +00:00Commented Jul 17, 2013 at 18:57
-
\$\begingroup\$ Remember that if you eventually go to WPF, it's a whole new ball game where setters are vital to making it work. But for this question, you're doing fine \$\endgroup\$Gayot Fow– Gayot Fow2013年07月17日 18:59:14 +00:00Commented Jul 17, 2013 at 18:59
-
\$\begingroup\$ Appreciate it. I'm slowly but surely getting down the nitty gritty with C# and I'm always trying to toe the tenuous line of 'it's good enough for right now' vs. 'don't get into bad habits!' \$\endgroup\$wootscootinboogie– wootscootinboogie2013年07月17日 19:03:47 +00:00Commented Jul 17, 2013 at 19:03
-
1\$\begingroup\$ I realize it's an old answer, but FYI you can now use the expression-bodied property syntax
public decimal TotalCost => CostOfFood * NumberOfPeople;
\$\endgroup\$Halter– Halter2018年07月25日 16:03:52 +00:00Commented Jul 25, 2018 at 16:03
How about something more along these lines:
class DinnerParty
{
public DinnerParty(int people = 4, decimal price = 3.99)
{
this.NumberOfPeople = people;
this.CostOfFood = price;
}
public int NumberOfPeople { get; private set;}
public decimal CostOfFood { get; private set;}
public decimal TotalCost { get { return CalculateFoodCost();} }
public void UpdateNumberOfPeople(int people)
{
if (people <= 0) {
throw InvalidArgumentException("Can't have Zero or Fewer people.");
}
this.NumberOfPeople = people;
}
public void UpdateCostOfFood(decimal price)
{
if (price <= 0.0) {
throw InvalidArgumentException("Can't have Zero or Negative Price.");
}
this.CostOfFood = price;
}
private decimal CalculateFoodCost()
{
return this.CostOfFood * NumberOfPeople;
}
}
- Default Values in Constructor (instead of down below "somewhere").
- Validation of updated values
- As other answer, only calculates price when needed.
-
3\$\begingroup\$ this is a viable solution, yes, but it ignores the great flexibility of property accessors. The
Update*
methods could be refactored into theset
accessors easily, and still expose the same functionality (but with a cleaner interface). \$\endgroup\$thriqon– thriqon2013年07月18日 07:46:45 +00:00Commented Jul 18, 2013 at 7:46
CalculateFoodCost()
isn't doing anything, in either case; I'd probably remove it (especially as you're not actually obeying the contract, as your comment explains). Heck, for something that simple, I'd probably remove the backing variable, and just run the calculation in the getter. \$\endgroup\$value
in both instances. \$\endgroup\$TotalCost
would be cleaner if you used an auto-implemented property (Requires C# 3.0+). Auto-implemented properties are just syntactic sugar to generate and use a backing field. In your case, you would removetotalCost
and would replace theTotalCost
property withpublic decimal TotalCost {get;private set;}
. \$\endgroup\$