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