5
\$\begingroup\$

When I read this answer, which has code:

linqObject.Where((obj, index) => {
  DoWork(obj, index);
  return true;
}).ToArray(); //MUST CALL ToArray() or ToList() or something to execute the lazy query, or the loop won't actually execute

My initial reaction was similar to Mark Sowul's comment:

"Wastes memory (allocating a list or array) and risks bugs (if you don't call ToList or ToArray). Why?".


But on further thought, what is a concise approach, avoiding the downsides of the above?
My first attempt:

int index = 0;
foreach (var item in someEnumerable) {
    DoWork(item, index);
    index++;
}

Compared to calling a Linq operator, that seems verbose, low-level. I'm defining and manipulating an index, whereas Linq has an Operator that "almost" does what I want - takes care of the index tracking for me.

Personally, I would write the explicit foreach loop - but maybe that is only because I've been programming a long time, and am comfortable with that syntax.


Is there a more concise solution, that doesn't have the downsides of the first version shown (Where / ToArray)?

I'm trying to get comfortable with encapsulating such stuff into methods, often by using Action<> or Func<>, which are newer [to me] than the style of programming that became my habit. So that instead of repeating similar snippets of code, I just call a method.

As an aside, Linq is also newer than my habits. Linq is concise, but not in a way that I find helpful; I find loops clearer to read; a better separation of the aspects of what is happening. But I'm open to seeing situations where its expressive power is appropriate.
[OFF-TOPIC: In contrast, I spent some time with F#, and found it to be fantastic. I LOVE the combination of functional with OO; just not the way Linq jams a handful of functional constructs into C#'s syntax. But this question is about making good use of C#, so all advice is appreciated.]

I think this becomes:

public static void ForEachWithIndex<T>(IEnumerable<T> e, Action<int, T> a)
{
    int index = 0;
    foreach (T v in e) {
        a(index, v);
        index++;
    }
}

which might be used like this:

ForEachWithIndex(e, (int index, T item) => {
    // Do Work based on index and item.
});

Or if an appropriate "DoWork" method already exists, simply:

ForEachWithIndex(e, DoWork);

Comments? Improvements, or alternatives?
Is there a way to use Linq more concisely/efficiently than in the original code sample?


NOTE: I did not make this an "extension method" on "IEnumerable", due to comments people made in stackoverflow threads regarding "ForEach" as a possible extension method: dislike of a syntax that looks similar to a Linq operator - which traditionally don't have side-effects - for a method that clearly DOES have side-effects; indeed is all about the side-effects.

\$\endgroup\$
6
\$\begingroup\$
linqObject.Where((obj, index) => {
  DoWork(obj, index);
  return true;
}).ToArray();

Never ever! This is the worst possible abuse of an API. I 100% agree with the comment.


int index = 0;
foreach (var item in someEnumerable) {
    DoWork(item, index);
    index++;
}

Yes, this is the way it should be, although this can be shortened... keep reading.


public static void ForEachWithIndex<T>(IEnumerable<T> e, Action<int, T> a)
{
    int index = 0;
    foreach (T v in e) {
        a(index, v);
        index++;
    }
}

This is better but the naming is still quite wrong and the Action parameters are in the wrong order. They should be incremental. This means that T is always there so it should be first, then any other parameters may follow, like the index.


I'm not sure what's wrong with using normal loops where necessary but if you really want to encapsulate them then...


Try to be consistant with the LINQ API and don't invent new method suffixes like WithIndex. Use overloads instead. In fact you can implement it once and call the core method with other overloads.

Notice the order of Action parameters. It's consistant with current APIs. T first then index. There is also no explicit parameter checking because it uses an empty collection if the source is null and the ? operator to not call the delegate if this one is null. You can also throw if you want.

public static class Loop
{
    public static void ForEach<T>(IEnumerable<T> source, Action<T, int> action)
    {
        var i = 0;
        foreach (var item in source ?? Enumerable.Empty<T>()) action?.Invoke(item, i++);
    }

    public static void ForEach<T>(IEnumerable<T> source, Action<T> action)
    {
        ForEach(source, (x, i) => action(x));
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks, very informative. Particularly useful was your detailed explanation of why to pick the parameter order, and showing two related overloads. One question: why create an empty enumeration, given that the result will lead to no action anyway? I see that would be a useful habit, if pipelining the result to another method that takes an enumerable, but in this case, I think parameter check would be more readable: var i = 0; if (source != null) foreach(var item in source) action(item, i++); \$\endgroup\$ – ToolmakerSteve Mar 3 '17 at 20:51
  • \$\begingroup\$ Re "using normal loops where necessary" - agreed, I wouldn't do this just to do it. And perhaps this case is so simple that it wouldn't make sense. I wanted to see the alternatives for this simple case, then I can extrapolate to other situations, where it is worth inventing a method, to "name" what is being done. \$\endgroup\$ – ToolmakerSteve Mar 3 '17 at 20:56
  • \$\begingroup\$ why create an empty enumeration well, no particular reason, I just liked it this way ;-) but you're right, an if is equally good. \$\endgroup\$ – t3chb0t Mar 3 '17 at 21:02
3
\$\begingroup\$

I think the best way of doing this with standard Linq is

enumerable.Aggregate(0, (idx, obj) => { DoWork(obj, idx); return idx + 1});

However, this is obviously a semantic abuse. IMO the best solution is a ForEach extension method. The argument against it seems to put purism above pragmatism, but I would argue that there's a purist case that if you're writing with a functional style you should use functional control structures.

\$\endgroup\$
  • \$\begingroup\$ Thanks - I appreciate seeing how clean one can get with Linq - your solution avoids the issues with the original code. And I agree that is still semantic abuse. Would you say the ForEachWithIndex method I show is a good solution? You would do something similar, and make it an extension method? \$\endgroup\$ – ToolmakerSteve Mar 3 '17 at 7:22
  • 1
    \$\begingroup\$ @ToolmakerSteve, I'd just call it ForEach and let the signature tell you that it's with an index, but yes. \$\endgroup\$ – Peter Taylor Mar 3 '17 at 8:12
2
\$\begingroup\$

I think unless you're going to be using it a lot I would avoid creating a method. It hides the intention behind a unnecessary layer of indirection.

You could save a bit of work by taking advantage of the Select overload that provides the index:

foreach(var item in enumerable.Select((v, i) => new { Index = i, Value = v }))
{
    DoWork(item.Value, item.Index);
}

The anonymous type creation could be shortened further, to simply (v, i) => new { i, v }, but this might not be as clear.

\$\endgroup\$
  • \$\begingroup\$ Ah, thanks - that is a useful variant of Select. However, "new" means a small object is created for each element? I guess the only way to avoid that cost is to embed "DoWork" inside the Select - but the result would not be as readable, so never mind. \$\endgroup\$ – ToolmakerSteve Mar 3 '17 at 20:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.