Is it wise to use LINQ to replace loops?

Question

Now that we have tremendous functionality thanks to LINQ, I'm wondering which syntax is preferable. For example, I found the following method (just thought it was a good example):

foreach (FixtureImageServicesData image in _fixture.Images)
{
    if (image.Filename != _selectedFixtureImage.Filename && image.IsPrimary)
    {
        image.IsPrimary = false;
        image.IsChanged = true;
    }
}

If we were to convert it to a LINQ approach, it would look like this (not tested):

_fixture.Images.Where(x => x.Filename != _selectedFixtureImage.Filename && x.IsPrimary).ForEach(x => { x.IsPrimary = false; x.IsChanged = true; });

Which would you would rather see and maintain? Is this crazy or genius?

Answer 1

Using a ForEach extension method is okay, but there's an intermediate approach:

// Rename 'query' to something meaningful :)
var query = _fixture.Images
                    .Where(image => _selectedFixtureImage.Filename 
                                    && image.IsPrimary);

foreach (FixtureImageServicesData image in query)
{
    image.IsPrimary = false;
    image.IsChanged = true;
}

If you do use a ForEach method, I'd definitely format it in multiple lines:

_fixture.Images
    .Where(image => _selectedFixtureImage.Filename && image.IsPrimary)
    .ForEach(image => { image.IsPrimary = false; image.IsChanged = true;});

(Reduced indentation to avoid wrapping...)

or:

_fixture.Images
        .Where(image => _selectedFixtureImage.Filename && image.IsPrimary)
        .ForEach(image => { image.IsPrimary = false; 
                            image.IsChanged = true; });

You might even want to extract the "making non-primary" bit into a separate method, at which point you'd have:

_fixture.Images
        .Where(image => _selectedFixtureImage.Filename && image.IsPrimary)
        .ForEach(MakeNonPrimary);

Answer 2

Anything that reduces vertical or horizontal complexity is a plus for me. Additionally, the Linq is much more descriptive of what you are trying to accomplish.

Answer 3

This reminds me a bit of "should I use Regex or standard string functions" or "should I use XSLT/XPATH to transform XML or use SelectSingleNode()".

The first option (ie. Regex/ XSLT/ Linq) is often considered more elegant and powerful by anyone who has spent some time learning it.

While for everyone else it appears to be less readable and more complex when compared to the second option (ie. string functions, SelectSingleNode(), simple foreach loop).

In the past I've been accused of over-complicating things by using Regex and XSLT/XPATH in my design.

Just recently I was accused of being "scared of change" by preferring simple foreach (and even for) loops in many situations over Linq Where, Foreach etc.

I soon realised that the people in both cases who said this were the sort who feel that there is "the one way" to do everything.

Whereas I've always found it's much smarter to consider each situation on its merits and choose the right tool for the job. I just ignore them and continue with my approach ;)

For that situation you describe, the first option is more preferable to me. However I probably would use the Linq approach if my team were all competent in Linq, and we had coding guidelines to avoid big one-liners (by splitting it them up)