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

Not exactly the answer but.. If your form is really complicated, have you considered using rules engines to validate it? It sometimes makes things much easier. http://www.codeproject.com/KB/validation/ValidateFormBusinessRules.aspx — Kizz, Sep 07 '11 at 23:51
I didn't tag it as such (since edited) but this in a windows form. Do you think this might translate easily from the webs? — Brad, Sep 08 '11 at 13:30
Not with the control that article uses. I don't think it has a WinForms component. I was just trying to illustrate another way to validate forms that I came across recently. Everything is on the web these days. Form to me means web form. I forgot to check your tags, I apologize :) — Kizz, Sep 08 '11 at 15:45

score 1 · Answer 1 · answered Sep 07 '11 at 13:52

1

Something akin to

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

So each condition is not nested

answered Sep 07 '11 at 13:52

Sign

1,919
18
33

score 1 · Accepted Answer · edited Jun 20 '20 at 09:12

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

This is slick. I like this ValidationResult class. Can it only have one error message? that is, it can't, as is, generate an list of errors, right? Also, I don't see how you got the .IsError member. — Brad, Sep 07 '11 at 16:39
@Brad: The idea is for you to customize your `ValidationResult` class to suit your needs. I'd suggest renaming it to indicate what you're validation specifically. I'm envisioning `.IsError` being set to `true` for Error results and `false` for Success results. Basically the opposite of `AllowRequest` in the other answer I linked to. You can certainly adjust the class to allow you to add error messages to some kind of a collection, rather than just having a single error message. — StriplingWarrior, Sep 07 '11 at 16:47
wow. This is fantastic. I just wrote my first extension method. I think I see what to do now. Thanks a lot for the boost! — Brad, Sep 07 '11 at 16:55
@Brad: I changed my example to show how it might handle your inner `if` statements. The method is less nested this way, but it's still long. You might still consider breaking it down into smaller methods. — StriplingWarrior, Sep 07 '11 at 17:04
It seems like there are diminishing returns with the method length at this point.(?) Is it better to have 15 methods called from the one validation method or just have all these `if` statements in series? Getting them out of a nested format was huge but would the next step be? Several breakdowns like: `if (cboInstType.Text.Length == 0) { errors.Add("D"); }`. I do want to learn good practice and I'm the only person here doing this kind of thing so it's hard for me to tell what the larger behavioral trend would be in these kinds of situations. — Brad, Sep 07 '11 at 17:30
@Brad: Think of someone trying to step through this code. Ideally they could "read" each line and get a general idea of what happens there. Stepping over 50 lines of "Then check this..." will be tedious, but so will stepping into 15 different methods. Find a happy medium that will allow someone to easily drill down to the part of the code they're interested in. Method names should briefly describe what the method does, so methods should be broken up around logical boundaries. For example, everything above the `//Etc...` might belong in its own method. Also be more DRY (see update). — StriplingWarrior, Sep 07 '11 at 18:12

score 0 · Answer 3 · edited May 23 '17 at 12:29

0

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

edited May 23 '17 at 12:29

Community

1
1

answered Sep 07 '11 at 13:53

rie819

1,249
12
19

score 0 · Answer 4 · answered Sep 07 '11 at 13:53

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;

score 0 · Answer 5 · answered Sep 07 '11 at 13:56

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

score 0 · Answer 6 · answered Sep 07 '11 at 14:03

0

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.

answered Sep 07 '11 at 14:03

Eric Farr

2,683
21
30

score 0 · Answer 7 · answered Sep 07 '11 at 14:42

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
}

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

7 Answers7

Update