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 designed this object for a "bread formula".

[Bread formulas use a concept known as baker's percentages, but you don't need to understand the math to help with the object design.]

I designed it as an object (of type Formula) that contains objects (of type Ingredient).

Both classes have nullable properties. C# demarcates nullable types using a ? after the type, like so:

public decimal? Weight { get; set; }

The reason I declare them as nullable types is because I don't know their values (when I create an instance of them) until I calculate them later, as which point I update the null value with a calculated value.

The calculations are just basic methods, like CalculateSum() or CalculateWeight().

My question is: Is there a better way to design this object? It works, but I feel like it's the wrong way to do things. I feel like I'm trying to force-fit inline/declarative style programming into an OOP design, but I don't know how else to do it, being so new to OOP.

My thinking is: This object has this property. I won't know its value when I create an instance of it, so must declare it as nullable. Once I calculate its value using a method that's also internal to the class, I assign/set its value. Then it will no longer be null.

The calculation methods are basic calculations: public type Method(input,input,...){calc; return calc;}.

Code:

 class Ingredient
    {
        public string Name { get; set; }
        public decimal Percentage { get; set; }
        public bool IsFlour { get; set; }
        public decimal? Weight { get; set; }

        public Ingredient(string _Name, decimal _Percent, bool _IsFlour, decimal? _Weight)
        {
            this.Name = _Name;
            this.Percentage = _Percent;
            this.IsFlour = _IsFlour;
            this.Weight = _Weight;
        }
    }

    class Formula
    {
        public decimal Weight_Total_Dough { get; set; }
        public ArrayList Ingredients { get; set; }
        public int NumberOfIngredients { get; set; }
        public decimal? SumPercentages { get; set; }
        public decimal? SumWeights { get; set; }
        public decimal? SumWeightTotalFlour { get; set; }

        public Formula(decimal _Weight_Total_Dough, ArrayList _Ingredients, int _NumberOfIngredients, decimal? _SumPercentages, decimal? _SumWeights, decimal? _SumWeightTotalFlour)
        {
            this.Weight_Total_Dough = _Weight_Total_Dough;
            this.Ingredients = _Ingredients;
            this.NumberOfIngredients = _NumberOfIngredients;
            this.SumPercentages = _SumPercentages;
            this.SumWeights = _SumWeights;
        }
    }

Here's the code for calculating Formula.SumPercentages, which was declared as nullable, but is now assigned a value.

public void SumPercentages(Formula f)
        {
            //Interate through Formula object, which contains an ArrayList of INGREDIENT objects.
            //Extract the precentages for each ingredient.
            //Add them up.
            //Return the summation as SumOfPercentages.

            decimal SumOfPercentages;
            SumOfPercentages = 0;
            foreach (object x in f.Ingredients) //for each INGREDIENT object in the FORMULA object
            {
                Ingredient ig = (Ingredient)x; //get the INGREDIENT.Percentage
                SumOfPercentages += ig.Percentage; //add it to the other INGREDIENT.Percentages
            }

            f.SumPercentages = SumOfPercentages; //SumOfPercentages was null before, but now we assign a value (SumOfPercentages) to it.
share|improve this question
1  
If you are always setting a field in the constructor, it doesn't need to be nullable. If you're never setting it to a useful value, it shouldn't be a parameter to the constructor. And if it can be derived entirely from other properties on the same object, it should be set by that object rather than calling a function to perform calculations on it. –  Bobson Jan 9 '13 at 18:08
    
@Bobson By derived, do you mean calculated? I can (and do) calculate values from other properties and set those values (to replace) the nulls. –  Thomas Jan 9 '13 at 19:59
1  
I do mean calculated, but my point was that if you have all the information already, you don't need them to be nullable - just do the calculation in the constructor. There's no need for a separate function call to do the math later. And if you don't have all the information at first, then you should reconsider whether those values belong on the object - see my answer for another way to do it. –  Bobson Jan 9 '13 at 20:11
add comment

7 Answers

up vote 16 down vote accepted

Everyone has had good ideas, but since you're asking about OOP, I'd suggest refactoring it altogether.

void Main()
{
    var formula = new Formula(new[] {
        new Ingredient("Flour, AP", 100m, true),
        new Ingredient("Water, Warm", 70.15m, false),
        new Ingredient("Salt", 3.04m, false),
        new Ingredient("Yeast", .24m, false),
        new Ingredient("Pate fermentee", 2.53m, false)
    });
    var weights = formula.GetWeights(10000m);
}

class Ingredient
{
    public string Name { get; private set; }
    public decimal Percentage { get; private set; }
    public bool IsFlour { get; private set; }

    public Ingredient(string _Name, decimal _Percent, bool _IsFlour)
    {
        this.Name = _Name;
        this.Percentage = _Percent;
        this.IsFlour = _IsFlour;
    }
}

class Formula
{
    private IEnumerable<Ingredient> Ingredients { get; set; }
    public decimal SumPercentages { get; private set; }

    public Formula(IEnumerable<Ingredient> _Ingredients)
    {
        this.Ingredients = _Ingredients;
        this.SumPercentages = _Ingredients.Sum(x => x.Percentage);
    }

    public Dictionary<Ingredient, decimal> GetWeights(decimal weightTotalDough)
    {
        return Ingredients.ToDictionary(k => k, v => (v.Percentage / this.SumPercentages) * weightTotalDough);
    }
}

By doing it this way, you separate the formula (which is basically just Ingredient/Percentage pairs) from each application of it. You can use the same Formula object to calculate the weights for 10000g of dough or 50000g of dough - simply call GetWeights() with a different value.

Notice how many fewer properties you need on each class, and the question of "late setting" of some of them is entirely irrelevant. Effectively, weight is not an intrinsic property of an ingredient, because it varies based on the total weight, so Weight shouldn't be a property of Ingredient.

share|improve this answer
    
This is magnitude more elegant than my attempt, if I understand what you've done. I don't entirely, but I'm working on it now and might have questions later. –  Thomas Jan 9 '13 at 20:17
3  
Ask away! I'll be glad to explain anything you need me to. –  Bobson Jan 9 '13 at 20:22
2  
I would probably make some of the properties readonly (or remove the setters) too, so that consumers of the formula can't change it. –  RichardTowers Jan 10 '13 at 0:56
1  
Huh. I did that for Formula, and forgot to do it for Ingredient. Good catch. Edited. –  Bobson Jan 10 '13 at 14:35
add comment

Here's my humble opinion, it generally looks OK, here are some basic tips:

  1. Use generic collections if you can, List<Ingredient> instead of ArrayList, you won't have to cast it later on
  2. You can use LINQ for summing/counting/filtering/projections etc. All kinds of collection-related operations.

The nullable part is not really needed, but it can work. If '0' is a valid value for 'no weight', then you can use regular decimal which will be initialized to 0. If you really want a null value for something special, you can leave it. Right now I'll be working under the assumption that the properties are still nullable.

Considering that, I'd just make a few changes:

public class Formula
{
    public IList<Ingredient> Ingredients { get; private set; }
    public decimal? SumPercentages { get; private set; }
    public decimal? SumWeights { get; private set; }
    public decimal? SumWeightTotalFlour { get; private set; }
    public decimal? WeightTotalDough {get;private set;}
    public int NumberOfIngredients {get;private set}

    public Formula(IEnumerable<Ingredient> ingredients, decimal? weightTotalDough)
    {
        Ingredients = new List<Ingredient>(ingredients);
        SumWeights = Ingredients.Sum(x => (x.Weight ?? 0));
        SumWeightTotalFlour = Ingredients.Where(x => x.IsFlour).Sum(x => (x.Weight ?? 0))
        SumPercentages = Ingredients.Sum(x => x.Percentage);
        WeightTotalDough = weightTotalDough;
        NumberOfIngredients = Ingredients.Count;
    }

}

I'm not quite sure what the WeightTotalDough is and where it comes from, but there it is. I don't know if you have to expose the Ingredients collection or do you just want to have the fields calculated. If it's the latter, make the List private and just expose the properties. Also I assume you won't pass null collection inside the Formula. After initialization, you can just get the values:

var formula = new Formula(yourIngredients, weightTotalDough);
var percentageSumValue = formula.SumPercentages;
share|improve this answer
    
Very helpful. Thank you. If those LINQ queries work as you've written them, that's about 100 lines of code I can rid myself of. Really need to get LINQ into my system. –  Thomas Jan 9 '13 at 16:28
1  
@Thomas No problem, feel free to ask if you have any other questions. :) LINQ is good to know since it can be hugely helpful and make your code more readable. Of course unless you go too far... ;) –  Patryk Ćwiek Jan 9 '13 at 16:51
    
re:WeightTotalDough. That's an aspect of the Baker's Percentages math. You add up all the percentages of a formula (100%, 70%, 5%, 2%) = 175%. Flour (or total flours) is always 100%. It's the baseline. So a recipe with sum of percentages = 175% just means there are 175 "parts" and 100 of those parts are flour. That's where WeightTotalDough comes in. If you want to make 10000 grams (weight) total dough for 177% formula, you know flour weight will be (100 parts/177 parts)*WeightTotalDough or (100/177)*10000=5650g flour. Other ingredients are calculated same way. –  Thomas Jan 9 '13 at 17:50
1  
@Thomas oh, OK, makes sense. My curiosity has been satiated. ;) I guess you might freely leave that property there in such case... –  Patryk Ćwiek Jan 9 '13 at 20:14
1  
@Thomas isn't "Baker's math" just a special case of "N parts" ingredient listing, with one of the ingredients being "100 parts" instead of e.g. saying "10 parts meat, 1 part onions"? Part of the reason I'm asking this is because it means that flour doesn't need to be "special" at all - the only thing distinguishing it from the other ingredients in the math is the fact that its quantity is 100. (or that two different kinds of flour are 65 and 35). Although, there might be some recipes you can't do this with - scaling isn't always a matter of multiplying everything equally. –  Random832 Jan 9 '13 at 21:55
show 2 more comments

I'm a fan of immutable objects for their various benefits. Below is an example of how to make your Ingredient class immutable. Similar techniques can be made to your Formula class. I also highly recommend not using ArrayList in favor of a generic List<Ingredient> as Trust me - I'm a Doctor recommends.

sealed class Ingredient
{
    private readonly string name;
    private readonly decimal percentage;
    private readonly bool isFlour;
    private readonly decimal? weight;

    public string Name { get { return this.name; } }
    public decimal Percentage { get { return this.percentage; } }
    public bool IsFlour { get { return this.isFlour; } }
    public decimal? Weight { get { return this.weight; } }

    public Ingredient(string _Name, decimal _Percent, bool _IsFlour, decimal? _Weight)
    {
        this.name = _Name;
        this.percentage = _Percent;
        this.isFlour = _IsFlour;
        this.weight = _Weight;
    }
}
share|improve this answer
    
Thank you, Jesse. I need to read up on immutability before I understand what your changes mean. I might have questions in a bit. Thanks again. –  Thomas Jan 9 '13 at 16:39
1  
I agree that immutability is great, but I don't like that you left the nullable weight. How can you modify it if it is immutable? Are you going to create a new object? Check my answer for an alternate design –  mariosangiorgio Jan 9 '13 at 16:48
1  
@mariosangiorgio you DON'T modify it. You pass in the weight (or lack thereof) during construction. Nullable doesn't mean necessarily it's going to need to change during the variable's lifetime. I read it as being fixed to either an actual weight or null (no weight needed) when an ingredient is constructed. –  Jesse C. Slicer Jan 9 '13 at 17:42
1  
I don't think you can make the object immutable based on my understanding of baker's percentages. Since you have to calculate the weight of non-flour ingredients based on the weight of the flour, and presuming this code is a way to do that, you'll need to be able to modify the ingredient weights based on changes to the flour weight. –  Josh Jan 9 '13 at 19:54
add comment

I usually don't like too much nullable and I propose an alternative design to your problem.

I think that you should separate the ingredients as specified in the recipe, which contains only the percentages with the actual quantities you need.

I'd go for a class Ingredient describing an ingredient and its percentage, another class Quantity that represent the actual quantity of each ingredient and finally a Recipe class.

The Recipe class should contain the list of ingredients needed and it should have a method List<Quantity> computeQuantities(decimal totalDoughWeight) that computes the quantities of each ingredient you need.

In this way you clearly separate the recipe and the quantities needed in a particular time.

I think that this design is closer to the domain you want to model (In which recipes and actual quantities are separate entities) and it is cleaner because it does not require you to introduce any nullable.

share|improve this answer
    
That's basically what I've done, isn't it? The Formula class (what you call the Recipe class) contains an array (a list) of ingredients and has methods to compute quantities. Maybe I don't understand, but your suggestion is that I should have three classes instead of two: Recipe > Ingredient > Quantity? So instead of quantity being a property of Ingredient, it's a subclass? Let me think on this some more. I see how this would eliminate the nullable type in Ingredient. This might actually be better. Thank you. –  Thomas Jan 9 '13 at 17:35
1  
No, it isn't. My suggestion was more or less the same thing @Bobson suggested. Quantity should not be a subclass of ingredient, but they should represent two different things. Ingredients are the abstract entities you have in recipes, while with quantities you know the actual amount needed. –  mariosangiorgio Jan 10 '13 at 15:43
add comment

Mutability is very often the source of pain for developers and I suggest you to avoid it as much as possible, by breaking into two parts your logic: object construction and object usage.

If the properties of your class are not available all from the beginning, the right approach for me is to to use a builder, which is mutable, but express clearly the fact that there is no object created yet. When you have all the properties set, you can call builder.build to create an instance of the object, and in case the properties are not all set, you will throw an exception here.

As a result, all the clients of the class (meaning those parts of the code which use an instance of that class) could rely on the fact that the instance is in correct state and you can use your object for an infinite amount of time without having to check if that property is null or not. The single point of failure is now the creation of the object, which can throw an exception if you didn't provide on the builder all the required properties.

Separating object construction from object itself is a great way to express your intention, make your code readable and avoid tedious bugs.

share|improve this answer
1  
I believe this exactly what I need to do, because the mutability of these objects is what's causing me so much pain. I did not know about Builder until you mentioned it, so I'm essentially "reinventing the wheel", manually "building" this object and using nullables for the pieces of the object I don't yet have. I think you may have helped me tremendously here, as I often find myself deal with object construction vs. object usage issues. Thank you. I may have more questions later. Thank you! –  Thomas Jan 9 '13 at 20:09
add comment

There are several points that is possible to improve.

If you are working at least with C# 2.0 (as I expect because you mentioned Nullable<decimal>), you could replace ArrayList with List<Ingredient> or even IEnumerable<Ingredient>. This will make it more intuitive, and later you could remove type casting from SumPercentages.

foreach (Ingredient ig in f.Ingredients)
{
    SumOfPercentages += ig.Percentage;
}

Then, if you are working at least with C# 3.0 you could, rewrite SumPercentages using LINQ:

public void SumPercentages(Formula f)
{
    f.SumPercentages = f.Ingredients.Select(ig => ig.Percentage).Sum();
}

Last point, to implement ICloneable for both Formula and Ingredient, then replace

this.Ingredients = _Ingredients;

with

this.Ingredients = Ingredients.Select(ig => ig.Clone()).ToList();

So Ingredients will be a copy of the initial array. This will prevent a situation when you unwittingly change the list of ingredients after you have created Formula.

share|improve this answer
    
My C# is a bit dated and it's very deficient in LINQ. I'm rather glad you guys have pointed this out, as I'm about to start looking for a development job and better get my C# updated and LINQ learned. –  Thomas Jan 9 '13 at 17:51
add comment

Not sure I have a whole lot to add here, but based on the business requirement of representing baker's percentages, I think the object model should be changed to reflect the domain better. As in:

class Ingredient
{
    public string Name { get; set; }
    public decimal Percentage { get; set; }
    // I don't think you want to be able to have multiple flour ingredients
    // so this isn't appropriate in the ingredient object
    //public bool IsFlour { get; set; }
    // Likewise, the weight is calculated in relation to the flour weight, which
    // I believe should be in the domain of the Formula object
    //public decimal? Weight { get; set; }

    public Ingredient(string _Name, decimal _Percent) //, bool _IsFlour, decimal? _Weight)
    {
        this.Name = _Name;
        this.Percentage = _Percent;
        // this.IsFlour = _IsFlour;
        //this.Weight = _Weight;
    }
}

class Formula
{
    // This can't be nullable since it's the key to baker's percentages.
    // It's in the formula since it's a key property of the formula, more than an ingredient
    public decimal FlourWeight { get; set; }
    // In baker's percentages flour is always 100%, isn't it?
    public decimal FlourPercentage
    {
        get { return 100m; }
    }

    // This should be a method, since it's a function of the ingredients
    //public decimal Weight_Total_Dough { get; set; }
    public decimal GetTotalDoughWeight()
    {
        // Cycle through ingredients and return the weight based on
        // the ingredient percentage and the flour weight.
    }

    // Or you can instantiate in the constructor
    private List<Ingredient> _ingredients = new List<Ingredient>();
    public List<Ingredient> Ingredients 
    { 
        get { return _ingredients; }
        set { _ingredients = value; }
    }

    public int NumberOfIngredients()
    { 
        // I agree with other posters that this could be pulled from the list directly
        return _ingredients.Count();
    }

    // All of these can be done on the list directly or via linq
    public decimal GetSumPercentages()
    {
        return _ingredients.Sum(i => i.Percentage);
    }

    // This is where the formula class does its work
    public decimal SumWeights 
    {
        // I assume you want flour in here, too
        return _ingredients.Sum(i => i.Percentage * FlourWeight) + FlourWeight;
    }

    // This is the only part I'm not comfortable with...I think you'll need it but
    // it doesn't feel right here. I might override IEnumerable and do a custom collection for ingredients
    public decimal GetIngredientWeight(Ingredient ingredient)
    {
        return ingredient.Percentage * FlourWeight;
    }

    // Again, I think this should be unnecessary based on how I understand the domain
    //public decimal? SumWeightTotalFlour { get; set; }

    public Formula(decimal flourWeight, List<Ingredient> ingredients)
    {
        this.FlourWeight = flourWeight;
        this.Ingredients = ingredients;
    }
}

Granted, I'm taking more of a DDD approach here, but it makes more sense to me.

share|improve this answer
    
The IsFlour bool is important, but that's domain knowledge. You wouldn't know it otherwise. In short, 100 parts of any recipe is flour. That can be one flour (100%), four flours (25%, 25%, 25%, 25%) or any other combination. Hence the isFlour bool. I see what you mean about the weight calculation belonging in formula. Others suggested that as well. Will move it. Still looking at your other comments. They seem to be related to domain knowledge areas that I left unexplained. Thanks for all of the above. Much appreciated. –  Thomas Jan 10 '13 at 0:29
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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