views:

186

answers:

4

I had asked this question previously on SO. This is related to it. We have code base similar to this:

IRecipie FindRecipiesYouCanMake(IEnumerable<Ingredientes> stuff, Cook cook)
{
 if(stuff.Any(s=>s.Eggs && s.Flour) && cook.DinerCook)
 {
  if(s=>s.Sugar)
   return new Pancake("Yum");
  if(s=>s.Salt)
   return new Omlette("Yay");
 }
 /*.....
 ......
 .....
 loads of ifs and buts and else*/
}

I want to get rid of this mess and take a more data structure and OO route. Even the code sample i have provided is not as horrendous as it is. I looked at the specification pattern and found it applicable. Any ideas how to improve the code.

EDIT: Now that I realize it, I might even like to implement a method of this signature:

List<IRecipe> WhatAllCanBeCooked(IEnumerable<Ingredients> stuff, Cook cook);
A: 

I think what that block of code is essentially trying to accomplish is linking recipes to ingredients within that recipe. One approach would be to include a list of ingredients on the recipe class itself, and then compare that against the list of ingredients passed in, like so:

public interface IRecipe {
   IEnumerable<Ingredient> Ingredients { get; }
}

public class Omlette : IRecipe {
   public IEnumerable<Ingredient> Ingredients { 
      get {
         return new Ingredient[]{new Ingredient("Salt"), new Ingredient("Egg")};
      }
   }
}

// etc. for your other classes.

IRecipie FindRecipiesYouCanMake(IEnumerable<Ingredientes> stuff, Cook cook)
{
    var query = Recipes.Where(r => !r.Ingredients.Except(stuff).Any());
    return query.First();
}

This assumes that you have a collection somewhere of all your recipes. It should be simple enough to set up a static list of those or pull from a database however.

The Linq query in question looks for any recipes where all ingredients passed in stuff are present in the list of ingredients (or, as it's stated, there are no ingredients in Ingredients that are not in stuff). This also might decrease the need to have subclasses for Recipes, which seems a bit odd (although for all I know there's additional reasons you'll require this)

Ryan Brunner
+1  A: 

Some ideas:

  • use decision tables

  • use the strategy pattern. This helps you to encapsulate a group of actions or parameters belonging together in different concrete classes. Once you have decided which strategy to use, you don't need any 'ifs' any more to dispatch between the strategies.

EDIT: some additional ideas:

  • start "small": most often, just simple refactoring to smaller, well-named, reusable functions will help you to reduce the if-else-if-else-soup. Sometimes, a simple, well named boolean variable does the trick. Both are examples for refactorings you will find in Fowler's book "Refactoring".

  • think "big": if you have really lot of complex business rules, constructing a "domain specific language" is an option that can sometimes be the right way of getting the complexity down. You will find lots of material on this topic just by googling for it. Citing David Wheeler All problems in computer science can be solved by another level of indirection.

Doc Brown
+1  A: 

ORIGIINAL POST -- Martin Fowler has solved this problem for you... its called the Specification pattern.
http://en.wikipedia.org/wiki/Specification_pattern

UPDATED POST --

Consider using the Composite Specification Pattern when:

  • You need to select a subset of objects based on some criteria,
  • You need to check that only suitable objects are used for a certain role, or
  • You need to describe what an object might do, without explaining the details of how the object does it

The true power of the pattern is in the ability to combine different specifications into composites with AND, OR and NOT relationships. Combining the different specifications together can be done at design time or runtime.

Eric Evan book on Domain Driven Design has an excellent example of this pattern (the Shipping Manifest)

This is the Wiki link:

http://en.wikipedia.org/wiki/Specification_pattern

At the bottom of the wiki link is this PDF link:

http://martinfowler.com/apsupp/spec.pdf

fremis
This pattern shifts the coding/changing of a chain of business rules from the developer to the user, giving such systems a very high degree of flexibility. This may reduce the complexity of the code, but on the other hand, may end up in chain of composite objects with the same degree of complexity the original code had before. So I doubt it is a real solution to the question, it just changes the responsibility for maintaining the business rules.
Doc Brown
Doc, I agree with you if they choose the parameterized specification. However, like any design pattern it can vary in implementaion. There are 3 main flavors of Specifications: 1. Hard Coded Specifications (AKA GOF Strategy Pattern) 2. Parameterized Specification , and 3. Composite Specification (I would recommend this route for the user) This link supports my statements: http://martinfowler.com/apsupp/spec.pdf
fremis
@fremis: the 2nd link you gave uses the term 'Specification' in a different, more generalized way than the Wikipedia link you gave in your answer (that one was only about 'composite specification'). If you mean "Specification" in this more generalized way, you should have changed the link in your answer. Nethertheless, IMHO using 'composite specification' shifts the problem from the coder to the user (so perhaps it solves it for the coder :-) ). Let the OP decide if this is what he finds useful.
Doc Brown
+2  A: 

I would delegate this logic to the individual IRecipie classes:

if (Pancake.CanBeMadeBy(stuff, cook)) {
    return new Pancake("Yum");
}
....


public class Pancake: IRecipe {
    ...
    public static bool CanBeMadeBy(IEnumerable<Ingredientes> stuff, Cook cook) {
        return stuff.Any(s=>s.Eggs && s.Flour && s.Sugar) && cook.DinerCook;
    }

}

Edit in response to comment

To find all the recipes that can be cooked, just do something like this:

List<IRecipe> results = new List<IRecipe>();

if (Pancake.CanBeMadeBy(stuff, cook)) {
    results.Add(new Pancake("Yum");
}
....

Edit 2 Alternatively, if you store a list of all possible recipes somewhere, you can turn CanBeMadeBy into an instance method instead of a static, and do this:

List<IRecipe> allRecipes = // all possible recipes
...
return allRecipes.Where(r => r.CanBeMadeBy(stuff, cook));
Gabe Moothart
I think there is a difference in perspective, i am not trying to find out if a pancake can be made, instead I want to see what all recipies can be created with this item and this cook.
Perpetualcoder