Click here to Skip to main content
15,884,986 members
Please Sign up or sign in to vote.
1.80/5 (2 votes)
See more:
C#
if (length >= 24 && Acct == strAcct && code == strCode1)
            {
                if (Box.HasValues)
                {
                    if (alk.Count > 1)
                    {
                        if (Effect != strReceive && strVal == "A")
                        {
                            Method1();
                        }
                        else
                        {
                            Method2();
                        }
                    }
                    else
                    {
                        Method2();
                    }
                }
            }
            else if (length >= 24 && Acct == strAcct && code == strCode2)
            {
                if (Box.HasValues)
                {
                    Method3();
                }
            }



For the first if and else if only difference is strCode1 & strCode2.
Is there any way I make this avoiding if -else if.
I want to make this little short and updated with new Operators maybe!
Posted
Updated 28-Jan-16 13:21pm
v3
Comments
PIEBALDconsult 28-Jan-16 21:10pm    
There's no such thing as "too many".
Member 12268082 29-Jan-16 0:10am    
I don't do C#, but isn't there some type of Select Case Method in this language?
Philippe Mori 29-Jan-16 9:09am    
C# has a switch statement. It is similar to C++ switch statement with the following différences:
- Switch can be used for string when cases are constants.
- Implicit fall-through is not allowed (you need goto case case_value).

However it is not as general as in other languages like F# or Swift for example.

Well, in that case, it is very easy to remove duplicate conditions...
C#
if (length >= 24 && Acct == strAcct && Box.HasValues)
{
    if (code == strCode1)
    {
        if (alk.Count > 1 && Effect != strReceive && strVal = "A")
        {
            Method1();
        }
        else
        {
            Method2();
        }
    }
    else if (code == strCode2)
    {
        Method3();
    }
}

You might decide to use a switch case to test the code (if the string you compare to are constants). This would be useful if you have more than 2 code to test.

Also, you can use ternary operator for the inner condition.
As someone pointed out in comments, in C#, that won't work in that case. It would works only if both function returns compatible type and the result is assigned. In C++ there are no such restriction.

Although, you might also use it for outer conditions, sometime an appropriate mix of if, switch and ?: can help make the code easier to read.

Appropriate split of line can also help make the code easier to read. Often for an if with multiple condition, I would break the line after each &&.
C#
// In C#, constant string can be used for switch cases. 
// If string are not constant, then one would have to use previous example code...
const string strCode1 = "code1";
const string strCode2 = "code2";

if (length >= 24 && Acct == strAcct && Box.HasValues)
{
    switch (code)
    {
        case strCode1:
            // As pointed out in comments, this won't work in C# (it does in C++).
            alk.Count > 1 && Effect != strReceive && strVal = "A" ? 
                Method1() : 
                Method2();
            // Replace instead by code from previous example.
            break;

        case strCode2:
            Method3();
            break;
    }
}
 
Share this answer
 
v4
Comments
Midi_Mick 28-Jan-16 19:52pm    
you can't use a switch, as case statements must be a constant.
Philippe Mori 28-Jan-16 20:49pm    
In C#, if case strings are constant, then one can use them. See my updated answer.
Andreas Gieriet 28-Jan-16 20:03pm    
I agree with Midi_Mick: you cannot use a variable in the cases.
You can neither call a void function in a ... ? ... : ....
Finally, a switch without a default can be considered as a code smell, where an if ... else if ... without an else is fine.
Cheers
Andi
Philippe Mori 28-Jan-16 20:26pm    
As indicated in my comment, the strings must be constant. In practice, this is often the case in that kind of scenario. Thus if the string is declared: private const string strCode1 = "code1"; then it will work perfectly right in C#. I do this, all the time.

It look like you are right about the fact that ?: cannot be used in that case in C#. It would have work in C++.

One can always add a default case, if he want too...
Andreas Gieriet 28-Jan-16 21:35pm    
Yes, if the string is declared as you show in your example, it would work. We can only guess what kind of declaration the OP has in his code.
Cheers
Andi
Reducing complex nested ifs and elses is not a trivial task. This is a typical refactoring task which first of all calls for a good unit test coverage.

Now, how to tackle the problem in a systematic manner?

1. No restructuring but combine common terms into functions

I suggest to first not change the structure but simplify by combining common elaborate conditions into predicate functions. In your case (assuming names starting on capital letters are members of the class), convert
C#
if (length >= 24 && Acct == strAcct && ...
...
else if (length >= 24 && Acct == strAcct && ...
into
C#
bool IsAcct(int length, string strAcct)
{
    return length >= 24 && Acct == strAcct;
}
...
if (IsAcct(length, strAcct) && ...
...
else if (IsAcct(length, strAcct) && ...
Likewise convert
C#
if (Effect != strReceive && strVal == "A") ...
into
C#
bool IsDesiredEffect(string strReceive, string strVal)
{
    return Effect != strReceive && strVal == "A";
}
...
if (IsDesiredEffect(strReceive, strVal)) ...

2. List all conditions for each code branch

This is best done by first give each simple condition a code, e.g.
C#
// A: IsAcct(length, strAct)
// C: code == strCode1
// D: Box.HasValues
// E: alk.Count > 1
// F: IsDesiredEffect(strRecieve, strVal)
// G: code == strCode2
The decorated code looks like this
C#
if (IsAcct(length, strAcct) && code == strCode1)         // A && C
{
    if (Box.HasValues)                                   // A && C && D
    {
        if (alk.Count > 1)                               // A && C && D && E
        {
            if (IsDesiredEffect(strReceive, strVal))     // A && C && D && E && F
            {
                Method1();
            }
            else                                         // A && C && D && E && !F
            {
                Method2();
            }
        }
        else                                             // A && C && D && !E
        {
            Method2();
        }
    }
}
else if (IsAcct(length, strAcct) && code == strCode2)    // A && !C && G
{
    if (Box.HasValues)                                   // A && !C && G && D
    {
        Method3();
    }
}
This results in the following conditions for the methods
C#
Method1() //  A && C && D && E && F
Method2() //  A && C && D && E && !F  || A && C && D && !E
Method3() //  A && !C && G && D


3. Re-arrange and simplify expressions

Here you need to know basics of Boolean logic transformations.
C#
Method1() //    A && D && C && E && F
Method2() //    A && D && C && E && !F || A && D && C && !E
          // => A && D && C && (E && !F || !E)
          // => A && D && C && (!E || !F)
          // => A && D && C && !(E && F)
Method3() //    A && D && !C && G
Uncluttered:
C#
Method1() //    A && D &&  C &&   E && F
Method2() //    A && D &&  C && !(E && F)
Method3() //    A && D && !C &&  G


4. Combining common expressions again

E.g.
C#
Method1() //    AD &&  C &&  EF
Method2() //    AD &&  C && !EF
Method3() //    AD && !C &&  G
While extending the predicates accordingly:
C#
// AD: IsAcct(length, strAct)
// C: code == strCode1
// EF: IsDesiredEffect(alk, strRecieve, strVal)
// G: code == strCode2
...
bool IsAcct(int length, string strAcct)
{
    return length >= 24 && Acct == strAcct && Box.HasValue;
}
bool IsDesiredEffect(ICollection alk, string strReceive, string strVal)
{
    return alk.Count > 1 && Effect != strReceive && strVal == "A";
}


5. Finally composing the reduced version
C#
if (IsAcct(length, strAct))
{
    if (code == strCode1)
    {
        if (IsDesiredEffect(alk, strReceive, strVal))
        {
            Method1();
        }
        else
        {
            Method2();
        }
    }
    else if (code == strCode2)
    {
        Method3();
    }
}


Summary

This approach might look a bit overwhelming, I agree. But it provides you with a systematic approach for such complex transformation. The approach with the codes for the expressions (A, C, D, etc.) allows for easier dealing with the Boolean expression transformations. It also helps in a peer review/pair programming to have the condensed logic to discuss rather than having the logic cluttered with code details.

Don't forget to start with a good set of unit tests before refactoring decision trees!

Have fun!
Cheers
Andi
 
Share this answer
 
Comments
Philippe Mori 28-Jan-16 21:49pm    
Using functions for complex condition is generally a good idea. In some case, it might make sense to have methods similar to bool ShouldCallMethodA(...) { ... }

In some cases where there are a lot of condition to check, it might even be a good idea to have a class that would handle all that stuff. And it might be easier to unit test.
Andreas Gieriet 28-Jan-16 22:02pm    
I fully agree!
Cheers
Andi
Cyrus_Vivek 28-Jan-16 23:19pm    
Thanks!! This will help me in future as well!! :)
Andreas Gieriet 29-Jan-16 8:49am    
You are welcome.
Cheers
Andi
BillWoodruff 29-Jan-16 1:23am    
+5 Excellent analysis, a contribution worthy, imho, of being posted as a Tip/Trick
To illustrate another approach to re-factoring:

a. analyze the cases where analysis can be skipped:

if (length < 24) return;
if (Acct != strAcct) return;
if (! Box.HasValues) return;

b. Combine them into a "short-circuiting" OR:
C#
if(length < 24 || (Acct != strAcct) || (! Box.HasValues)
{
     // throw an error ?
     // log a call that wasn't evaluated ?

     // do nothing ? 
     return;
}
Which of these (none, many, or, all of them) that you implement can be guided by your heuristic knowledge of what the most common parameter-values are in the most frequent calls to the method.
C#
if (code == strCode2)
{
    Method3();
}
else if (alk.Count > 1)
{
    if (Effect != strReceive && strVal == "A")
    {
        Method1();
    }
    else
    {
        Method2();
    }
}
else if // alk.Count <= 1
{
     // throw an error ?
     // log a call that wasn't evaluated ?
     // do nothing ?
}
Note: I like the approach of "teasing out" the "no go" factors before more complex evaluation, because, imho, it makes it easy to insert logging, or error handling (or, unit tests?), without touching the rest of the code.

imho, re-factoring is both a skill, craft, science, and art, when it takes into account your prior experience and wisdom, and the unique application context in which code is executed.

In the "real-world," however, where you work, and who you work with, may constrain how, and when, you re-factor. I have seen a scenario where a new programmer on the team of a very complex application got carried away with re-factoring in part due to being in fear of the very hard work required to learn the structure and dynamics of the application. Unfortunately, the re-factoring they did introduced some bugs, as well as requiring an expensive test cycle, and their code had to be discarded. Yes, there was a failure of management there in letting the newcomer kind of "go off on their own."
 
Share this answer
 
v4
Comments
Sascha Lefèvre 29-Jan-16 21:57pm    
+5
Cyrus_Vivek 29-Jan-16 22:10pm    
+5.
Thank you Bill.
Sergey Alexandrovich Kryukov 2-Feb-16 16:56pm    
A 5 just for short-circuiting...
—SA
When you see code like this:

C++
if (A)
{
    if (B)
    {
        work();
    }
}


It is equivalent to this:

C++
if (A && B)
{
    work();
}


With that in mind ...

C#
if (length >= 24 && Acct == strAcct && Box.HasValues)
{
    if (code == strCode1)
    {
        if (alk.Count > 1 && Effect != strReceive && strVal == "A")
        {
            Method1();
        }
        else
        {
            Method2();
        }
    }
    else if (code == strCode2)
    {
        Method3();
    }
}
 
Share this answer
 
v5
Comments
Andreas Gieriet 28-Jan-16 19:33pm    
You missed if (alk.Count > 1) ....
Andi
[no name] 2-Feb-16 16:28pm    
Thanks Andi.
Sergey Alexandrovich Kryukov 28-Jan-16 19:38pm    
5ed. It gives the right idea.
I would add that if (A & B) would not be quite equivalent, only &&, which is a short-cut form: B is only evaluated if A returns true.
—SA
Cyrus_Vivek 28-Jan-16 23:20pm    
Thanks!!! :)
PIEBALDconsult 29-Jan-16 23:26pm    
"It is equivalent to this:"

Except for that the former allows for more precise placement of breakpoints.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900