c# lamba expression - add delegate results to generic list

Question: I have just wrote my first code using c# lamba expressions. It works, but I am not sure if this is the best way to do it. Any recommendations on a better way to do the lambda expression? It seems odd to have numerous lines of code in the expression like I do below.

Background: I have a generic list of delegates. Each delegate function returns an enum value indicating what happened in the function. Upon evaluation of the delegate, I need to add the enum to a List if it was not a specific enum value.

Disclaimer: Code here is very generic, the real code actually does stuff in the delegates to determine the return value!

---

class Class1
{
 public enum WhatHappened
 {
  ThingA,
  ThingB,
  Nothing
 }

 private delegate WhatHappened del();

 public static List<WhatHappened> DoStuff()
 {
  List<del> CheckValues = new List<del>();

  List<WhatHappened> returnValue = new List<WhatHappened> { };

  CheckValues.Add(delegate { return method1(); });
  CheckValues.Add(delegate { return method2(); });

  CheckValues.ForEach(x =>
  {
   WhatHappened wh = x();
   if (wh != WhatHappened.Nothing)
    returnValue.Add(wh);
  });

  return returnValue;

 }

 private static WhatHappened method1()
 {
  return WhatHappened.Nothing;
 }

 private static WhatHappened method2()
 {
  return WhatHappened.ThingA;
 }

}

Note: I originally had the lambda like adding all the items (see below), then removing the ones I didn't want (WhatHappened.Nothing).

CheckValues.ForEach(x => returnValue.Add(x()));

From stackoverflow

• In my opinion, based on the example, it looks fine. You could refactor even more by replacing:

  CheckValues.Add(delegate { return method1(); });
  CheckValues.Add(delegate { return method2(); });
  

  with:

  CheckValues.Add(() => WhatHappened.Nothing);
  CheckValues.Add(() => WhatHappened.ThingA);
  

• You can go lambda all the way by chaining Select (map) and Where (filter) instead of multiple FOR loops and IF statements

  // get results from the list of functions
  var results = CheckValues.Select(x => x());
  
  // filter out only the relevant ones.
  var returnValues = results.Where(x => x != WhatHappened.Nothing);
  

  Basically, you should think more declaratively instead of imperatively when work ing with lambdas. It'll help you write more elegant code.

• It's a bit more idiomatic to write the following instead of using the delegate keyword. It doesn't change the underlying functionality though.

  CheckValues.Add( () => method1() );
  

  Also, I find it more readable to rewrite the ForEach as the following

  CheckValues = CheckValues.
    Select(x => x()).
    Where(wh => wh != WhatHappened.Nothing ). 
    ToList();
  

  Jon Skeet : That won't compile unless you've got a ForEach extension method. I also like having my dots at the start of lines :)

  JaredPar : @Jon, I'm so used to my custom LINQ methods that I forget they don't exist by default. Updated to avoid extension methods i don't own. I honestly go back and forth on dot before or dot after. My normal preference is dot before. But some languages I use require character to be after (PowerShell and VB.Net) so I drift back and forth ;)

• Okay, a few suggestions:

  • Don't call your delegate del. In this case, I'd use Func<WhatHappened> - but if you do want to declare your own delegate type, give it a more descriptive name, and obey the .NET naming conventions.

  • Instead of using anonymous methods to add to CheckValues, you can just use:

    CheckValues.Add(method1);
    CheckValues.Add(method2);
    

    The compiler will convert the method groups into delegates.

  • I'd recommend not using Pascal case for a local variable name to start with.

  • Your collection initializer for returnValues isn't really doing anything for you - just call the List<T> constructor as normal, or use my code below which doesn't require a local variable to start with.
  • If your list really only has two delegates in it, I'd just call them separately. It's a lot simpler.

  • Otherwise you can indeed use LINQ as Jared suggests, but I'd do it slightly differently:

    return CheckValues.Select(x => x())
                      .Where(wh => wh != WhatHappened.Nothing)
                      .ToList();
    

  EDIT: As suggested, here's the full example. It's not quite the same as Denis's though... I've made a couple of changes :)

  public static List<WhatHappened> DoStuff()
  {
      var functions = new List<Func<WhatHappened>> { Method1, Method2 };
  
      return functions.Select(function => function())
                      .Where(result => result != WhatHappened.Nothing)
                      .ToList();
  }
  

  (I'm assuming that method1 and method2 have been renamed to fit the naming convention. Of course in real life I'm sure they'd have more useful names anyway...)

  Mike Ohlsen : I agree with your suggestions, but I think you took the example a little too literally. In the example code I didn't spend time with naming conventions. The real code has more descriptive names.

  Jon Skeet : If you give an example of your code, and ask us what we would do differently, how are we meant to know the difference between mistakes you *do* know about and mistakes you *don't* know about?

  Joel Coehoorn : I think the main thing is to use the existing Func rather than your own delegate type. Then the naming is moot.

  Denis Troller : damn Jon, you beat me to it AND you manage to give better explanations. Does your keyboard read your mind or something ? :)

  Sean Reilly : Lots of great advice in this answer. I would suggest combining it with the completely rewritten example from Denis Troller's answer.

• I would simply use Linq, but that's just me:

  public static List<WhatHappened> DoStuff()
  {
      List<del> CheckValues = new List<del>();
  
      List<WhatHappened> returnValue = new List<WhatHappened>();
  
      CheckValues.Add(method1);
      CheckValues.Add(method2);
  
      return CheckValues
                 .Select(dlg => dlg())
                 .Where( res => res != WhatHappened.Nothing)
                 .ToList();
  }
  

  Note that you can also use Func instead of declaring a Delegate type if you want, but that's less terse in that case. Also, I'd return an IEnumerable<WhatHappened> instead of a List, but it's all about the context.

• Here's a LINQ-free solution:

  return CheckValues
      .ConvertAll<WhatHappened>(x => x())
      .FindAll(y => y != WhatHappened.Nothing);
  

  caveat

  This is not the most performant solution, as it would iterate twice.

• I can't fathom the purpose of the code.. however here goes.
  Used delegate chaining Update: and picked up some Enumerable goodness from Jon n Jared's posts

  private delegate WhatHappened WhatHappenedDelegate();
  
  public static List<WhatHappened> DoStuff()
  {
      WhatHappenedDelegate delegateChain = null;
      delegateChain += method1;
      delegateChain += method2;
  
      return delegateChain.GetInvocationList() 
              .Select(x => (WhatHappened) x.DynamicInvoke())
              .Where( wh => (wh != WhatHappened.Nothing))
              .ToList<WhatHappened>();
  }