CODEWITHSUNDEEP
c#winformslinqforeachnested
shipr
shipr

Reputation: 2839

How/should I replace nested foreach with one linq query?

Trying to set a property on certain labels contained in a Windows Forms GroupBox, I wrote the loops below. This works fine, but I don't like it due to its (I think unnecessary) double foreach nesting.

I have tried to rewrite this to be more clear, use only one foreach, and a combined Linq expression, but all my attempts fail at runtime with a CastException, either from GroupBox to Label or vice versa.

Is there a more clear, more efficient, or more readable way to write this loop construct?

        foreach (var gb in (from Control c in this.Controls where c is GroupBox select c))
            foreach (Label tlbl in (from Control a in gb.Controls
                                    where a is Label && a.Tag != null && a.Tag.ToString() == "answer"
                                    select a))
                tlbl.ForeColor = (tlbl.Name.Replace("lbl", "") == rb.Name) ? afterSelectColor : beforeSelectColor;

Readability is my highest goal. With that in mind, is it worth trying to rewrite it?

Upvotes: 2

Views: 1520

Answers (2)

It'sNotALie.
It'sNotALie.

Reputation: 22794

I'd recommend you do the editing in a foreach, as LINQ is not meant to cause side effects. Like this:

foreach (Label tlbl in (this.Controls.OfType<GroupBox>()
    .SelectMany(g => g.Controls.Cast<Control>()).OfType<Label>()
    .Where(a => a.Tag != null && a.Tag.ToString() == "answer")))
{
    tblb.ForeColour = tlbl.Name.Replace("lbl", "") == rb.Name ? afterSelectColor : beforeSelectColor;
}

Note SelectMany in here. That's how you translate nested foreach loops to LINQ, as it is pretty much just a nested foreach loop.

Upvotes: 4

Fede
Fede

Reputation: 44038

Controls.OfType<GroupBox>
        .SelectMany(x => x.Controls.OfType<Label>)
        .Where(x => x.Tag != null && x.Tag.ToString() == "answer")
        .ToList()
        .ForEach(x => x ForeColor = (x.Name.Replace("lbl", "") == rb.Name) ? afterSelectColor : beforeSelectColor);

Notice that the ForEach() method is not part of LINQ. It's a member of the List<T> class. LinQ is a functional feature, therefore it's methods are not supposed to affect the source objects. That's why there's no ForEach() in LINQ.

Edit:

If you don't like the use of List<T>.ForEach(), then you may also do it this way:

var labels = Controls.OfType<GroupBox>
                     .SelectMany(x => x.Controls.OfType<Label>)
                     .Where(x => x.Tag != null && x.Tag.ToString() == "answer")

foreach (var label in labels)
{
  label.ForeColor = (label.Name.Replace("lbl", "") == rb.Name) ? afterSelectColor : beforeSelectColor);
}

While this separates the code in 2 statements, it improves readability a lot compared to other approaches.

Edit2:

Since this is winforms, the Control.Controls collection is not an IEnumerable<T>, but an IEnumerable therefore the OfType<T> must be included inside the SelectMany() expression. Corrected that.

Upvotes: 1

Related Questions