How to make large if-statement more readable

Question

Is there a better way for writing a condition with a large number of AND checks than a large IF statement in terms of code clarity?

Eg I currently need to make a field on screen mandatory if other fields do not meet certain requirements. At the moment I have an IF statement which runs over 30 LOC, and this just doesn't seem right.

if(!(field1 == field2 &&
     field3 == field4 &&
     field5 == field6 &&
     .
     .
     .
     field100 == field101))
{
   // Perform operations
}

Is the solution simply to break these down into smaller chunks and assign the results to a smaller number of boolean variables? What is the best way for making the code more readable?

Thanks

Answer 1

Personally, I feel that breaking this into chunks will just make the overall statement less clear. It's going to make the code longer, not more concise.

I would probably refactor this check into a method on the class, so you can reuse it as needed, and test it in a single place. However, I'd most likely leave the check written as you have it - one if statement with lots of conditions, one per line.

Answer 2

You could refactor your conditional into a separate function, and also use De Morgan's Laws to simplify your logic slightly.

Also - are your variables really all called fieldN?

Answer 3

Is there some other relationship between all the variables you're comparing which you can exploit?

For example, are they all the members of two classes?

If so, and provided your performance requirements don't preclude this, you can scrape references to them all into a List or array, and then compare them in a loop. Sometimes you can do this at object construction, rather than for every comparison.

It seems to me that the real problem is somewhere else in the architecture rather than in the if() statement - of course, that doesn't mean it can easily be fixed, I appreciate that.

Answer 4

The first thing I'd change for legibility is to remove the almost hidden negation by inverting the statement (by using De Morgan's Laws):

if ( field1 != field2 || field3 != field4 .... etc )
{
   // Perform operations
}

Although using a series of && rather than || does have some slight performance improvement, I feel the lack of readability with the original code is worth the change.

If performance were an issue, you could break the statements into a series of if-statements, but that's getting to be a mess by then!

Answer 5

I would consider building up rules, in predicate form:

bool FieldIsValid() { // condition }
bool SomethingElseHappened() { // condition }
// etc

Then, I would create myself a list of these predicates:

IList<Func<bool>> validConditions = new List<Func<bool>> {
  FieldIsValid,
  SomethingElseHappend,
  // etc
};

Finally, I would write the condition to bring forward the conditions:

if(validConditions.All(c => c())
{
  // Perform operations
}

Answer 6

Part of the problem is you are mixing meta data and logic.

WHICH Questions are required(/must be equal/min length/etc) is meta data.

Verifying that each field meets it's requirements is program logic.

The list of requirements (and the fields that apply too) should all be stored somewhere else, not inside of a large if statement.

Then your verification logic reads the list, loops through it, and keeps a running total. If ANY field fails, you need to alert the user.

Answer 7

Isn't this what arrays are basically for?

Instead of having 100 variables named fieldn, create an array of 100 values.

Then you can have a function to loop combinations in the array and return true or false if the condition matches.

Answer 8

It may be useful to begin using the Workflow Engine for C#. It was specifically designed to help graphically lay out these sorts of complex decision algorithms.

Windows WorkFlow Foundation

Answer 9

The right approach will vary depending upon details you haven't provided. If the items to be compared can be selected by some sort of index (a numeric counter, list of field identifiers, etc.) you are probably best off doing that. For example, something like:

Ok = True
For Each fld as KeyValuePair(Of Control, String) in CheckFields
  If fld.FormField.Text  fld.RequiredValue Then
    OK = False
    Exit For
  End If
Next

Constructing the list of controls and strings may be a slight nuisance, but there are reasonable ways of doing it.