What is an effective way to validate a bunch of variables before moving on?

Question

I've been reading about the ideal size of methods and the single responsibility principle then I go look at some of my code. I feel I can break up a lot (>90%) of my stuff to be small manageable methods but then I get to validating a data or a form. It always seems really large and bloated. I tend to validate my data with nested if statements and try to catch errors or issues at each level. But when I start to get 6, 8, 10+ levels of validation it is very cumbersome. But I'm not sure how to break it up to be more effective.

An example of something I think is cumbersome but not sure how to improve upon it is below. Each of the levels has a unique action associated with it and only once all the conditions return true can the whole thing return true but this is tough to read, especially after coming back to the program after a month or so.

if (InitialUsageSettings.zeroed || sender.Equals(btnZero))
{   
    if (InitialUsageSettings.StandardFilterRun || sender.Equals(btnStandard))
    {   
        if (InitialUsageSettings.ReferenceFilterRun || sender.Equals(btnReference) || sender.Equals(btnStandard))
        {   
            if (InitialUsageSettings.PrecisionTestRun || sender.Equals(btnPrecision) || sender.Equals(btnReference) || sender.Equals(btnStandard))
            {   
                if (txtOperatorID.Text.Length > 0 && cboProject.Text.Length > 0 && cboFilterType.Text.Length > 0 && cboInstType.Text.Length > 0)
                {   
                    if (txtFilterID.Text.Length > 0 && txtLot.Text.Length > 0)
                    {   
                        return true;
                    }
                    else
                    {
                        if (txtFilterID.Text.Length == 0)
                        {
                            //E
                        }
                        if (txtLot.Text.Length == 0)
                        {
                            //D
                        }
                    }
                }
                else
                {
                    if (txtOperatorID.Text.Length == 0)
                    {
    //A
                    }
                    if (cboProject.Text.Length == 0)
                    {
    //B
                    }
                    if (cboFilterType.Text.Length == 0)
                    {
    //C
                    }
                    if (cboInstType.Text.Length == 0)
                    {
    //D
                    }
                    //return false;
                }
            }
            else
            {
                outputMessages.AppendLine("Please correct the folloring issues before taking a reading: X");
            }
        }
        else
        {
            outputMessages.AppendLine("Please correct the folloring issues before taking a reading: Y");
        }
    }
    else
    {
        outputMessages.AppendLine("Please correct the folloring issues before taking a reading: Z");
    }
}
else
{
    outputMessages.AppendLine("Please correct the folloring issues before taking a reading: A");
}

Answer 1

If your main purpose is to break the methods up into manageable chunks, you could encapsulate each if block in its own method. e.g.:

 if (InitialUsageSettings.zeroed || sender.Equals(btnZero))
 {
     ValidateStandardFilter();
 }
 else
 {   
     outputMessages.AppendLine("Please correct the folloring issues before taking a reading: A");
 }       

But it seems to me that this method has too many responsibilities: You're trying to make it validate and also output a message. Instead, the method should be solely responsible for validating.

public ValidationResult Validate(Sender sender)
{
    if (!(InitialUsageSettings.zeroed || sender.Equals(btnZero)))
    {   
        return ValidationResult.Error("A");
    }
    if (!(InitialUsageSettings.StandardFilterRun || sender.Equals(btnStandard)))
    {   
        return ValidationResult.Error("Z");
    }
    // Etc...
    if (txtOperatorID.Text.Length == 0)
    {
        errors.Add("A");
    }
    if (cboProject.Text.Length == 0)
    {
        errors.Add("B");
    }
    if (cboFilterType.Text.Length == 0)
    {
        errors.Add("C");
    }
    if (cboInstType.Text.Length == 0)
    {
        errors.Add("D");
    }
    if(errors.Count > 0)
    {
        return ValidationResult.Errors(errors);
    }
    if (txtFilterID.Text.Length == 0)
    {
        errors.Add("E");
    }
    if (txtLot.Text.Length == 0)
    {
        errors.Add("D");
    }
    return errors.Count > 0 
        ? ValidationResult.Errors(errors) 
        : ValidationResult.Success();
}

And then the calling code can worry about the output:

var result = Validate(sender);
if (result.IsError)
{
    outputMessages.AppendLine("Please correct...: " + result.Issue);
}

To get an idea of what the ValidationResult class might look like, see my answer here.

Update

The code above could be further refactored to reduce repetition even more:

public ValidationResult Validate(Sender sender)
{
    if (!(InitialUsageSettings.zeroed || sender.Equals(btnZero)))
    {   
        return ValidationResult.Error("A");
    }
    if (!(InitialUsageSettings.StandardFilterRun || sender.Equals(btnStandard)))
    {   
        return ValidationResult.Error("Z");
    }
    // Etc...

    var firstErrorBatch = GetEmptyStringErrors(
        new[]{
            new InputCheckPair(txtOperatorID, "A"),
            new InputCheckPair(cboProject, "B"),
            new InputCheckPair(cboFilterType, "C"),
            new InputCheckPair(cboInstType, "D"),
        })
        .ToList();
    if(firstErrorBatch.Count > 0)
    {
        return ValidationResult.Errors(firstErrorBatch);
    }

    var secondErrorBatch = GetEmptyStringErrors(
        new[]{
            new InputCheckPair(txtFilterID, "E"),
            new InputCheckPair(txtLot, "D"),
        })
        .ToList();
    return secondErrorBatch.Count > 0 
        ? ValidationResult.Errors(secondErrorBatch) 
        : ValidationResult.Success();
}

private class InputCheckPair
{
    public InputCheckPair(TextBox input, string errorIfEmpty)
    {
        Input = input;
        ErrorIfEmpty = errorIfEmpty;
    }
    public TextBox Input {get; private set;}
    public string ErrorIfEmpty{get; private set;}
}

public IEnumerable<string> GetEmptyStringErrors(IEnumerable<InputCheckPair> pairs)
{
    return from p in pairs where p.Input.Text.Length == 0 select p.ErrorIfEmpty;
}

Answer 2

Something akin to

if(errorCondition1)
  errors.add(message1);
if(errorCondition2)
  errors.add(message2);
return errors.Count == 0;

So each condition is not nested

Answer 3

You can invert your if statements and use Guard Clauses instead. See this example.

Answer 4

Reverse the flow. Instead of

If(cond) {
  if(someothercond) {
     //great sucess!
     return true;
  } else {
    // handle
    return false;
  }
} else {
  // handle
  return false;
}

do:

if(!cond1) {
  // handle
  return false;
}
if(!someothercond) {
  // handle
  return false;
}

// great sucess!
return true;

Answer 5

One way is to have a validation method that is called prior to executing your other code.

For example:

private String ValidateThis() {
  StringBuilder result = new StringBuilder();

  if (!cond1) {
    result.AppendLine("error on cond1");
  } 

   if (!cond2) {
     result.AppendLine("error on cond2");
   }

   return result.ToString();
}

public void ButtonClick(object sender) {
    String isValid = ValidateThis();

    if (!String.IsNullOrEmpty(isValid)) {
        // set your error message
        outputMessages.AppendLine(isValid);
        return;
    } 
    // ... perform your other operations.
}

Answer 6

I would try to have each validation defined as a predicate, something like this...

delegate bool Validator(object sender, out string message);

Then you could string as many of those together as you need.

Answer 7

There are a number of ways to tackle this. You really want to limit the amount of repeated code, such as the code that adds an output message, which is nearly identical in four or more places.

If you think of these nested if…else blocks as a sequence, where as soon as one fails you take action and stop further processing, you can create a list and leverage LINQ's FirstOrDefault functionality to process the list of conditions sequentially until one fails, or you get null if they all pass.

Creating an object to encapsulate the conditions will help consolidate and reduce duplication.

Here is an example:

public class Validator
{
    public Validator(string code, bool settingsCheck, Button button, object sender)
    {
        Code = code;
        IsValid = sender != null && button != null && sender.Equals(button);
    }

    public bool IsValid { get; private set; }

    public string Code { get; private set; }
}

Now, your method looks more like this:

var validationChecks = new List<Validator>
{
    new Validator("A", InitialUsageSettings.zeroed, btnZero, sender),
    new Validator("Z", InitialUsageSettings.StandardFilterRun, btnStandard, sender),
    new Validator("Y", InitialUsageSettings.ReferenceFilterRun, btnReference, sender),
    new Validator("X", InitialUsageSettings.PrecisionTestRun, btnPrecision, sender)
}

var failure = validationChecks.FirstOrDefault(check => !check.IsValid);
if (failure != null)
{
    outputMessages.AppendLineFormat(
        "Please correct the following issues before taking a reading: {0}", failure.Code);
    return;
}
else
{
    // further checks; I'm not sure what you're doing there with A-E
}