3
\$\begingroup\$

Before I can perform a more or less complex mappings, treatments in business objects and saving results in databases, I often need to validate contents from flat text files or excel files

All the validation libraries I know are designed to validate the content of strongly typed objects. The parsers or readers I tried are really good to load or write data but in case of an error in file, they are design for logging purpose and not for the end user. In addition, I'd like to use a library to do the validation only and be able to change the parser/reader if necessary. My goal is to validate the content of theses files and provide maximun feedbacks to the end users as if it was a form validation in web page.

Instead of wrting code that for sure no one will ever review, I decided to spend the day trying to create a library as an exercice and publish it on GitHub for differents reasons : try to write a fluent API, learn to use GitHub and git more accurately, create unit tests with xUnit and use a CI to automate test project (Travis, AppVeyor...), learn how to create a Nuget package and now try to collect some comments, ideas, remarks...and maybe use the library

For the time being, I end up with something like this :

I define a class with string properties that represent the columns of the files (I could also use a dynamic object) and validate with :

public class Row
{
    public string Key { get; set; }
    public string DecimalValue { get; set; }
    public string DateTimeValue { get; set; }
}

// ...

ClassValidator<Row> validator = ClassValidator<Row>
    .Init()
    .AddProperty(PropertyValidator<Row>.For(x => x.Key).IsNotNull().HasLength(5,10))
    .AddProperty(PropertyValidator<Row>.For(x => x.DateTimeValue).IsNotNull().IsDateTime("yyyyMMdd"))
    .AddProperty(PropertyValidator<Row>.For(x => x.DecimalValue).IsNotNull().IsDecimal())
    .Validate(new Row()
    {
        Key = "thiskey",
        DateTimeValue = "20181201",
        DecimalValue = "123,45"
    });

The ClassValidator contains N PropertyValidator for the N properties to validate. Each PropertyValidator can contains N MethodValidator to do the validation (IsNull, TryDecimal, TryRegex ...). Most of the methods are defined as Func, Action delegates executed only when the Validate method is called.

The ClassValidator constructor is private and the Init method can receive some options. The Validate method loop over the List<PropertyValidator<TRow>> populated with the AddProperty method

/// <summary>
/// Validation at class level.
/// </summary>
/// <typeparam name="TRow">The type of the model.</typeparam>
public class ClassValidator<TRow>
{
    private readonly IClassValidatorOptions options;

    private readonly List<PropertyValidator<TRow>> listValidator = new List<PropertyValidator<TRow>>();

    /// <summary>
    /// Initializes a new instance of the <see cref="ClassValidator{TRow}"/> class.
    /// Private constructor.
    /// </summary>
    private ClassValidator()
    {
    }

    private ClassValidator(IClassValidatorOptions options)
    {
        this.options = options;
    }

    /// <summary>
    /// Gets validation errors.
    /// </summary>
    public List<ValidationError> ValidationErrors { get; private set; } = new List<ValidationError>();

    /// <summary>
    /// Gets a value indicating whether the validation is successful or not.
    /// </summary>
    public bool IsValid
    {
        get { return this.ValidationErrors.Count == 0; }
    }

    /// <summary>
    /// Create an instance with no option.
    /// </summary>
    /// <returns>The created instance.</returns>
    public static ClassValidator<TRow> Init()
    {
        return new ClassValidator<TRow>();
    }

    /// <summary>
    /// Create an instance with option.
    /// </summary>
    /// <param name="options">options.</param>
    /// <returns>The created instance.</returns>
    public static ClassValidator<TRow> Init(IClassValidatorOptions options)
    {
        return new ClassValidator<TRow>(options);
    }

    /// <summary>
    /// Add a property validator. <see cref="PropertyValidator{TRow}"/>.
    /// </summary>
    /// <param name="propValidator">Targeted property validator.</param>
    /// <returns>current instance.</returns>
    public ClassValidator<TRow> AddProperty(PropertyValidator<TRow> propValidator)
    {
        this.listValidator.Add(propValidator);
        return this;
    }

    /// <summary>
    /// Validation for one row.
    /// </summary>
    /// <param name="row">Row to validate.</param>
    /// <returns>Current instance.</returns>
    public ClassValidator<TRow> Validate(TRow row)
    {
        foreach (var validator in this.listValidator)
        {
            validator.Validate(row);

            if (!validator.IsValid)
            {
                this.ValidationErrors.AddRange(validator.ValidationErrors);
            }
        }

        return this;
    }

    /// <summary>
    /// Validation for rows collection.
    /// </summary>
    /// <param name="rows">rows collection to validate.</param>
    /// <returns>Current instance.</returns>
    public ClassValidator<TRow> ValidateList(IEnumerable<TRow> rows)
    {
        int index = 0;

        foreach (var row in rows)
        {
            foreach (var validator in this.listValidator)
            {
                if (this.options != null && this.options.ShowRowIndex)
                {
                    validator.SetRowIndex(index);
                }

                validator.Validate(row);

                if (!validator.IsValid)
                {
                    this.ValidationErrors.AddRange(validator.ValidationErrors);
                }
            }

            index++;
        }

        return this;
    }
}
  • If the generic type is set on the top class ClassValidator <Row>. Ideally, I would like to set the type once and reuse the same type for PropertyValidator. In the sample, I have to define the type for ClassValidator and for eachPropertyValidator, which could lead to some inconsistency.

The PropertyValidator exposes all the methods to do the validation of the property. Each validation method (eg : TryParseDecimal ) adds a MethodValidator in the Collection>.

/// <summary>
/// Define a validator for a property in <see cref="PropertyValidator{TRow}"/> type.
/// </summary>
/// <typeparam name="TRow">Type for this validator.</typeparam>
public class PropertyValidator<TRow> : IPropertyValidatorAction<TRow>
{
    private readonly Collection<MethodValidator<TRow>> methods = new Collection<MethodValidator<TRow>>();
    private int? rowIndex;
    private string fieldName;
    private Func<TRow, string> getter;
    private Expression getterExpression;
    private dynamic extraObject;
    private bool preserveErrorHeader = true;

    private PropertyValidator()
    {
    }

    /// <summary>
    /// Gets the list of validation errors.
    /// </summary>
    public List<ValidationError> ValidationErrors { get; private set; } = new List<ValidationError>();

    /// <summary>
    /// Gets a value indicating whether the validation is successfull.
    /// </summary>
    public bool IsValid
    {
        get { return this.ValidationErrors.Count == 0; }
    }

    /// <summary>
    /// Creates a property validator for the given property.
    /// </summary>
    /// <param name="getterExpression">The targeted property.</param>
    /// <param name="overrideFieldName">To override field Name. By default uses the name of property.</param>
    /// <returns>A property validator.</returns>
    public static PropertyValidator<TRow> For(Expression<Func<TRow, string>> getterExpression, string overrideFieldName = null)
    {
        PropertyValidator<TRow> prop = new PropertyValidator<TRow>();
        prop.getter = getterExpression.Compile();
        prop.getterExpression = getterExpression;
        prop.fieldName = overrideFieldName != null ? overrideFieldName : ExpressionUtiities.PropertyName(getterExpression);
        return prop;
    }

    /// <summary>
    /// Creates a property validator for a dynamic property.
    /// </summary>
    /// <param name="getter">The targeted dynamic property.</param>
    /// <param name="fieldName">Fieldname for the dynamic property.</param>
    /// <returns>A property validator.</returns>
    public static PropertyValidator<dynamic> ForDynamic(Func<dynamic, string> getter, string fieldName)
    {
        PropertyValidator<dynamic> prop = new PropertyValidator<dynamic>();
        prop.getter = getter;
        prop.fieldName = fieldName;
        return prop;
    }

    /// <summary>
    /// Allow to set a row index. Usefull to display a line number in error message.
    /// </summary>
    /// <param name="rowIndex">Row index.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> SetRowIndex(int rowIndex)
    {
        this.rowIndex = rowIndex;
        return this;
    }

    /// <summary>
    /// Allow to set an object to pass an extra object to the validator.
    /// </summary>
    /// <param name="extraObject">Extra dynamic object.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> SetExtraObject(dynamic extraObject)
    {
        this.extraObject = extraObject;
        return this;
    }

    /// <summary>
    /// Allow overriding default error message.
    /// </summary>
    /// <param name="msgErrorFunc">Custom error message as a Func.</param>
    /// <param name="preserveErrorHeader">Preserve line, field name header in error message.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> OverrideErrorMessage(Func<TRow, string> msgErrorFunc, bool preserveErrorHeader = false)
    {
        this.preserveErrorHeader = preserveErrorHeader;
        this.methods.Last().OverrideErrorMessage = (current) => msgErrorFunc(current);
        return this;
    }

    /// <summary>
    /// Check if property is not null.
    /// </summary>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> IsNotNull()
    {
        StringMethods<TRow> method = new StringMethods<TRow>((x) => this.getter(x));
        method.IsNotNull();
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property is not null or empty.
    /// </summary>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> IsNotNullOrEmpty()
    {
        StringMethods<TRow> method = new StringMethods<TRow>((x) => this.getter(x));
        method.IsNotNullOrEmpty();
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property is convertible to decimal.
    /// </summary>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> TryParseDecimal()
    {
        DecimalMethods<TRow> method = new DecimalMethods<TRow>((x) => this.getter(x));
        method.TryParseDecimal();
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property is convertible to DateTime.
    /// </summary>
    /// <param name="format">specified format.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> TryParseDateTime(string format)
    {
        DateTimeMethods<TRow> method = new DateTimeMethods<TRow>((x) => this.getter(x));
        method.TryParseDateTime(format);
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property has required length.
    /// </summary>
    /// <param name="min">min length.</param>
    /// <param name="max">maw length.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> HasLength(int min, int max)
    {
        StringMethods<TRow> method = new StringMethods<TRow>((x) => this.getter(x));
        method.HasLength(min, max);
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property is in list values.
    /// </summary>
    /// <param name="values">List of values.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> IsStringValues(string[] values)
    {
        StringMethods<TRow> method = new StringMethods<TRow>((x) => this.getter(x));
        method.IsStringValues(values);
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Check if property value match regex.
    /// </summary>
    /// <param name="pattern">Regex pattern.</param>
    /// <param name="options">Regex options.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> TryRegex(string pattern, RegexOptions options = RegexOptions.None)
    {
        StringMethods<TRow> method = new StringMethods<TRow>((x) => this.getter(x));
        method.TryRegex(pattern, options);
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Add a condition on a validation action of <see cref="IPropertyValidatorAction"/>.
    /// </summary>
    /// <param name="ifFunc">Condition as a predicate.</param>
    /// <param name="action">
    /// Validation action from <see cref="IPropertyValidatorAction{TRow}"/> interface.
    /// Action may add 1 to N methods.
    /// </param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> If(Func<TRow, bool> ifFunc, Action<IPropertyValidatorAction<TRow>> action)
    {
        // track method count before and after to link condition on all new methods.
        var before = this.methods.Count;
        action(this);
        var after = this.methods.Count;
        var nbAdded = after - before;

        // take last N methods
        var newMethods = this.methods.Skip(Math.Max(0, after - nbAdded));

        // add condition on new methods
        foreach (var item in newMethods)
        {
            item.Condition = (current) => ifFunc(current);
        }

        return this;
    }

    /// <summary>
    /// Add a condition on a custom validation action.
    /// </summary>
    /// <param name="predicate">Condition as a predicate.</param>
    /// <param name="tocheck">tocheck if condition is ok. TRow as the current row.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> If(Func<TRow, bool> predicate, Func<TRow, bool> tocheck)
    {
        MethodValidator<TRow> method = new MethodValidator<TRow>((x) => this.getter(x));
        method.Condition = (current) => predicate(current);
        method.ToCheck = (current) => tocheck(current);
        method.ErrorMessage = (current) => Translation.IfError;
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Add a condition on a custom validation action.
    /// </summary>
    /// <param name="predicate">Condition as a predicate.</param>
    /// <param name="tocheck">todo if condition is ok.</param>
    /// <returns>Current instance.</returns>
    public PropertyValidator<TRow> If(Func<TRow, bool> predicate, Func<TRow, dynamic, bool> tocheck)
    {
        MethodValidator<TRow> method = new MethodValidator<TRow>((x) => this.getter(x));
        method.Condition = (current) => predicate(current);
        method.ToCheck = (current) => tocheck(current, this.extraObject);
        method.ErrorMessage = (current) => Translation.IfError;
        this.methods.Add(method);
        return this;
    }

    /// <summary>
    /// Validate the property with the current row.
    /// </summary>
    /// <param name="row">Current row.</param>
    public void Validate(TRow row)
    {
        this.ValidationErrors = new List<ValidationError>();

        foreach (var method in this.methods)
        {
            bool condition = true;
            if (method.Condition != null)
            {
                condition = method.Condition(row);
            }

            if (condition)
            {
                bool ok = method.ToCheck(row);
                if (!ok)
                {
                    this.ValidationErrors.Add(ValidationError.Failure(this.fieldName, this.MessageErrorFactory(row, method)));

                    break; // by default breaks if error
                }
            }
        }
    }

    /// <summary>
    /// Create an error message.
    /// </summary>
    /// <param name="current">Current element.</param>
    /// <param name="messageError">Error informations from method. <see cref="IMethodMessageError{TRow}"/>.</param>
    /// <returns>Error message.</returns>
    private string MessageErrorFactory(TRow current, IMethodMessageError<TRow> messageError)
    {
        StringBuilder errorMsg = new StringBuilder();

        // when overriden error message suppress header by default
        if (this.preserveErrorHeader)
        {
            if (this.rowIndex.HasValue)
            {
                errorMsg.Append($"{Translation.Row} {this.rowIndex} ");
            }

            errorMsg.Append($"{Translation.Field} {this.fieldName} : ");
        }

        // default or override msg
        if (messageError.OverrideErrorMessage == null)
        {
            errorMsg.Append(messageError.ErrorMessage(current));
        }
        else
        {
            errorMsg.Append(messageError.OverrideErrorMessage(current));
        }

        return errorMsg.ToString();
    }
}
  • The If methods should look for the last method added to the collection. I do not like this way of adding the condition on the MethodValidtor as it does not seem robust and reliable

The MethodValidator defines delegates to get the value, the method for validation and and optional condition.

The concrete implemenation is done in sub classes of the MethodValidator.

/// <summary>
/// Define a method to validate a property.
/// </summary>
/// <typeparam name="TRow">Type to validate.</typeparam>
public class MethodValidator<TRow> : IMethodMessageError<TRow>
{
    /// <summary>
    /// Initializes a new instance of the <see cref="MethodValidator{TRow}"/> class.
    /// </summary>
    /// <param name="value">delegate to get the value to validate.</param>
    public MethodValidator(Func<TRow, string> value)
    {
        this.Value = value;
    }

    /// <summary>
    /// Gets a method returning the value to validate.
    /// </summary>
    public Func<TRow, string> Value { get; private set; }

    /// <summary>
    /// Gets or sets a condition prior to check if property value is valid.
    /// </summary>
    public Func<TRow, bool> Condition { get; set; }

    /// <summary>
    /// Gets or sets a method to check if property value is valid.
    /// </summary>
    public Func<TRow, bool> ToCheck { get; set; }

    /// <summary>
    /// Gets or sets a method to define a custom error message.
    /// <summary>
    public Func<TRow, string> OverrideErrorMessage { get; set; }

    /// <summary>
    /// Gets or sets a method to define the final error message.
    /// <summary>
    public Func<TRow, string> ErrorMessage { get; set; }
}

The whole source code is here : https://github.com/NicoDvl/StringContentValidator

What do you think of that ? How to improve the design, the source code ? Could you find the library useful ? What am I missing ?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! The code that you have posted in the question is just an example usage, rather than the implementation of the validator. Therefore, I am voting to close this question due to the code not being included. \$\endgroup\$ – 200_success Nov 29 '18 at 23:59
  • \$\begingroup\$ @200_success I'll include more parts of the implementation. I did not want it to be too long. \$\endgroup\$ – NicoD Nov 30 '18 at 0:02
  • \$\begingroup\$ @200_success I added parts of the implementation and precised some questions. Do not hesitate to tell me if it's better or not. Regards. \$\endgroup\$ – NicoD Nov 30 '18 at 1:15
  • \$\begingroup\$ Thanks for adding the code. It's much better now. Close vote retracted. \$\endgroup\$ – 200_success Nov 30 '18 at 5:50
5
\$\begingroup\$

I'll take time to review just one of the APIs which I find is the most confusing one:

public void Validate(TRow row)
{
    this.ValidationErrors = new List<ValidationError>();

    foreach (var method in this.methods)
    {
        bool condition = true;
        if (method.Condition != null)
        {
            condition = method.Condition(row);
        }

        if (condition)
        {
            bool ok = method.ToCheck(row);
            if (!ok)
            {
                this.ValidationErrors.Add(ValidationError.Failure(this.fieldName,this.MessageErrorFactory(row, method)));

                break; // by default breaks if error
            }
        }
    }
}

This method does some very strange things:

  • calls .Condition that returns a condition
  • if condition is true then ToCheck returns a bool and if that one is false the validation failes
  • stops evaluating other methods on first error without any particular reason

There is not a single helpful name here. Every single line just makes it worse. But at least you have commented these APIs so that one can look there for an explanation:

/// <summary>
/// Gets or sets a condition prior to check if property value is valid.
/// </summary>
public Func<TRow, bool> Condition { get; set; }

OK, I understand it's a pre-validation-condition but why do we need this at all? This should be part of the validation logic. A value is either valid or not. This puts it somehow inbetween. One could argue that it's inconclusive when Condition is false but I'd say if it's not invalid then it's valid. How else would I handle the inconclusive state?

I would remove this step or if you insist on keeping it than you should rename it to something like CanValidate.

/// <summary>
/// Gets or sets a method to check if property value is valid.
/// </summary>
public Func<TRow, bool> ToCheck { get; set; }

This API also needs to be changed. Since this is the actual validation method then you should name it IsValid.

break; // by default breaks if error

There is still this unexplained break. I see what it's doing so this comment does not help me to understand the logic. You need to tell the reader why this is the default. To me, a casual reader, it's simply weird that it cannot continue with other checks.

When you apply these suggestions and squeeze the conditions together then the method can be shortend to this nice LINQ:

public void Validate(TRow row)
{
    ValidationErrors = 
        (
            from method in methods
            where method.CanValidate(row) && !method.IsValid(row)
            select ValidationError.Failure(fieldName, MessageErrorFactory(row, method))
        ).ToList();
}  
\$\endgroup\$
  • \$\begingroup\$ Perfectly right, I will improve the naming : .Condition that return a condition by .PreCondition that return a result. This PreCondition is used to condition a Validation eg : .If(x => x.Type == "P", p.IsNotNull().TryParseDecimal())); that's why it's not included in the validation. The unexplained break stands for : stop validation on first failure. It could be helpfull if a validator depends on the previous validator to succeed. I agree it should not be the default and need clarification. \$\endgroup\$ – NicoD Nov 30 '18 at 9:24
  • \$\begingroup\$ @NicoD I agree it should not be the default - not quite what I mean ;-) it can be default but there has to be a reason for that. If you have one then it's perfectly fine, just put this reason as a comment above the break - you virtually should write // by default breaks if error because... ;-] \$\endgroup\$ – t3chb0t Nov 30 '18 at 9:26
  • \$\begingroup\$ I really think it should not be the default but seems easier for the first draft ;). I will update this part and comments. What is the best way to share corrections: I update the code in my question? \$\endgroup\$ – NicoD Nov 30 '18 at 9:31
  • \$\begingroup\$ @NicoD I update the code in my question? - no, please don't - it's not allowed when there are already answers posted - the best you can do is to wait a couple of days for more reviews to come and when you think there is some helpful information you can accept one answer and post your new code either as a self-answer or a new question is you'd like to have one more review. \$\endgroup\$ – t3chb0t Nov 30 '18 at 9:34
  • 1
    \$\begingroup\$ That makes sense. I will update the Git repository and I'll keep you informed if you want to take a look. Thank you for the review :) \$\endgroup\$ – NicoD Nov 30 '18 at 9:39

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.