A long story today, but very instructive if you do not have much experience with SOLID. I will give you 2 clues in advance:
Image you have the folowing data transfer class that needs to be validated:
public class Person
{
public string Name { get; set; }
public int Age { get; set; }
public bool ConsumesAlcohol { get; set; }
}
These validations needs to be created:
public class AlcoholSeller
{
// Other codes not shown
private void Validate(Person person)
{
private const int MinimumAge = 18;
// Previous code forces person can not be null
if (person.Age < MinimumAge && person.ConsumesAlcohol)
{
throw new ValidationException(
"{0} is not allowed to consume alcohol because his or her age ({1}) is not {2} or higher.",
person.Name, person.Age, MinimumAge);
}
if (person.Age < 0)
{
throw new ValidationException("Age of {0} must be higher than 0",
person.Name);
}
}
}
using this exception:
public class ValidationException : Exception
{
public ValidationException(string message, params object[] args)
: base(String.Format(message, args))
{
}
}
This is not a good (SOLID) solution:<
An alternative solution would be: 1 class implements 1 validation. Advantages:
To make polyformism possible, let's use a small interface (interface segregation principle):
public interface IValidation
{
bool IsValid { get; } // True when valid
void Validate(); // Throws an exception when not valid
string Message { get; } // The message when object is not valid
}
and then create a base class for (most of) the validations
public abstract class ValidationBase<T> : IValidation where T : class
{
protected T Context { get; private set; }
protected ValidationBase(T context)
{
if (context == null)
{
throw new ArgumentNullException("context");
}
Context = context;
}
public void Validate()
{
if (!IsValid)
{
throw new ValidationException(Message);
}
}
public abstract bool IsValid { get; }
public abstract string Message { get; }
}
The 2 validations can now be rewritten:
public class OnlyAdultsCanConsumeAlcoholValidation : ValidationBase<Person>
{
private const int MinimumAge = 18;
public OnlyAdultsCanConsumeAlcohol(Person context)
: base(context)
{
}
public override bool IsValid
{
get { return !Context.ConsumesAlcohol || Context.Age >= 18; }
}
public override string Message
{
get
{
return string.Format(
"{0} is not allowed to consume alcohol because his or her age ({1}) not is {2} or higher.",
Context.Name, Context.Age, MinimumAge);
}
}
}
and
public class Age0OrHigherValidation : ValidationBase<Person>
{
public Age0OrHigherValidation(Person context)
: base(context)
{
}
public override bool IsValid
{
get { return Context.Age >= 0; }
}
public override string Message
{
get
{
return string.Format("The Age {1} of {0} is not 0 or higher.",
Context.Name, Context.Age);
}
}
}
Now it can be combined with a special List to make polyformism possible:
public class ValidationList : List<IValidation>, IValidationList
{
public bool IsValid
{
get
{
return this.All(v => v.IsValid);
}
}
public void Validate()
{
foreach (var validation in this)
{
validation.Validate();
}
}
public IEnumerable<string> Messages
{
get
{
return this.Where(v => !v.IsValid).Select(v => v.Message);
}
}
}
Implementing this small interface (interface segregation principle):
public interface IValidationList
{
bool IsValid { get; }
void Validate();
IEnumerable<string> Messages { get; }
}
Now the AlcoholSeller class can be loosely coupled from the validations applying Dependency inversion:
public class AlcoholSeller
{
private readonly IValidationList _validationList;
public AlcoholSeller(IValidationList validationList)
{
_validationList = validationList;
}
// Other code not shown
private bool IsValid
{
get
{
return _validationList.IsValid;
}
}
private IEnumerable<string> Messages
{
get
{
return _validationList.Messages;
}
}
}
When new validations are added to validationList, the AlcoholSeller class does not need any change (Open/close principle, Dependency inversion).
Unit test of the validations are now completely separated from the AlcoholSeller class. The unit tests of both the validation classes are separated as wel.
[TestClass]
public class OnlyAdultsCanConsumeAlcoholValidationTest
{
[TestMethod]
public void Person18AndConsumingAlcohol()
{
AssertHelper(18, true, true);
}
[TestMethod]
public void Person17AndConsumingAlcohol()
{
AssertHelper(17, true, false);
}
[TestMethod]
public void Person18AndNotConsumingAlcohol()
{
AssertHelper(18, false, true);
}
[TestMethod]
public void Person17AndNotConsumingAlcohol()
{
AssertHelper(17, false, true);
}
private void AssertHelper(int age, bool consumesAlcohol, bool expected)
{
var person = new Person { Age = age, ConsumesAlcohol = consumesAlcohol };
var validation = new OnlyAdultsCanConsumeAlcoholValidation(person);
Assert.AreEqual(expected, validation.IsValid);
}
}
and
[TestClass]
public class Age0OrHigherValidationTest
{
[TestMethod]
public void AgeIs0()
{
AssertHelper(0, true);
}
[TestMethod]
public void AgeIsBelow0()
{
AssertHelper(-1, false);
}
private void AssertHelper(int age, bool expected)
{
var person = new Person { Age = age };
var validation = new Age0OrHigherValidation(person);
Assert.AreEqual(expected, validation.IsValid);
}
}
The OnlyAdultsCanConsumeAlcoholValidation has a IsValid property containing the following implication: If a person consumes alcohol, the person's age needs to be at least 18. This is written as !Condition || requirement:
return !Context.ConsumesAlcohol || Context.Age >= 18;
I have seen that this line of code implemented by colleagues , but all used different variations. That makes it hard to review and multiple implementations are also a rich source of bugs. Some examples:
return !(Context.ConsumesAlcohol && Context.Age < 18);
if (!Context.ConsumesAlcohol)
{
return true;
}
if (Context.Age >= 18)
{
return false;
}
return true;
if (!Context.ConsumesAlcohol)
{
return true;
}
return Context.Age < 18;
if (Context.ConsumesAlcohol)
{
return Context.Age < 18;
}
return true;
if (Context.Age >= 18)
{
return true;
}
return !Context.ConsumesAlcohol;
if (Context.Age >= 18)
{
return true;
}
if (Context.ConsumesAlcohol)
{
return false;
}
return true;
This makes the IsValid propery hard to read and a mistake can easily be made.
Implements the implication in a sealed IsInvalid property and split the condition and the requirement in two different properties. The ValidationList and the AlcoholSeller class does not need any change.
The base class:
public abstract class ValidationBase<T> : IValidation where T : class
{
protected T Context { get; private set; }
protected ValidationBase(T context)
{
if (context == null)
{
throw new ArgumentNullException("context");
}
Context = context;
}
public void Validate()
{
if (!IsValid)
{
throw new ValidationException(Message);
}
}
public virtual bool Condition
{
get
{
// If the condition is not overriden, the requirent always needs to be true
return true;
}
}
public abstract bool Requirement { get; }
public bool IsValid { get { return Implication(Condition, Requirement); } }
public abstract string Message { get; }
private static bool Implication(bool condition, bool requirement)
{
return !condition || requirement;
}
}
The first validation has an implication, but the implication is already implemented in the base class to improve readability. 3 Simple properties are left in this class:
public class OnlyAdultsCanConsumeAlcoholValidation : ValidationBase<Person>
{
private const int MinimumAge = 18;
public OnlyAdultsCanConsumeAlcoholValidation(Person context)
: base(context)
{
}
public override bool Requirement
{
get
{
return Context.Age >= 18;
}
}
public override bool Condition
{
get
{
return Context.ConsumesAlcohol;
}
}
public override string Message
{
get
{
return string.Format(
"{0} is not allowed to consume alcohol because his or her age ({1}) not is {2} or higher.",
Context.Name, Context.Age, MinimumAge);
}
}
}
The other validation does not need to implement an implication so it has only 2 properties:
public class Age0OrHigherValidation : ValidationBase<Person>
{
public Age0OrHigherValidation(Person context)
: base(context)
{
}
public override bool Requirement
{
get {return Context.Age >= 0; }
}
public override string Message
{
get
{
return string.Format("The Age {1} of {0} is not 0 or higher.",
Context.Name, Context.Age);
}
}
}
Hi Alex, (as you know) I like your general aproach in creating these separate validation classes, especially if there are a lot of validations based on the same input. By creating some infrastructure for this you get a great benefit to enforce consistency in how validations are implemented!
One thing I am not sure about is the separation of the condition and the requirement, this seems to be quite arbitrary. I could say
- When you drink(condition) you should be at least 18 (requirement)
But I could also say
- If you are under 18 (condition) you are not allowed to drink (requirement)
Personally I would phrase it the second way, but that is besides the point. My point is that there is no good objective way to choose one over the other, so there is no real distinction to make between two parts of the same validation. Maybe your real world scenario's are better suitable for this, but I would be very carefull not to split all your validations in 2 parts just because you have two methods that can be overridden while some simple boolean logic will also do the trick.
Regards Frank
Hi Frank,
The validation with a separate condition and requirement forces you to NOT implement the implication in the IsValid property . Maybe it is a bit overenginered now. Thanks for your comment .I will consider changing my production code.
Best regards,
Alex
Arme mensen helpen is een nobel streven dat onze gemeenschap sterker maakt. Laten we samenwerken om de last van armoede te verlichten en een positieve influence te hebben operation de levens van degenen pass on het nodig hebben. Samen kunnen we een verschil maken en loop brengen naar degenen pass on arme mensen helpen.