15
\$\begingroup\$

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
| improve this question | | | | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Your incoming parameter to the 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\$ – Clockwork-Muse Jul 17 '13 at 18:11
  • \$\begingroup\$ @Clockwork-Muse you're right, I kind of mangled it so I could get it to work by passing the parameter value in both instances. \$\endgroup\$ – wootscootinboogie Jul 17 '13 at 18:13
  • \$\begingroup\$ Applying Garry's excellent answer renders the following advice moot, but the information is important for future reference. 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 remove totalCost and would replace the TotalCost property with public decimal TotalCost {get;private set;}. \$\endgroup\$ – Brian Jul 17 '13 at 20:17
16
\$\begingroup\$

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.

| improve this answer | | | | |
\$\endgroup\$
  • \$\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 Jul 17 '13 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 Jul 17 '13 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 Jul 17 '13 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 Jul 25 '18 at 16:03
4
\$\begingroup\$

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.
| improve this answer | | | | |
\$\endgroup\$
  • 3
    \$\begingroup\$ this is a viable solution, yes, but it ignores the great flexibility of property accessors. The Update* methods could be refactored into the set accessors easily, and still expose the same functionality (but with a cleaner interface). \$\endgroup\$ – thriqon Jul 18 '13 at 7:46

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.