Validating Shift class through builder pattern

Question

I have a Shift class that looks like this:

public class Shift
{
    public string Name { get; private set; }
    public TimeSpan StartHour { get; private set; }
    public TimeSpan EndHour { get; private set; }

    public TimeSpan Duration => EndHour - StartHour;

    private Shift()
    {
    }
}

Since there are some limitations and restrictions to what StartHour && EndHour can be I needed some way to validate the input of the class and if it fails - prevent the creation of the object or at least not return it to the user, without throwing an exception because I want to allow the user to re-enter the arguments if some of them were invalid.

Than I created an interface and some derived classes to be injected during the construction of the object:

public interface IShiftValidator
{
    Exception FailedValidationException { get; }
    string FailedValidationMessage { get; }
    bool IsValid(Shift shiftToValidate);
}

public class ShiftRangeValidator : IShiftValidator
{
    public Exception FailedValidationException { get; private set; }
    public string FailedValidationMessage { get; private set; } = string.Empty;

    public bool IsValid(Shift shiftToValidate)
    {
        if (shiftToValidate.Duration.TotalMinutes <= 0)
        {
            FailedValidationException = new ArgumentOutOfRangeException(nameof(shiftToValidate.Duration));
            FailedValidationMessage = "Shift's duration cant be less than 1 minute.";
            return false;
        }
        if (shiftToValidate.Duration.TotalHours > SharedSettings.MaximumShiftHours)
        {
            FailedValidationException =
                new ArgumentOutOfRangeException(nameof(shiftToValidate.Duration));
            FailedValidationMessage = "Shift's duration cant be longer than the maximum allowed hours.";
            return false;
        }
        return true;
    }
}

After that I implemented the builder pattern as a nested class in the Shift class, so I can make the constructor of the class private and construct the object fully in the builder class:

public class ShiftBuilder
{
    private Shift _shift;

    public ShiftBuilder()
    {
        _shift = new Shift();
    }

    public ShiftBuilder WithName(string name)
    {
        _shift.Name = name;   
        return this;
    }

    public ShiftBuilder WithStartHour(TimeSpan startHour)
    {
        _shift.StartHour = startHour;
        return this;
    }

    public ShiftBuilder WithEndHour(TimeSpan endHour)
    {
        _shift.EndHour = endHour;
        return this;
    }

    public Shift Build(IEnumerable<IShiftValidator> validators)
    {
        if (validators != null)
        {
            foreach (var shiftValidator in validators)
            {
                if (!shiftValidator.IsValid(_shift))
                {
                    if (!string.IsNullOrEmpty(shiftValidator.FailedValidationMessage))
                    {
                        MessageBox.Show(shiftValidator.FailedValidationMessage);
                    }
                    throw shiftValidator.FailedValidationException;
                }
            }
        }
        Shift shift = _shift;
        _shift = null;
        return shift;
    }
}

Here's an example usage:

Shift.ShiftBuilder shiftBuilder = new Shift.ShiftBuilder();
var a = shiftBuilder.WithName("Test")
    .WithStartHour(new TimeSpan(12, 0, 0))
    .WithEndHour(new TimeSpan(24, 0, 0))
    .Build(new[] { new ShiftRangeValidator() });

var b = shiftBuilder.Build(null); // null
var c = shiftBuilder.Build(new[] {new ShiftRangeValidator()}); // crash

I have few concerns:

1. Is it clear that you're unable to reuse the same ShiftBuilder twice?
2. Is IShiftValidator good as it is or should I separate it in few smaller interfaces?

Answer 1

Don't be mad at me because I will again suggest you a different approach than your current implementation that exists in .Net for exactly that purpose, this is, validating models ;-)

But first a few comments about this one.

The ShiftBuilder requires a collection of validators to build the object IEnumerable<IShiftValidator>. This is a good start because you can use various validations however the current ones don't follow this principle yet. The example you showed has two responsibilities, this means it validates two things:

if (shiftToValidate.Duration.TotalMinutes <= 0)
{
    FailedValidationException = new ArgumentOutOfRangeException(nameof(shiftToValidate.Duration));
    FailedValidationMessage = "Shift's duration cant be less than 1 minute.";
    return false;
}
if (shiftToValidate.Duration.TotalHours > SharedSettings.MaximumShiftHours)
{
    FailedValidationException =
        new ArgumentOutOfRangeException(nameof(shiftToValidate.Duration));
    FailedValidationMessage = "Shift's duration cant be longer than the maximum allowed hours.";
    return false;
}

There are two ifs where each can fail. This means you need to split this validator into more specific one to test only one condition at a time.

---

Other then this I find this is a very complex implementation that leads me to the alternative suggestion which uses the ValidationAttribute. C# already has a system for constructing model validations. Here's a small example.

You can create a bunch of custom attributes that you use to decorate the properties of the Shift class with.

public class Shift
{
    public string Name { get; private set; }
    public TimeSpan StartHour { get; private set; }

    [GreaterThen(nameof(StartHour))]
    public TimeSpan EndHour { get; private set; }

    [GreaterThenZero]
    public TimeSpan Duration => EndHour - StartHour;

    public Shift()
    {
        // Just an example to make the validation fail.
        StartHour = TimeSpan.FromSeconds(100);
        EndHour = TimeSpan.FromSeconds(50);
    }
}

There are various ways of implementing them so you'll need to read more about them to know all options. I show you one of them where they implement the IsValid method in a very generic way (might not be perfect yet but it should make the point).

So you may need one attribute to validate the Duration:

public class GreaterThenZeroAttribute : ValidationAttribute
{
    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        return
            ((IComparable)value).CompareTo((IComparable)TimeSpan.FromSeconds(0)) > 0
                ? ValidationResult.Success
                : new ValidationResult($"{validationContext.MemberName} must be greater then zero");
    }
}

but on the other hand if you add GreaterThenAttribute the GreaterThenZeroAttribute for the Duration is no longer necessary.

public class GreaterThenAttribute : ValidationAttribute
{
    private readonly string _otherPropertyName;
    public GreaterThenAttribute(string otherPropertyName)
    {
        _otherPropertyName = otherPropertyName;
    }

    protected override ValidationResult IsValid(object value, ValidationContext validationContext)
    {
        var otherProperty = 
            validationContext
                .ObjectType
                .GetProperty(_otherPropertyName)
                .GetValue(validationContext.ObjectInstance);

        return
            ((IComparable)value).CompareTo((IComparable)otherProperty) > 0
                ? ValidationResult.Success
                : new ValidationResult($"{validationContext.MemberName} must be greater then {_otherPropertyName}");
    }
}

You may however need a new attribute for

shiftToValidate.Duration.TotalHours > SharedSettings.MaximumShiftHours

to make sure the duration does not exceed a certain number like MaxHoursFromSettings etc. and you can read the actual value from the settings later.

Finally to invoke the validation you use the Validator

var shift = new Shift();
Validator.ValidateObject(shift, new ValidationContext(shift), true);

Answer 2

public interface IShiftValidator
{
    Exception FailedValidationException { get; }
    string FailedValidationMessage { get; }
    bool IsValid(Shift shiftToValidate);
}

This interface is a bit confusing. It implies that FailedValidationException and FailedValidationMessage are properties of IShiftValidator, while in fact those are essentially return values of IsValid method (in other words they are properties of validation result). I think you would be better off returning a complex object:

 public interface IShiftValidator
 {
     ValidationResult Validate(Shift shiftToValidate);
 }

In C#7 you can just use tuples.