1

I saw this loop in test code:

foreach ( StuffId Id in Result.GetIdList() )
{
    if ( Id.Level == 3 )
    {
        Level3Id = Id.ToString();
    }
    if ( Id.Level == 5 )
    {
        Level5Id = Id.ToString();
    }
}

Other tests imply that either there is only one Id for each level or when there are multiples for each level then the Id will be the same.

Being slightly obsessed with LINQ right now, I first refactored to this:

IEnumerable<StuffId> Ids = Result.GetIdList();

Level3Id = Ids.Where( x => x.Level == 3 ).First().Id.ToString();
Level5Id = Ids.Where( x => x.Level == 5 ).First().Id.ToString();

Then the code repetition bothered me so I refactored to this:

IEnumerable<StuffId> Ids = Result.GetIdList();
Func<int,string> IdFromLevel = 
    level => Ids.Where( x => x.Level == level ).First().Id.ToString();

Level3Id = IdFromLevel(3);
Level5Id = IdFromLevel(5);

A colleague wondered why I didn't use a method in place of the delegate. My reasoning is a method would be slightly more 'messy' because I'd have to additionally pass in the collection and that using a delegate is no big deal for a simple test (for which terse, readable and no branching are good qualities).

I had a look on SO, of course, and found this seemingly relevant question:

C#: Func<> instead of methods?

where the consensus seems to favour a method over a delegate. Does the same apply in my case?

Community
  • 1
  • 1
petemoloy
  • 695
  • 1
  • 5
  • 9
  • 1
    yes its better to go for method ...for the same reason mention in answer of that question.. – Pranay Rana Apr 17 '13 at 10:10
  • 2
    The `foreach` code uses the value of the last element, your linq code the first one. So they're not equivalent, unless there is only a single match, in which case your LINQ code should use `Single` instead of `First`. – CodesInChaos Apr 17 '13 at 10:51
  • 1
    Your LINQ is also less efficient than the original loop, as it enumerates the collection twice. – Alex Apr 17 '13 at 11:11
  • 1
    @CodesInChaos: I acknowledged your point in my question: "tests imply that either there is only one Id for each level or when there are multiples for each level then the Id will be the same". I could OrderBy the index in descending order before using First but will be the same (guaranteed by other tests). – petemoloy Apr 18 '13 at 07:37
  • @Pranay Rana: OK so you are discounting the things I think weigh in favor of the lambda (terse, readable, no branching, slight obsession with lambda ) but _*why*_ do you discount them? The answer linked is not test code and therefore has different intent. – petemoloy Apr 18 '13 at 07:40
  • @Alex G: it may be less efficient in relative terms but I assure you that in absolute terms it makes no real difference to the execution time of the test suite! – petemoloy Apr 18 '13 at 07:43

3 Answers3

0

It is a question of reuse. If you are using methods that might be reusable in other cases, you should define them seperately.
But if you have only short statements that additionally vary, you should stick with anonymous functions/lambda-expressions, these might result in a better runtime-behavior.

From SO:

There is no advantage in the code you posted. In your code, using the delegate just adds complexity as well as an extra runtime cost - so you're better off just calling the method directly.

However, delegates have many uses. "Passing around" to other methods is the primary usage, though storing a function and using it later is also very useful.

LINQ is built on top of this concept entirely. When you do:

var results = myCollection.Where(item => item == "Foo");

You're passing a delegate (defined as a lambda: item => item == "Foo") to the Where function in the LINQ libraries. This is what makes it work properly.

Community
  • 1
  • 1
bash.d
  • 13,029
  • 3
  • 29
  • 42
  • OK, does *that* question apply :) "There is no advantage in the code you posted" -- presumably that applies to the other question. In *my* question I stated the advantages *to me* i.e. terse, no branching, readable (due to being 'self-contained'). – petemoloy Apr 17 '13 at 10:41
  • "With the delegate you could inline your implementation and have access to the variables in scope" -- that too sounds good to me (I point I kind of made). – petemoloy Apr 17 '13 at 10:43
  • This is why I wrote "From SO:" it is a quote – bash.d Apr 17 '13 at 10:46
0

Not sure about pros and cons, but reading these articles might help you to take a better decision:

http://msdn.microsoft.com/en-in/library/bb549151.aspx

http://msdn.microsoft.com/en-us/library/orm-9780596516109-03-09.aspx

http://msdn.microsoft.com/en-in/library/98dc08ac.aspx

S2S2
  • 8,322
  • 5
  • 37
  • 65
0

I would go with the first block:

foreach (StuffId Id in Result.GetIdList())
{
    if (Id.Level == 3)
    {
        Level3Id = Id.ToString();
    }
    if (Id.Level == 5)
    {
        Level5Id = Id.ToString();
    }
}

This does loop the collection only once. I see you don't worry about performance here, but for me its not a matter of performance or optimization. It's a matter of doing something logically correct way. Why do something twice if it could be in one step (provided readability is not hurt).

The added benefit is that you don't have the dilemma between Func<,> and method, and the related complexities. As far as number of characters or ease of typing goes, its almost the same, except you're writing it horizontally (in the second case) than vertically. You can even write the above in two lines inside the foreach block.

If you're bent on writing the two actions separately, I would make the choice based on whether the function has relevance outside the scope of current method in this case. It seems to me that its a trivial predicate which is relevant only for the two assignments. So I like:

var Ids = Result.GetIdList();
Func<int, string> IdFromLevel = level => Ids.Single(x => x.Level == level).Id.ToString();

Level3Id = IdFromLevel(3);
Level5Id = IdFromLevel(5);

Prefer Single here over First..

nawfal
  • 70,104
  • 56
  • 326
  • 368