How many conditions in an "if clause" is acceptable?

Question

I'm trying to validate an HTTP Request, and first I want to get sure that all of the information has been posted (and then I'll check the correct format of each incoming data), so to get sure that no Form Spoofing attack is made. However, since this HTTP Post comes from a form, and that form contains almost 40 fields, I came across an if clause like this:

// Here, the if clause has almost 40 conditions to check
// first, second, third, etc. are imaginary names of incoming variables
if (Request["first"] == null || Request["second"] == null || ...)
{
    throw new ApplicationException("Incoming data is incomplete");
}
else
{
    // Now trying to validate the format of incoming data
}

Is it a normal checking mechanism? Is there a better way for doing it?

Answer 1

You could put all of your variable names into an array and loop over it, something like this (pseudocode):

var names = { 'First', 'Second', 'Third' };

for (var name in names) {
    if (Request[names[name]] == null) {
        throw new ApplicationException('Error');
    }
}

// No errors if we reach here

Answer 2

Looks unreadable.

I would normally extract such a condition to a private boolean function and reformat it for readability. That would be the first step.

A second refactoring may present itself after this - perhaps having a list of string corresponding to the field names and iterating over it with the Request indexer, checking for null values. This assumes that all the parts of the if are essentially doing the same nullity check. This is essentially what @Ant is presenting in his answer.

To answer your question - how many conditions are acceptable? It is a subjective thing, but in general as many as are readable. Personally, if the meaning of the conditions is not clear, the extract a method refactoring with a good meaningful name is a great start.

Answer 3

I would extract the validation into its own method:

if (RequestIsIncomplete(Request))
{
    throw new ApplicationException("Incoming data is incomplete");
}
else
{
    // Now trying to validate the format of incoming data
}

private bool RequestIsIncomplete(Request request)
{
    if (request["First"] == null)
        return false;

    if (request["second"] == null)
        return false;

    // etc...

    return true;
}

Seperating the decision making from the grunt work is a good win. As the grunt-work is seperated out, you're free to refactor it as necessary (honestly, don't leave it the way I've done it here - see Ants answer).

Answer 4

I don't know what language you're using (maybe C#?) but you should be referring to the fields posted form data as a collection rather than listing them individually. I think you should do this in an OOP approach using a Form object which contains field objects.

This is a very crude example of what I mean. You could actually have a Field interface implemented by concrete field classes like int, text, file etc.

class Field {
  public bool isValid = true;
  public string errorMessage;
  public string value;
  public Field(string name, string label, string type, bool required) {

  }
}
class Form {
  public List<Field> fields;
  public bool isValid = false;
  public void validate() {
    foreach(Field field in fields) {
      if(field.required && field.value = "") {
        this.isValid = false;
         field.isValid = false;
         field.errorMessage = field.name+" field is required";
      }
    }
  }

  public void loadData(Array request) {
    //stuff the request data into the field objects
    foreach(KeyValuePair<string, string> kvp in request) {
      fields.Where(x => x.name = kvp.Key).first().value = kvp.value;
    }
  }
}

On your post request you could do this

Form form = new Form();
//name, type, required
form.fields(new Field('field1','text',true);


if(Page.IsPostBack) {
  form.loadData(Request);
  form.Validate();
  if(!form.isValid) {
    //print some errors or whatever
    throw new Exception(...);
  }
}

Most web frameworks have Form and field classes, you could look at some framework APIs for ideas. You can also state exactly which field is left empty rather than just displaying a generic one or more fields are blank message. Even if this is only for a web service it can be helpful to state which of the 40 or so fields are not valid.

Your other option is to define required fields in a collection and use a loop to check if each Request[fieldName] is null.

Answer 5

Any if-statement clause system is acceptable provided it is easily readable and understandable.

First off, when the number of clauses gets above one (1), put each clause in parentheses. That way you don't have to think about operator precedence.

Second, when the if-line gets long, break it into multiple lines, one for each clause.

Third, whenever coding lines that parallel each other, add blanks in order to make the components line up. That way you can instantly see the parallelism and be confident that you haven't thrown in an extra character somewhere.

I would recode your example as:

if (     ( Request[ "first"  ] == null )
      || ( Request[ "second" ] == null )
      || ( Request[ "third"  ] == null )
      || (...) )
{

Fourth, if the clauses involve both AND and OR, be sure to parenthesise the sub-clauses and vary the indenting to show what pices oare sub-components of other pieces. And, when it gets this complex, think about breaking it up into multiple if-statements.

Answer 6

You could make it simpler using linq.

var requiredFiels = new [] { "First", "Second", ... };
if (requiredFields.Any(x => Request[x] == null))
    throw new ApplicationException(...);

I think that I would take a different approach, though. I would probably have developed the application following a domain driven approach. That means I would already have classes in my system representing the business entities to which the data in the form belongs. I would then simply fill out the domain objects with the data posted in the form, and place the responsibility of validating the data in the domain object itself.

However, neither of these techniques prevent you from form spoofing attack (which you said was the goal). To prevent this you should use an anti-forgery token.

Answer 7

You've received good answers for doing a simplistic check against all the fields. If this isn't the case, I'd suggest encapsulating the check.

Create an object which is a validHTTPrequest. It has all forty of your fields and the values are set to be what you consider correct. Take your input values, and assign them to an new object, then do an equality check. If you've overloaded the equals operator for that object to check is var and see if they are the same, then you've solved the problem.

Your statement then becomes:

if (currentRequest = validRequest)
{
    //do stuff
}
else
{
    throw new ApplicationException("Incoming data is incomplete");
}

If the definition of a valid request ever changes, you just modify the vaildRequest object initialization.