How should I rewrite a very large compound if statement in C#?

Question

In my C# code, I have an if statement that started innocently enough:

if((something == -1) && (somethingelse == -1) && (etc == -1)) {
    // ...
}

It's growing. I think there must be 20 clauses in it now.

How should I be handling this?

Gnome

I should have given more information, I see. I'm more than a little stunned at the number of responses, and will take some time to pore over all of them. I'm checking to see whether I can run the main code of this part of my program. I will UNLESS the string I'm looking at has such-and-such in it. I discover these such-and-suches with experience. Each time I find one, I add a clause. Again, thanks incredibly for all the help... – Gnome

In that case, why not just make a List<String> somehow (config file?) and do your comparison in a foreach? You could make that a guard clause, returning as soon as it finds something you don't like. – Pianissimo

Answer 1

Use gates where possible.

the if statement

if(bailIfIEqualZero != 0 && 
   !string.IsNullOrEmpty(shouldNeverBeEmpty) &&
   betterNotBeNull != null &&
   !betterNotBeNull.RunAwayIfTrue &&
   //yadda

the refactored version

if(bailIfIEqualZero == 0)
  return;

if(string.IsNullOrEmpty(shouldNeverBeEmpty))
  return;

if(betterNotBeNull == null || betterNotBeNull.RunAwayIfTrue)
  return;

//yadda

Bowleg

IMHO, this is the best solution of all those presented yet. +1 – Genteel

However, this breaks the single entry/single exit rule. – Carbineer

@Mystere: In this situation, that is OK. – Namesake

@Mystere - Good riddance. I never liked that rule anyway. – Pianissimo

As far as I can tell, it's identical to my answer except skipping the factor-into-a-function part that makes using return statements viable... – Unlearned

@Mystere - That rule leads to the Arrowhead anti-pattern. Bad ju-ju. – Cubiform

Its very close, chaos. I think you submitted it while I was editing. But I'd prefer to not call out to another method like your example unless there is some very complex stuff going on in the decision making process. – Bowleg

Hmm. I guess it's just a difference of assumptions. I was assuming that there would be other stuff going on in the current function after this conditional block that we're talking about, such that we couldn't use return because we'd be skipping that stuff. If all that's happening in the current function is this if block, then yeah, you don't need to refactor it, it's already its own function. – Unlearned

Answer 2

Factor it out into a function and make each condition a guard clause:

int maybe_do_something(...) {
    if(something != -1)
        return 0;
    if(somethingelse != -1)
        return 0;
    if(etc != -1)
        return 0;
    do_something();
    return 1;
}

Unlearned

Code-wise, this is no different from a nested if-statement method. Instead I would try to separate out the criteria into more manageable methods, where each method would check just a few criteria, and then the "innermost" method would execute the actual code. This would make the code more reusable and easier to maintain. – Hopefully

Redundant, yes. When you program though, you aren't just programming for the machine but for other poor souls who may have to read/maintain what you wrote. So for readability's sake, the above is an improvement. – Wherry

I don't think so. When breaking the original if-statement in mutiple lines too, it looks much better. – Scarification

See my comments to Brian Postow's answer. – Unlearned

Redundant? Because it has more than one 'return'? I don't think you know what the term means. – Bowleg

Answer 3

Assuming all those conditions are really necessary, you could consolidate the conditions into one or more uber-booleans or function calls to enhance readability.

E.g.,

bool TeamAIsGoForLaunch = BobSaysGo && BillSaysGo;
bool TeamBIsGoForLaunch = JillSaysGo && JackSaysGo;

if (TeamAIsGoForLaunch && TeamBIsGoForLaunch && TeamC.isGoForLaunch())

Sharecropper

This. There ought to be semantic groups of options within that if. See if you can rewrite the code around the if to allow that, and then make them sub-functions if possible/necessary (e.g. if they are going to be reused). – Octans

I usually go with this approach. This is more readable than any other solution, since you are naming the condition and it provides much easier debugging because you can see each conditions value before you hit the if statement. – Iridissa

@aehiilrs: Wait.. it isn't? oh... damn my dyslexia! B-) – Berthold

Answer 4

One thing to consider is why you have so many clauses. Just as SWITCH statements often indicate that you should be moving the choices into subclasses, a large complex chain of IF statements can indicate that you are combining too many concepts (and thus decisions) in one place.

As an example, I will use the example of calculating commission. In one system I built, the commission rate is dependent upon the amount of commission given to the wholesaler, who passes some on to the retailer. The amount given from a wholesaler to retailer depends on the specific agreement between the retailer and the wholesaler (based on a contract). The amount the wholesaler gets likewise depends on the product line, specific product and amount of the product sold.

Beyond this, there were additional "exception" rules based on the customer's state, specific product customizations, etc. This could all be worked out in a complex chain of IF statements, but we instead made the application data driven.

By putting all of this information into tables, we were then able to make passes on the data rules. First the wholesaler's generic rules would trigger and then any override rules would trigger. Then, having the base wholesaler commission in hand we would run the wholesaler to retailer generic rules and then the exceptions to those.

This turned a huge hairball of logic into a simple four step process, with each step simply making a database query to find the correct rule to apply.

This of course may not apply in your situation, but often a large complex like that really means that either there are not enough classes dividing up the responsibility (another solution to our problem could have been factory classes that contained the specific rules and overrides), or the functionality should be data driven.

Luncheon

EDIT: Steamer's suggestion is a good one if you don't want to go with a full rules system or class set: combining various small conditions into "super conditionals" can really clean code up and make it easier to read the later if statements. – Luncheon

This turned a huge hairball of logic into a simple four step process -- I'd rather say that this simply moved the huge hairball of logic from code into the rule table. – Mcabee

@AntonTykhyy Why would you rather say that? By dividing the logic into four passes (each pass divided into a default rule and a potential override rule) each pass is far simpler to understand for the humans that maintain the commission system, since it now reflects the mental model the users of the system use. Now the users can query the rules system for a scenario, see the rules that trigger and know why they triggered. None of that was available in a huge block of code based logic. Obviously a poorly constructed rule system won't help and could make things worse. Don't do that. – Luncheon

I see. I haven't considered the trigger visibility angle. Thanks! – Mcabee

Answer 5

Refactor it to a function.

bool Check()
{
  return (something == -1) && (somethingelse == -1) && (etc == -1);
}

Alternatively, you can build more readable code/logic in your Check function.

Nebiim

So the answer to "What should I do with my big if-statement?" is to just move it out of the way? That hardly sounds like a solution now, does it? The correct way to deal with big criteria-blocks like this is to split them up into manageable chunks. – Hopefully

@Lasse- that depends. Sometimes the issue is what does this boolean logic do? Using extract method makes the functionality of the boolean logic self describing- refactoring.com/catalog/extractMethod.html. – Syne

Answer 6

There are many ways to handle this, but let me pick a few.

First, there's the case where all of the criteria (all of the AND-criteria in your if-statement) + the code to execute if they're all true is a one-off situation.

In this case, use the code you have. You might want to do what several others have already suggested, rewrite to use a Guard-clause type of code.

In other words, instead of this:

if (a && b && c && d && ......)
    DoSomething();

... you rewrite to something similar to this:

if (!a) return;
if (!b) return;
if (!c) return;
if (!d) return;
if (!...) return;
DoSomething();

Why? Because once you start introducing OR-criteria into the mix, it gets hard to read the code and figure out what is going to happen. In the above code, you split the criteria on each AND-operator (&&), and thus the code becomes easier to read. Basically you rewrite the code from saying "if this and that, or that other thing and that third thing, or some other thing, then do something" to be "if this, then exit; if that other thing; then exit; if some other thing; then exit; if none of the above, do something".

However, in many cases, you also have the case of reusability. If some of those criteria appear else-where, but the code that will actually execute (DoSomething) isn't the same, then I'd go for, again, what others have already suggested. Rewrite the criteria into methods that return a Boolean result depending on the result of evaluating the criteria.

For instance, what is easier to read, this?

if (a && b && c && d && e && f && (h || i) && (j || k) || l)

or this:

if (CanAccessStream() && CanWriteToStream())

assuming all those letters can be split into those two criteria.

In that case I would take some of the criteria and put into those methods, and choose an appropriate name for the criteria.

The third option is where the criteria differs in several places in the code, but the actual code to execute is the same.

In this case I would rewrite so that you group criteria together and layer the methods so that calling one method will check some criteria, and then call another method, which will check some other criteria, etc.

For instance, you could write this:

if (stream != null && buffer != null && inBuffer > 0 && stream.CanWrite)
  stream.Write(buffer, 0, inBuffer);
else
    throw new InvalidOperationException();

or you could write this:

if (inBuffer > 0)
{
    Debug.Assert(buffer != null);
    WriteToStream(buffer, inBuffer);
}

...

private void WriteToStream(Byte[] buffer, Int32 count)
{
    if (stream.CanWrite)
        stream.Write(buffer, 0, count);
    else
        throw new InvalidOperationException();
}

I'd say the second way is easier to read, and is more reusable, than the first.

Hopefully

Answer 7

Perform aggregate operations on a list of your values.

if (new[] { something, somethingelse, ... }.All(x => x == -1)) {
}

*Edit: Givin' the data an extra-line:

var Data = new[] { something, somethingelse, ... };
if (Data.All(x => x == -1)) {
}

Scarification

I'd probably put this in a function or macro to avoid calling new inside an if condition... – Berthold

Answer 8

It looks like you have 3 pieces of information that together represent a particular state in your application. Instead of switching on these 3 pieces of state, why not create a value that encapsulates them? Then you could use an object hierarchy or a delegate at creation time to bind the action you are trying to run.

Pullet

Answer 9

You could refactor it as a function, and return an Enum value that represents the case that was true:

if(something != -1)
    return MyEnum.Something;
if(somethingelse != -1)
    return MyEnum.SomethingElse;
if(etc != -1)
    return MyEnum.SomethingElseEntirely;
return MyEnum.None;

Moquette

Answer 10

You could also factor the state into a class.

class mystate
{
    int something;
    int somethingelse;
    int etc;

    bool abletodostuff()
    {
        return (something == -1) && (somethingelse == -1) && (etc == -1);
    }
}

Namesake

Answer 11

I don't know C#, but it seems to include the conditional operator. If your conditions are short, you can replace long if/elsif/else chains with nice table-like structures, like this:

return   something == 0      ? 0
       : somethingelse == -1 ? 1
       : yetanotherthing > 2 ? 2
       :                       3; # default

Heymann

He has a long && chain. Putting each condition on a separate is sufficient. This pattern is confusing to people who have never seen it, among other problems. – Namesake

While I don't think that the pattern is confusing with a few items, it would be after 20 comparisons all nested. – Cowell

I like that this answer hasn't been downvoted, and the one to separate into nested if-statements was, which is the first step towards refactoring into separate methods. Peer pressure FTW! – Hopefully

Hmm, you're right. The other one would just make 20 nested if statements. If it was "the first step towards refactoring", then the rest of the steps should have been noted. This one isn't an answer either. – Cowell

What I meant by "first steps towards refactoring" is actually to separate out the criteria so that you get reusable, easy to maintain, method. If those methods only check a couple of criteria each, but are named appropriately, and the criteria logically belong together, then yes, that's the way I would go. – Hopefully

Answer 12

Yet another avenue that you may explore is the use of composite expressions. These expressions (which are the basis of how LINQ works in the .NET framework) allows you to create expression trees based on all these conditions and then the business logic code can simply operate on the top level expression to get back the true/false result.

To evaluate the expressions, you can make use of the visitor pattern

This allows you to easily compose trees of the conditions. these trees can be serialized even to let you persist the conditions that you made the decision under. Lots of opportunities present themselves here.

Pulpwood

Answer 13

It's not that common to see that many clauses in one "if". You usually find that you need to nest the "ifs" to get the logic you need, when you need to execute some line regardless of the truth of some conditions. I'm not saying nest them if you don't need to, if they all need to be tested at the same time. Only if there's some common functionality. Another consideration is to set a boolean variable with the result of some set of these conditions, that might make it easier to understand. If your variables are an array or collection, could you loop through them? Are you testing them all against -1?

Grissel

Answer 14

While I like Dario's solution (as I commented, I'd put it in a boolean function so I didn't have to have a new in the condition part of an if...) I'm not sure what's wrong with:

if((something == -1) &&
   (somethingElse == -1) &&
   (elseElse == -1) &&
   ...
  )

I think that's probably a lot easier to read than a lot of ((A && B) || (C &&(D||E))) that I have to deal with...

Berthold

The main thing that's wrong with it is that you can't do anything non-suck with the indentation. Aligning on three spaces or some other not-a-real-indent-level, like you're doing, is pure evil, and going out to a full indent level makes the conditionals visually indistinct from the contained block. – Unlearned

And going out two indent levels, though probably the most non-suck thing I've seen for condition continuation, is still pretty crazy. – Unlearned

Why is that evil? Emacs does that automatically for me, so I never even think about it... – Berthold

1) I dislike the idea of breaking a logical convention for the sake of aligning with parens wherever they happened to land 2) now you can't use tabs for indentation, only spaces -- either that or you have mixed tabs and spaces, which is something far beyond ordinary evil. – Unlearned

Odd, I mix tabs and spaces all the time with no issue... I mean, as long as you never had tabs AFTER spaces, there should be no problem... – Berthold

Meh. Probably I'm just opinionated and tend to overgeneralize from my experience. Programmers are hardly ever like that... – Unlearned

Answer 15

I do this in a way nobody likes except me. I don't care if the code "looks good". If I've got >1 test in that conditional, that means I'm liable to want even more, or I may want to comment some of them in or out, and I want to simplify the making of such changes.

So I code it like this:

if (true
  && test_1
  && test_2
  ...
  )
{
  ...
}

This makes it easy to comment out tests, or add new ones, as 1-line edits.

But I'll be the first to admit, it doesn't claim to be pretty.

Overcrop

Answer 16

And Something like this

I'll explain a litlle further. (And fix the stupid errors :S)

//Interface to be able to which classes are able to give a boolean result used in the if stuff
public interface IResultable
{
    bool Result();
}

//A list that is IResultable itself it gathers the results of the IResultables inside.
public class ComparatorList<T> : List<T>, IResultable where T : IResultable
{
    public bool Result()
    {
        bool retval = true;
        foreach (T t in this)
        {
            if (!t.Result())
            {
                retval = false;
            }
        }
        return retval;
    }
}

//Or two bools
public class OrComparator : IResultable
{
    bool left;
    bool right;

    public OrComparator(bool left, bool right)
    {
        this.left = left;
        this.right = right;
    }
    #region IResultable Members

    public bool Result()
    {
        return (left || right);
    }

    #endregion
}

// And two bools
public class AndComparator : IResultable
{
    bool left;
    bool right;

    public AndComparator(bool left, bool right)
    {
        this.left = left;
        this.right = right;
    }
    #region IResultable Members

    public bool Result()
    {
        return (left && right);
    }

    #endregion
}

// compare two ints
public class IntIsComparator : IResultable
{
    int left;
    int right;

    public IntIsComparator(int left, int right)
    {
        this.left = left;
        this.right = right;
    }
    #region IResultable Members

    public bool Result()
    {
        return (left == right);
    }

    #endregion
}

Is you have a lot of there if statements this could be cool :)

So we have this sturcture for handeling a lot of comparisons. I'll give a little example implementation.

//list of ands
ComparatorList<AndComparator> ands = new ComparatorList<AndComparator>();    
ands.Add(new AndComparator(true,true));

//list of ors
ComparatorList<OrComparator> ors = new ComparatorList<OrComparator>();
ors.Add(new OrComparator(false, true));

//list of intiss
ComparatorList<IntIsComparator> ints = new ComparatorList<IntIsComparator>();
ints.Add(new IntIsComparator(1, 1));

//list of all things :)
ComparatorList<IResultable> collected = new ComparatorList<IResultable>();
collected.Add(ands);
collected.Add(ors);
collected.Add(ints);

// if all things are as they must be :)
if (collected.Result())
{
    //Evertything is as it schould be :)
}

Socinian

Answer 17

If you really have to check against all these conditions, you can't get rid of them. You can only refactor it and make it more readable, but the logic stays the same.

I'm quite surprised that no one has mentioned anything about redesigning your code. Do you really really need 20 differents states? In my experience, a lot of states are often depending on other states and are therefore often logically redundant to check against.

Refactoring your code might help you get a better understanding of it and how the states are coupled to each other. I'd start here first if I were you :)

Graeco

"I'm quite surprised that no one has mentioned anything about redesigning your code." Godeke and JaredPar both suggested that. – Bedrock

Sorry my bad. They got lost in all those if statements :) – Graeco

Indeed. :) – Bedrock

Answer 18

you say you are looking at strings so how about something like this which someone commented about already.

        var items = new List<string>();

        items.Add("string1");
        items.Add("string2");

        if (items.Contains("string2"))
        {
            // do something
        }

you could even grab the values from a configuration file of some sort to populate the list.

Santee