Validation check and code-saving

Question

I have the ubiquitous "validate and save" scenario, wherein the Save() method performs various validity checks and proceeds to save the data. The solution works just fine, but I am interested to know how elegantly and correctly this code can be refactored.

First, the domain:

public class Car
{
    public int Id { get; set; }
    public decimal Length { get; set; }
    public int Years { get; set; }

    private List<IValidationRule<Car>> _validationList;

    public Car()
    {
        this._validationList = new List<IValidationRule<Car>>();
        this._validationList.Add(new ValidateCarId());
        this._validationList.Add(new ValidateCarLength());
    }

    public void Save()
    {
        // This commented code will also work!
        //_validationList.TrueForAll(x => x.Validate(this));

        foreach (var item in _validationList)
        {
            item.Validate(this);
        }

        // Proceed to Save the data
        Console.WriteLine("Done!");
    }
}

Validation interface and validation provider classes:

interface IValidationRule<T>
{
    bool Validate(T t);
}

public class ValidateCarId : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Id <= 0)
        {
            throw new IdNotFoundException("This Id is not available!");
        }
        else
        {
            return true;
        }
    }
}

public class ValidateCarLength : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Length <= 1000m)
        {
            throw new LengthInvalidException("Length must be more than 1000");
        }
        else
        {
            return true;
        }
    }
}

public class ValidateCarYear : IValidationRule<Car>
{
    public bool Validate(Car t)
    {
        if (t.Years <= 0 || t.Years >= 2)
        {
            throw new YearInvalidException("Year invalid!");
        }
        else
        {
            return true;
        }
    }
}

Custom exceptions:

public class IdNotFoundException : Exception
{
    public IdNotFoundException() { }
    public IdNotFoundException(string message) : base(message) { }
    public IdNotFoundException(string message, Exception inner) : base(message, inner) { }
    protected IdNotFoundException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

[Serializable]
public class LengthInvalidException : Exception
{
    public LengthInvalidException() { }
    public LengthInvalidException(string message) : base(message) { }
    public LengthInvalidException(string message, Exception inner) : base(message, inner) { }
    protected LengthInvalidException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

[Serializable]
public class YearInvalidException : Exception
{
    public YearInvalidException() { }
    public YearInvalidException(string message) : base(message) { }
    public YearInvalidException(string message, Exception inner) : base(message, inner) { }
    protected YearInvalidException(
      System.Runtime.Serialization.SerializationInfo info,
      System.Runtime.Serialization.StreamingContext context)
        : base(info, context) { }
}

And finally, the client:

class Program
{
    static void Main(string[] args)
    {
        Car vw = new Car { Id = -2, Length = 102, Years = 25 };

        try
        {
            vw.Save();
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message);
        }

        Console.WriteLine("Passed!");
    }
}

PS: I had to provide an else block in Validate(Cat t) for _validationList.TrueForAll(x => x.Validate(this)); to work. I felt this can be superfluous. How does it compare with foreach (var item in _validationList)?

Answer 1

Creating a custom validator class for each field in each class could potentially lead to bloated code, while you will not get much out of this.

Your model does not support user feedback. All it does is generate exceptions, and Exceptions are not suitable for representing error messages which are displayed to the user. You can catch these exceptions and display the messages to user, but you would abuse the exception mechanism that way. The rule is - never display exception content to users. It's a potential security hole.

Your model supports only a detection of the first error - an exception is thrown, and no further analysis can run. This way you can't provide a complete report of what's invalid in the model.

Initialization of static rule objects in model constructor is suboptimal - you are creating lots of these objects any time a constructor called. Simply making these static would optimize memory usage.

I would suggest returning a custom result object containing the status of the operation and a collection of rule violations from a 'Save' method, instead of throwing and catching exceptions. This way you would not abuse the exception handling mechanism with logic that is not exceptional - for example having an incorrectly entered string field is a normal situation, not an exception.

UPDATE (based on comment): As for the displaying exception contents to UI, a better approach would be separate save and validate operation, and use validation result to change UI state, so the Save operation is not allowed in the first place when model is not valid. For example, you could disable the save button. Then you don't need to handle validation exception in Save method. I strongly encourage you to reconsider the practice of displaying exception contents to user. Let me reiterate - non-valid user input should be considered as normal program flow, not en exceptional situation. Consider the following: Is it a vulnerability to display exception messages in an error page?

So i would recommend the following workflow: When initializing a form, update the ui based on validation result. Handle every change to the model in the ui: If model is not valid based on current user input - update UI with validation result, such as a Save button, display validation summary as text to the user, highlight form controls containing invalid values and so on. If model is valid for particular operation - enable the UI controls which allow this operation (the 'Save' button), and clean validation summary and the highlights from user screen. When Save button is pressed, you already sure that the model is valid, you don't need a check, because you have already sure you enabled it because of successful validation result, and it would be disabled on failed validation result.

Answer 2

Since your validators are stateless, I think you should initialize them in the static constructor of the object (called only once) and make your validation list static, this would save you the performance cost of creating validation instances everytime you create a Car instance.

private static List<IValidationRule<Car>> _validationList;

static Car()
{
    _validationList = new List<IValidationRule<Car>>();
    _validationList.Add(new ValidateCarId());
    _validationList.Add(new ValidateCarLength());
}

Also, I'm not sure if it is a preference or not, but classes name should represent something that can do an action, not the action itself, so ValidateCarId should be CarIdValidator

I don't know why you return a boolean in your validations if you never return false but throw an exception. You should make a choice between return false if the validation fails, or changing the interface signature to void Validate(T t) and return if the validation succeeds.

You could use the validators returning void with the following signature

validationList.ForEach(v => v.Validate(this));

To answer your last question, TrueForAll is the equivalent of this

foreach(var validator in _validationList)
{
    if(!validator.Validate(this))
        return false;
}

return true;

As you can see, it is basically the same thing

Answer 3

I'm assuming you're going to want to be validating more than this in the future. I recommend you abstract your validations based on what data they're validating, and what they're checking for, instead of making specific classes for each field you ever create in your model.

For example, instead of ValidateCarLength why not this?

public class ValidateGreaterThan : IValidationRule<Decimal>
{
    public bool Validate(Decimal target, Decimal actual)
    {
        if (actual <= target)
        {
            throw new ValueInvalidException(String.Format("Length must be more than {0}", target.ToString()));
        }
        else
        {
            return true;
        }
    }
}

Your domain would have to change slightly, though as they're now different generic types. A simple Validate method might work like this:

private const Decimal MinimumCarLength = 1000m;

public void Validate()
{
    new ValidateGreaterThan().Validate(MinimumCarLength, Car.Length);
}

If you're that enamoured with the existing domain structure, you could create your Car-specific validation classes by subclassing the more abstract versions, but I'd advise against it, as it just pushes tiny bits of code all other the project.