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?

Answer 1

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.

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;
}

Answer 3

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.

Answer 4

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;

Answer 5

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?

Answer 6

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

Answer 7

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())

Answer 8

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);
    }
}

Answer 9

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

Answer 10

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)) {
}

Answer 11

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.

Answer 12

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...

Answer 13

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.

Answer 14

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.

Answer 15

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 :)
}

Answer 16

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.

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 :)

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.