What is an effective way to validate a bunch of variables before moving on?
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 issu开发者_开发百科es before taking a reading: A");
}
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;
}
Something akin to
if(errorCondition1)
errors.add(message1);
if(errorCondition2)
errors.add(message2);
return errors.Count == 0;
So each condition is not nested
You can invert your if statements and use Guard Clauses instead. See this example.
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;
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.
}
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.
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
}
精彩评论