HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Validation check and code-saving

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
validationcodeandchecksaving

Problem

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> _validationList;

    public Car()
    {
        this._validationList = new List>();
        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
{
    bool Validate(T t);
}

public class ValidateCarId : IValidationRule
{
    public bool Validate(Car t)
    {
        if (t.Id 
{
    public bool Validate(Car t)
    {
        if (t.Length 
{
    public bool Validate(Car t)
    {
        if (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 LengthInvali

Solution

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.

Context

StackExchange Code Review Q#62620, answer score: 10

Revisions (0)

No revisions yet.