4
\$\begingroup\$

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?
\$\endgroup\$
  • \$\begingroup\$ @Hosch250 I initially did that, but I felt like a separate variable would be more suitable, so I can write stuff that the user can understand and keep the exception clean so I can understand it. \$\endgroup\$ – Denis May 20 '17 at 23:58
  • 1
    \$\begingroup\$ To short to be an answer: about your first question, no it's not clear. Especially because it will throw a null reference exception. Create a new object after each Build(), keep track of parameters outside Shift and create a new instance each time (best one for me, you can create multiple shifts with same parameters, for example for different employees) or...at least throw an informative exception to explain it's not used as intended (2nd best, IMO) \$\endgroup\$ – Adriano Repetti May 21 '17 at 11:04
  • 1
    \$\begingroup\$ Given that shift's do not have to start or end at the top of the hour, I suggest you rename 'StartHour' to 'StartTime' and 'EndHour' to 'EndTime'. \$\endgroup\$ – Rick Davin May 22 '17 at 13:11
5
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Thanks for your comments on the current implementation I will break ShiftRangeValidator into few smaller classes. Haha I wont be mad because of the alternative solution, however there is one difference in particular that I'm not happy, I can no longer inject different validators without changing the class itself. With my solution I can have 2 different shift instances that are being validated in completely different way. I feel like it would be best if I combine both methods. \$\endgroup\$ – Denis May 21 '17 at 12:25
  • \$\begingroup\$ @Denis if you need to have two different instances validated differently... then the shift objects are probably of different types and having derived classes that reflect those differences like ManagerShift and EmployeeShift etc. might be a good idea too. How else are you going to tell the which is which? Just another idea ;-) \$\endgroup\$ – t3chb0t May 21 '17 at 12:35
  • \$\begingroup\$ That's a really good point, worth changing the design for that alone. \$\endgroup\$ – Denis May 21 '17 at 12:45
  • \$\begingroup\$ I don't think having different shift types is a good idea. It's like introducing LondonDateTime and TexasDateTime classes to represent the difference in time zone. Employees and managers share the same concept of "shift", there is no difference in meaning. If validation logic is different, then I think you need different validators, not different Shift classes. \$\endgroup\$ – Nikita B May 22 '17 at 12:04
  • \$\begingroup\$ @NikitaB perhaps you're right ;-] It's just brainstorming. \$\endgroup\$ – t3chb0t May 22 '17 at 12:31
2
\$\begingroup\$
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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.