views:

1010

answers:

7

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()));
+1  A: 

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);
Jose Basilio
+3  A: 

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.

chakrit
+2  A: 

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();
JaredPar
That won't compile unless you've got a ForEach extension method. I also like having my dots at the start of lines :)
Jon Skeet
@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 ;)
JaredPar
+9  A: 

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...)

Jon Skeet
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.
Mike Ohlsen
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?
Jon Skeet
I think the main thing is to use the existing Func rather than your own delegate type. Then the naming is moot.
Joel Coehoorn
damn Jon, you beat me to it AND you manage to give better explanations. Does your keyboard read your mind or something ? :)
Denis Troller
Lots of great advice in this answer. I would suggest combining it with the completely rewritten example from Denis Troller's answer.
Sean Reilly
+3  A: 

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.

Denis Troller
+1  A: 

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.

Michael Meadows
A: 

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>();
}
Gishu