23
votes
\$\begingroup\$

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?

\$\endgroup\$

locked by 200_success Mar 30 '16 at 3:26

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

migrated from programmers.stackexchange.com Oct 13 '11 at 17:34

This question came from our site for professionals, academics, and students working within the systems development life cycle.

  • 1
    \$\begingroup\$ Are you expecting them all to be null, if so a const array of strings might do it, if not a key/value pair should work. \$\endgroup\$ – George Duckett Oct 13 '11 at 6:41
  • \$\begingroup\$ Why don't you simply loop through the Request? \$\endgroup\$ – Ramhound Oct 13 '11 at 12:27
  • \$\begingroup\$ @Ramhound: Because there are no null values in it. By looping through the items you don't see which are missing. \$\endgroup\$ – Guffa Oct 13 '11 at 17:44
  • 2
    \$\begingroup\$ This isn't a code review question. Best practices are off topic for code review. \$\endgroup\$ – Winston Ewert Oct 14 '11 at 3:54
45
votes
\$\begingroup\$

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
\$\endgroup\$
  • 4
    \$\begingroup\$ application of the good old 0 1 infinity rule \$\endgroup\$ – jk. Oct 13 '11 at 7:19
  • 5
    \$\begingroup\$ Request[names[name]] looks odd to me, shouldn't that be Request[name] ? \$\endgroup\$ – ammoQ Oct 13 '11 at 9:46
  • 4
    \$\begingroup\$ @ammoQ: The code is written in a pseudocode derived from an abhorrent mix of C# and JavaScript. The loop iteration style is from JS, but Request[name] would be more appropriate for C#. For more bizarreness note the single quotes and array initialisation style (JS) vs the use of null and exception handling (C#). \$\endgroup\$ – Ant Oct 13 '11 at 9:55
  • 4
    \$\begingroup\$ @pdr using C# as an example: var requiredFields = new[] { "First", "Second", "Third", etc... }; if (requiredFields.Any(x => Request[x] == null)) { /* request is incomplete */ } \$\endgroup\$ – MattDavey Oct 13 '11 at 10:19
  • 1
    \$\begingroup\$ @Jan Hudec: In JS, name is an integer that represents an index within the names array. So, names[name] is correct. \$\endgroup\$ – Ant Oct 13 '11 at 12:18
32
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1 for putting the most important step first. Personally, I'll never see how adding an unnecessary iterator to these situations makes things more readable. \$\endgroup\$ – pdr Oct 13 '11 at 8:30
27
votes
\$\begingroup\$

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

\$\endgroup\$
8
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Yeah @Keyo. Good OOP approach. However, it really complicates the situation without any benefit at all. I prefer to work procedurally in UI, and work OO in business, data access, services, etc. +1 for a new point of view. \$\endgroup\$ – Saeed Neamati Oct 13 '11 at 9:11
  • \$\begingroup\$ I don't agree that there is no benefit. You get to keep all the form handling code in one place. It can be refactored and extended with new methods and validation rules that will work everywhere. If you wanted you could have a render function for your form object. I prefer to work procedurally in UI, and work OO in business... Is form validation not business logic? It isn't UI code. I don't know what your context is, but in any project with more than a few forms doing it procedurally would get messy. \$\endgroup\$ – Keyo Oct 13 '11 at 9:22
6
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ XLNT!! - I prefer the logicals at the end \$\endgroup\$ – Dave Oct 13 '11 at 22:35
  • 2
    \$\begingroup\$ I prefer them at the start - easier to align for readability, and I can easily inspect the logicals without having to skim over the conditions. \$\endgroup\$ – ANeves Nov 4 '11 at 16:41
3
votes
\$\begingroup\$

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.

\$\endgroup\$
2
votes
\$\begingroup\$

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.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.