Nesting conditionals in loops

Question

Given the following code, is there a better way to structure this?

foreach(Thing item in SomeRandomList)
{
    bool firstCondition = CalculateBool(item, someValue);
    bool secondCondition = CalculateBool(item, someOtherValue);

    //General things are done...
    if(firstCondition || secondCondition)
    {
        //semi-specific things are done...
        if(firstCondition)
        {
            //specific things are done...
        } 
        else
        {
            //specific things are done...
        }
    }
}

Also, what if there are more conditions, i.e. 3:

foreach(Thing item in SomeRandomList)
{
    bool firstCondition = CalculateBool(item, someValue);
    bool secondCondition = CalculateBool(item, someOtherValue);
    //imagine as many more as you want.
    bool nthCondition = CalculateBool(item, lastOtherValue);

    //General things are done...
    if(firstCondition || secondCondition || nthCondition)
    {
        //semi-specific things are done...
        if(firstCondition)
        {
            //specific things are done...
        } 
        else if(secondCondition)
        {
            //specific things are done...
        }
        else
        { 
            //specific things are done...
        }
    }
}

Answer 1

Yes: Polymorphism.

Derive Thing's from a common base (or define an Interface that all Thing's implement)

Failing that, move the conditional testing code into a method on Thing.

Answer 2

If you can do the semi-specific things after the specific ones, you could try this:

bool anyTrue = true;

if (firstCondition)
{
    // Specific things
}
else if (secondCondition)
{
    // Specific things
}
else
{
    // Specific things
    anyTrue = false;
}

if (anyTrue)
{
    // Semi-specific things
}

I don't know if it's necessarily better, but it's different...

Alternatively, I'm not all up on C# 3.0 and the fancy new LINQ stuff, but if its expressive enough you could try something like (pseudo-code):

bool[] conditions =
{
    CalculateBool(item, someValue),
    CalculateBool(item, someOtherValue),
    ...
};

if (conditions.AnyTrue)
{
    switch (conditions.IndexOfFirstTrueItem)
    {
        case 0:
            // Specific things
            break;

        case 1:
            // Specific things
            break;

        default:
            // Specific things
            break;
    }
}

Answer 3

I'd use some LINQ to use an intermediate query to help reduce the logic in the loop:

// Get condition in the query.
var query =
    from item in SomeRandomList
    let condition1 = CalculateBool(item, someValue1)
    let condition2 = CalculateBool(item, someValue2)
    ...
    let conditionN = CalculateBool(item, someValueN)
    where
        condition1 || condition2 || ... || conditionN
    select new { Item = item, 
        Condition1 = condition1, Condition1 = condition2, ...
        ConditionN = conditionN };

foreach(var item in query) 
{ 
    //semi-specific things are done... 
    if(firstCondition) 
    { 
        //specific things are done... 
    }  
    else 
    { 
    //specific things are done... 
    } 
}

Hopefully, this will reduce the amount of code in the loop tremendously.

It seems to me though that you have a sequence of values that are being passed to CalculateBool for each item in SomeRandomList. If that is the case, then you could easily generate a query which does a cross join and filter on that:

// Get all valid items across all values.
var query =
    from item in SomeRandomList
    from value in someValues
    where CalculateBool(item, value)
    select { Item = item, Value = value };

// Iterate and process.
foreach (var item in query)
{
    // Use item.Item and item.Value.
    // Specifically, use item.Value to perform a lookup
    // in a map somewhere to determine the course of action
    // instead of a giant switch statement.
}

This would work because your conditionals indicate that you would only have one value set for each item.

Answer 4

I like the approach of having a dictionary of Predicate<T> and their associated Actions. I answered a similar question here:

Coming out of the habit of writing ifs/elseifs for every possible condition

To modify it a bit for your question:

Dictionary<Predicate<Something>, Action> mappings = {{...}}
bool shouldDoAnything = mappings.Keys.Aggregate(true, (accum, condition) => 
    accum || condition);

if (shouldDoAnything)
{
   //do semi-specific things

   foreach(DictionaryEntry<Predicate<Something>, Action> mapping in mappings)
   {
      if (mapping.Key(input))
      {
         mapping.Value(); //do specific things
         break;
      }
    }
}

Answer 5

I might have not been able to get the question right, we can only have 2 results, if function has return type bool not n results, unless it is Nullable<bool>, which could return null as well.

so

bool result = CalculateBool(item, someValue);   

if(result) {}
else {} 

will do it.

about managing large if / else combination ? One way is to use Switch statement, that could increase readability.

But in any case, a method should always have least possible decision paths, this is known as

• cyclomatic complexity.

If this happens, split the code into more appropriate methods

Answer 6

foreach(Thing item in SomeRandomList)
{
    DoGeneralThings(); //pass in whatever your require to the method

    if(FirstCondition(item, someValue))
    {
        DoThingsWhenAnyConditionIsTrue(); //pass in whatever your require to the method 
        DoSpecificThingsForFirstCondition(); //pass in whatever your require to the method
        continue;
    } 

    if(SecondCondition(item, someValue))
    {
        DoThingsWhenAnyConditionIsTrue(); //pass in whatever your require to the method 
        DoSpecificThingsForSecondCondition(); //pass in whatever your require to the method
        continue;
    } 
}