Refactor a foreach with if - else if structure to LINQ

Question

I have this foreach loop:

var includedElements = new HashSet<int>();
foreach(var e in elements)
{
    var include = false;
    if(isTable(e.Key))
    {
        if(tables.ContainsKey(e.Key)
        {
            if(tables[e.Key].Elements
               .Any(subElem => shouldBeIncluded(subElem.Key) ) )
            {
                include = true;
            }
        }
    }
    else if(shouldBeIncluded(e.Key))
    {
        include = true;
    }
    if(include){
        includedElements.Add(e.Key);
        DoSomeMoreStuff(e);
    }
}

I have tried to refactor this to LINQ:

var query = 
    from e in elements
    where 
    ( 
        isTable(e.Key)
        && tables.ContainsKey(e.Key)
        && tables[e.Key].Elements
                .Any(subElem => shouldBeIncluded(subElem.Key) )
    ) || (
        !isTable(e.Key)
        && shouldBeIncluded(e.Key)
    )
    select e;
foreach(e in query){
    includedElements.Add(e.Key);
    DoSomeMoreStuff(e);
}

What I'm not sure of is the or clause here. In my head I need to include !isTable(e.Key) to handle the outer if/else if structure.
Am I thinking right with my refactoring? Are these two code examples resulting in the same logical functionality?

Is it a way I can get away with only one call to isTable? As I have it now I need to call it inverted on the other side of the ||.

xanatos · Accepted Answer · 2011-10-19 12:28:57Z

5

Yes you are right. This if isTable doesn't have side effects (doesn't do anything but check something) and is deterministic based on the parameters (so calling it twice with e.Key always result in the same value). Still it could (it could be a premature optimization... Who knows?) probably be better to keep it more similar to the original if and use a ternary operator (? :) so not to recheck isTable

var query = 
    from e in elements
    where 
        isTable(e.Key) ? 

            tables.ContainsKey(e.Key) && tables[e.Key].Elements
                .Any(subElem => shouldBeIncluded(subElem.Key) ) 
        :

            shouldBeIncluded(e.Key)
    select e;

I'll add that if you hate ternary operators, you could use the let keyword:

var query = 
    from e in elements
    let isT = isTable(e.Key)
    where 
        ( isT && tables.ContainsKey(e.Key) && tables[e.Key].Elements
            .Any(subElem => shouldBeIncluded(subElem.Key) ) )
            ||
        ( !isT && shouldBeIncluded(e.Key) )
    select e;

to cache the isTable(e.Key)

edited Oct 19, 2011 at 12:28

answered Oct 19, 2011 at 12:05

xanatos

112k13 gold badges209 silver badges296 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

awe Over a year ago

Great. Although the check on isTable does nothing fancy, it is still a call to check if it exists in a dictionary, which can potentioally be expensive if the dictionary is large. Your solution(s) is exactly what I need.

Heinzi · Accepted Answer · 2011-10-19 12:07:27Z

0

You are correct. The else if implies that the if condition did not match, so

if(A) { 
    if(B) { 
        if(C) { 
            include = true; 
        } 
    } 
}
else if(D) {
    include = true;
}

is equivalent to

if(A) { 
    if(B) { 
        if(C) { 
            include = true; 
        } 
    } 
}

if(!A && D) {
    include = true;
}

which is equivalent to

if ((A && B && C) || (!A && D)) {
    include = true;
}

which is exactly what you have written in LINQ.

answered Oct 19, 2011 at 12:07

Heinzi

173k61 gold badges386 silver badges554 bronze badges

1 Comment

awe Over a year ago

The question was not really about the boolean logic in the if's, but a way to refactor to LINQ without having to call the isTable method twice. Since it is a method, it can potentially be a performance issue if the method is heavy.

Collectives™ on Stack Overflow

Refactor a foreach with if - else if structure to LINQ

2 Answers 2

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related