Can the following complicated branching structure be simplified?-CodePudding

I was wondering if there is any way to simplify the following complicated branching structure in C#? Note that "do C;" appears twice. Thanks.

      if (condition1)
      {    
           if (condition2)
            {
                do A;
                if (condition3 on result of doing A)
                {
                    do B;
                }
                else
                {
                    do C;
                }                    
            }
            else 
            {
                do C;
            }
      }
      else
      {    
            do D;
       }
        do E;

CodePudding user response：

You can simplify it so that C() only appears once.

Your code is equivalent to

    if (condition1)
    {
        if (condition2)
        {
            if (A())
            {
                B();
            }
            else
            {
                C();
            }
        }
        else
        {
            C();
        }
    }
    else
    {
        D();
    }

    E();

Where the A() method returns a bool and is equivalent to your condition3 on result of doing A).

Looking at this simplification, note that you can replace:

    if (condition2)
    {
        if (A())
        {
            B();
        }
        else
        {
            C();
        }
    }
    else
    {
        C();
    }

With:

if (condition2 && A())
{
    B();
}
else
{
    C();
}

So the final code would look like this:

if (condition1)
{
    if (condition2 && A())
    {
        B();
    }
    else
    {
        C();
    }
}
else
{
    D();
}

E();

It's not a huge improvement, but it now mentions C() only once.

You could also write it as a switch but personally I don't think this is an improvement (I know that this is a subjective matter):

switch (condition1)
{
    case true when condition2 && A():
        B();
        break;

    case true:
        C();
        break;

    default:
        D();
        break;
}

E();

NOTE: It's critical to put condition2 && A() and not A() && condition2, of course.

CodePudding user response：

Usually the problem in this scenary it's the ciclomatic complexity, the code it's hard to read. One way to try to simplify the code is using return.

try
{
    if (condition1)
    {
        if (condition2)
        {
            if (A())
            {
                B();
                return;
            }
        }

        C();
        return;
    }

    D();
}
finally
{
    E();
}

Sometimes this give you a better code, more readable. But this is not the case. The final code is more or less the same and has lots of indentation levels. I include only because sometimes may be an option.

Another way it's divide and conquer. Create a method to reduce nesting:

if (condition1)
{
    if (condition2)
    {
        OnBothConditions();
    }
    else
    {
        C();
    }
}
else
{
    D();
}

E();

And delegate in other method:

private void OnBothConditions()
{
    if (A())
    {
        B();
    }
    else
    {
        C();
    }
}

You also can create this method:

private void OnCondition1()
{
    if (condition2)
    {
        OnBothConditions();
    }
    else
    {
        C();
    }
}

And change your code:

if (condition1)
{
    OnCondition1();
}
else
{
    D();
}

E();

There are other options, like Matthew Watson one, that are right and more compacted, but I prefer code more readable than more compacted because you must maintain that code and in 6 or 10 months probably will be difficult to understand this kind of "optimizations".

CodePudding user response：

I see now after writing it down that this answer is to some extent a mixture of that of Matthew Watson and that of Victor, but I will provide it anyway.

First of all, I agree with Victor that getting rid of all the nesting is an important improvement. I also agree with them that splitting your code into methods can be very helpful in that regard.

Secondly, I agree with Matthew that if the result of do A can be used directly to define the result of condition3, it may also be used directly rather than calling it before evaluating condition3.

If condition3 can be extracted to a method that takes the result of do A as an input parameter, you could avoid nesting entirely by creating one method per nesting level. As an example:

public static void Main()
{
    DoYourThing();
    do E;
}

DoYourThing()
{
    if (<condition1>)
    {
        DoYourNextThing();
    }
    else
    {
        do D;
    }
}

DoYourNextThing()
{
    if (<condition2> && Condition3(A()))
    {
        do B;
    }
    else
    {
        do C;
    }
}

private bool Condition3(object a)
{
    //...
}

If condition3 cannot be extracted to a method that takes the result of do A as an input parameter, DoYourNextThing() may need to be implemented differently. One possibility is:

DoYourNextThing()
{
    if (ShouldDoB())
    {
        do B;
    }
    else
    {
        do C;
    }
}

ShouldDoB()
{
    if (!condition2)
    {
        return false;
    }
    
    var a = do A;
    
    return condition3; // based on a
}

As a side note, I'd say that using descriptive variable names and method names is generally more important than refactoring away nested loops; whereas doing both (in a way that enhances comprehension and readability) is gold.

That being said, I trust that OP is using generic names simply to present a clear and easy example of the logic they are trying to achieve.