views:

623

answers:

5

In the ContainsIngredients method in the following code, is it possible to cache the p.Ingredients value instead of explicitly referencing it several times? This is a fairly trivial example that I just cooked up for illustrative purposes, but the code I'm working on references values deep inside p eg. p.InnerObject.ExpensiveMethod().Value

edit: I'm using the PredicateBuilder from http://www.albahari.com/nutshell/predicatebuilder.html

public class IngredientBag
{
    private readonly Dictionary<string, string> _ingredients = new Dictionary<string, string>();

    public void Add(string type, string name)
    {
        _ingredients.Add(type, name);
    }

    public string Get(string type)
    {
        return _ingredients[type];
    }

    public bool Contains(string type)
    {
        return _ingredients.ContainsKey(type);
    }
}

public class Potion
{
    public IngredientBag Ingredients { get; private set;}
    public string Name {get; private set;}        

    public Potion(string name) : this(name, null)
    {

    }

    public Potion(string name, IngredientBag ingredients)
    {
        Name = name;
        Ingredients = ingredients;
    }

    public static Expression<Func<Potion, bool>> 
        ContainsIngredients(string ingredientType, params string[] ingredients)
    {
        var predicate = PredicateBuilder.False<Potion>();
        // Here, I'm accessing p.Ingredients several times in one 
        // expression.  Is there any way to cache this value and
        // reference the cached value in the expression?
        foreach (var ingredient in ingredients)
        {
            var temp = ingredient;
            predicate = predicate.Or (
                p => p.Ingredients != null &&
                p.Ingredients.Contains(ingredientType) &&
                p.Ingredients.Get(ingredientType).Contains(temp));
        }

        return predicate;
    }

}


[STAThread]
static void Main()
{
    var potions = new List<Potion>
 {
  new Potion("Invisibility", new IngredientBag()),
    new Potion("Bonus"),
    new Potion("Speed", new IngredientBag()),
    new Potion("Strength", new IngredientBag()),
    new Potion("Dummy Potion")
 };

    potions[0].Ingredients.Add("solid", "Eye of Newt");
    potions[0].Ingredients.Add("liquid", "Gall of Peacock");
    potions[0].Ingredients.Add("gas", "Breath of Spider");

    potions[2].Ingredients.Add("solid", "Hair of Toad");
    potions[2].Ingredients.Add("gas", "Peacock's anguish");

    potions[3].Ingredients.Add("liquid", "Peacock Sweat");
    potions[3].Ingredients.Add("gas", "Newt's aura");

    var predicate = Potion.ContainsIngredients("solid", "Newt", "Toad")
        .Or(Potion.ContainsIngredients("gas", "Spider", "Scorpion"));

    foreach (var result in 
                from p in potions
                where(predicate).Compile()(p)
                select p)
    {
        Console.WriteLine(result.Name);
    }
}
+2  A: 

Can't you simply write your boolean expression in a separate static function which you call from your lambda - passing p.Ingredients as a parameter...

private static bool IsIngredientPresent(IngredientBag i, string ingredientType, string ingredient)
{
    return i != null && i.Contains(ingredientType) && i.Get(ingredientType).Contains(ingredient);
}

public static Expression<Func<Potion, bool>>
    ContainsIngredients(string ingredientType, params string[] ingredients)
{
    var predicate = PredicateBuilder.False<Potion>();
    // Here, I'm accessing p.Ingredients several times in one 
    // expression.  Is there any way to cache this value and
    // reference the cached value in the expression?
    foreach (var ingredient in ingredients)
    {
        var temp = ingredient;
        predicate = predicate.Or(
            p => IsIngredientPresent(p.Ingredients, ingredientType, temp));
    }

    return predicate;
}
Fake Jim
I can do that, but like I said this is a fairly trivial example. The example I'm working on uses deeply nested values, often several levels down and obtained via expensive method calls. It seems like I'm going to have to resort to splitting up the Predicates though...
ilitirit
My mistake. I have deleted my erroneous answer.
Chris Ammerman
A: 

I would say no in this case. I assume that the compiler can figure out that it uses the p.Ingredients variable 3 times and will keep the variable closeby on the stack or the registers or whatever it uses.

omouse
A: 

Turbulent Intellect has the exactly right answer.

I just want to advise that you can strip some of the nulls and exceptions out of the types you are using to make it friendlier to use them.

    public class IngredientBag
    {
      private Dictionary<string, string> _ingredients = 
new Dictionary<string, string>();
      public void Add(string type, string name)
      {
        _ingredients[type] = name;
      }
      public string Get(string type)
      {
        return _ingredients.ContainsKey(type) ? _ingredients[type] : null;
      }
      public bool Has(string type, string name)
      {
        return name == null ? false : this.Get(type) == name;
      }
    }

    public Potion(string name) : this(name, new IngredientBag())    {    }

Then, if you have the query parameters in this structure...

Dictionary<string, List<string>> ingredients;

You can write the query like this.

from p in Potions
where ingredients.Any(i => i.Value.Any(v => p.IngredientBag.Has(i.Key, v))
select p;

PS, why readonly?

David B
The readonly modifier is a hint to the compiler that the _ingredients variable is only written to once. It helps the compiler to optimize the code.
ilitirit
+2  A: 

Have you considered Memoization?

The basic idea is this; if you have an expensive function call, there is a function which will calculate the expensive value on first call, but return a cached version thereafter. The function looks like this;

static Func<T> Remember<T>(Func<T> GetExpensiveValue)
{
    bool isCached= false;
    T cachedResult = default(T);

    return () =>
    {
        if (!isCached)
        {
            cachedResult = GetExpensiveValue();
            isCached = true;
        }
        return cachedResult;

    };
}

This means you can write this;

    // here's something that takes ages to calculate
    Func<string> MyExpensiveMethod = () => 
    { 
        System.Threading.Thread.Sleep(5000); 
        return "that took ages!"; 
    };

    // and heres a function call that only calculates it the once.
    Func<string> CachedMethod = Remember(() => MyExpensiveMethod());

    // only the first line takes five seconds; 
    // the second and third calls are instant.
    Console.WriteLine(CachedMethod());
    Console.WriteLine(CachedMethod());
    Console.WriteLine(CachedMethod());

As a general strategy, it might help.

Steve Cooper
That's something I would usually do, but in this case I don't have access to the source code. The example I gave was just for illustrative purposes
ilitirit
A: 

Well, in this case, if you can't use Memoization, you're rather restricted since you can really only use the stack as your cache: You've got no way to declare a new variable at the scope you'll need. All I can think of (and I'm not claiming it will be pretty) that will do what you want but retain the composability you need would be something like...

private static bool TestWith<T>(T cached, Func<T, bool> predicate)
{
    return predicate(cached);
}

public static Expression<Func<Potion, bool>>
    ContainsIngredients(string ingredientType, params string[] ingredients)
{
    var predicate = PredicateBuilder.False<Potion>();
    // Here, I'm accessing p.Ingredients several times in one 
    // expression.  Is there any way to cache this value and
    // reference the cached value in the expression?
    foreach (var ingredient in ingredients)
    {
        var temp = ingredient;
        predicate = predicate.Or (
            p => TestWith(p.Ingredients,
                i => i != null &&
                     i.Contains(ingredientType) &&
                     i.Get(ingredientType).Contains(temp));
    }

    return predicate;
}

You could combine together the results from multiple TestWith calls into a more complex boolean expression where required - caching the appropriate expensive value with each call - or you can nest them within the lambdas passed as the second parameter to deal with your complex deep hierarchies.

It would be quite hard to read code though and since you might be introducing a bunch more stack transitions with all the TestWith calls, whether it improves performance would depend on just how expensive your ExpensiveCall() was.

As a note, there won't be any inlining in the original example as suggested by another answer since the expression compiler doesn't do that level of optimisation as far as I know.

Fake Jim