foreach(... in ...) or .ForEach(); that is the question

Question

This is a question about coding for readability.

I have an XDocument and a List<string> of the names of the elements that contain sensitive information that I need to mask (replace with underscores in this example).

XDocument xDoc;
List<string> propertiesToMask;

This can be written in two ways, using traditional foreach loops, or using .ForEach methods with lamba syntax.

foreach (string propertyToMask in propertiesToMask)
{
    foreach (XElement element in xDoc.Descendants(propertyToMask))
    {
        element.SetValue(new string('_', element.Value.Length));
    }
}

or

propertiesToMask
    .ForEach(propertyToMask => xDoc.Descendants(propertyToMask).ToList()
        .ForEach(element => element.SetValue(new string('_', element.Value.Length))));

Which approach do you think is the most readable, and why? If you prefer the second example, how would you present it for maximum readability?

Answer 1

Eric Lippert has a good entry about this on his blog. To summarize, the very task done by ForEach is to produce side effects which may not be a desirable thing to do with the functional style of programming in C#.

Answer 2

foreach (string propertyToMask in propertiesToMask)
{
    foreach (XElement element in xDoc.Descendants(propertyToMask))
    {
        element.SetValue(new string('_', element.Value.Length));
    }
}

Because the spacing makes it very simple to scan. The second is far to cluttered and I have to actually read it.

Answer 3

I strongly prefer the first, for three reasons.

First, it's more efficient (in the second, you have extra ToList() calls).

Secondly, it's more readable, in my opinion.

Finally, I'd recommend reading Eric Lippert's blog post on this subject. There are philosophical reasons to avoid List<T>.ForEach, as it's entire purpose is to cause side effects, even though it has a functional style.

Answer 4

Here's a really subjective answer:

I don't really agree with the philosophical reasoning behind not liking .ForEach. Maybe it's my lack of a computer science background, I don't know.

To me, the second set of code is easier to read and looks much less jumbled. As others have mentioned, the ToList() is kind of unfortunate, but it still just looks better to me.

I like Daniel Brückner's solution even better. It seems better than either of the other proposed solutions.

Answer 5

The first one can be altered while the debugger is running and Visual Studio allows you to continue debugging. After altering the .ForEach variant you have to restart the debugging session and recompile because it contains a lambda-expression (VS 2008)

Answer 6

The traditional way has the big advantage that it can be easily debugged. But I would personally prefer the ForEach() approach in this case. The circumstance that it is hard to debug code written in a fluent still is in my opinion a flaw of the available tools, not the coding style. In my personal experience the error rate in such methods is very low, hence it is no really big problem.

I would write some extension methods yielding the following code.

propertiesToMask
   .SelectMany(property => document.Descendants(property))
   .ForEach(element => element.MaskValue());