3
\$\begingroup\$

Verifying the user's input is almost always required, even in really simple apps such as console calculator. Due to the wide variety of scenarios where this is useful I decided to make a few classes that will make the process easier.

I currently have 2 classes for validation, one for input that will require parsing and one for input that is already parsed to the specified type. They both inherit a common interface:

public interface IInputValidator<T> 
{
    ValidationResult<T> Validate();
}

Where the ValidationResult class is implemented as follows:

public class ValidationResult<T>
{
    public bool Success { get; }
    public T Value { get; }

    public ValidationResult(bool success, T value)
    {
        Success = success;
        Value = value;
    }

    public ValidationResult(bool success) : this(success, default(T))
    {
    }
}

The InputValidatorUnparsed<TSource, TValue> class which deals with input that needs to be parsed before operated on:

public class InputValidatorUnparsed<TSource, TValue> : IInputValidator<TValue>
{
    public delegate bool InputTryParse(TSource input, out TValue value);

    private readonly InputTryParse inputTryParse;

    private readonly Func<TSource> _getUnparsedValue;
    private Action _onFailedAction;
    private IEnumerable<TValue> _allowedItems = Enumerable.Empty<TValue>();
    private IEqualityComparer<TValue> _comparer = EqualityComparer<TValue>.Default;

    public InputValidatorUnparsed(Func<TSource> getUnparsedValue, InputTryParse tryParse)
    {
        inputTryParse = tryParse ?? throw new ArgumentNullException(nameof(tryParse));
        _getUnparsedValue = getUnparsedValue;
    }

    public InputValidatorUnparsed<TSource, TValue> WithFailedAction(Action onFailedAction)
    {
        _onFailedAction = onFailedAction;
        return this;
    }

    public InputValidatorUnparsed<TSource, TValue> WithAllowedItems(IEnumerable<TValue> allowedItems)
    {
        return WithAllowedItems(allowedItems, _comparer);
    }

    public InputValidatorUnparsed<TSource, TValue> WithAllowedItems(IEnumerable<TValue> allowedItems,
        IEqualityComparer<TValue> comparer)
    {
        _allowedItems = allowedItems ?? throw new ArgumentNullException(nameof(allowedItems));
        _comparer = comparer ?? throw new ArgumentNullException(nameof(comparer));
        return this;
    }

    public ValidationResult<TValue> Validate()
    {
        var parsingSuccess = inputTryParse.Invoke(_getUnparsedValue.Invoke(), out TValue value);
        if (parsingSuccess && IsAllowedItem(value))
        {
            return new ValidationResult<TValue>(true, value);           
        }
        _onFailedAction?.Invoke();
        return new ValidationResult<TValue>(false, value);
    }

    private bool IsAllowedItem(TValue item)
    {
        return !_allowedItems.Any() || _allowedItems.Any(i => _comparer.Equals(i, item));
    }
}

Example usage:

var validator = new InputValidatorUnparsed<string, int>(Console.ReadLine, int.TryParse)
    .WithFailedAction(() => Console.WriteLine("Invalid input please try again."))
    .WithAllowedItems(Enumerable.Range(1, 10));
var result = validator.Validate();
while (!result.Success)
{
    result = validator.Validate();
}
Console.WriteLine($"Correct input = {result.Value}");

The InputValidatorParsed<TValue> which deals with input that wont required parsing to be operated on:

public class InputValidatorParsed<TValue> : IInputValidator<TValue>
{
    private readonly Predicate<TValue> validator;
    private readonly Func<TValue> getInputValue;
    private Action _onFailedAction;
    private IEnumerable<TValue> _allowedItems = Enumerable.Empty<TValue>();
    private IEqualityComparer<TValue> _comparer = EqualityComparer<TValue>.Default;

    public InputValidatorParsed(Func<TValue> getValue, Predicate<TValue> validator)
    {
        getInputValue = getValue ?? throw new ArgumentNullException(nameof(getValue));
        this.validator = validator;
    }

    public InputValidatorParsed(Func<TValue> getValue) : this(getValue, null)
    {
    }

    public InputValidatorParsed<TValue> WithFailedAction(Action onFailedAction)
    {
        _onFailedAction = onFailedAction;
        return this;
    }

    public InputValidatorParsed<TValue> WithAllowedItems(IEnumerable<TValue> allowedItems)
    {
        return WithAllowedItems(allowedItems, _comparer);
    }

    public InputValidatorParsed<TValue> WithAllowedItems(IEnumerable<TValue> allowedItems,
        IEqualityComparer<TValue> comparer)
    {
        _allowedItems = allowedItems ?? throw new ArgumentNullException(nameof(allowedItems));
        _comparer = comparer ?? throw new ArgumentNullException(nameof(comparer));
        return this;
    }

    public ValidationResult<TValue> Validate()
    {
        var value = getInputValue.Invoke();
        if (validator == null)
        {
            return new ValidationResult<TValue>(IsAllowedItem(value), value);
        }
        if (validator.Invoke(value) && IsAllowedItem(value))
        {
            return new ValidationResult<TValue>(true, value);
        }

        _onFailedAction?.Invoke();
        return new ValidationResult<TValue>(false, value);
    }

    private bool IsAllowedItem(TValue item)
    {
        return !_allowedItems.Any() || _allowedItems.Any(i => _comparer.Equals(i, item));
    }
}

Example usage:

var validator = new InputValidatorParsed<string>(Console.ReadLine, t => !string.IsNullOrEmpty(t))
    .WithAllowedItems(new []{"value"})
    .WithFailedAction(() => Console.WriteLine("Invalid input please try again."));
var result = validator.Validate();
while (!result.Success)
{
    result = validator.Validate();
}
Console.WriteLine($"Correct input = {result.Value}");

Feel free to comment on anything, but I have few concerns in mind:

  1. I'm not happy with the naming of the classes.

  2. I do like the initialization of such object but I don't like the usage of it. Those classes will mostly be used in while loops I can imagine and the syntax for that isn't really pretty if you want to obtain the value of the result.

  3. There is repetition in the classes that maybe an abstract class can solve in some way but I don't think it's appropriate in this case as it will either look redundant or it will be way too restrictive for the derived classes.

\$\endgroup\$
  • \$\begingroup\$ one thing i noticed is that the ValidationResult class doesn't actually do anything all - it does is hold values - which suggests that it's possibly a redundant class \$\endgroup\$ – BKSpurgeon Jun 7 '17 at 21:53
  • \$\begingroup\$ That's the whole point of the class @BKSpurgeon. \$\endgroup\$ – Denis Jun 8 '17 at 2:35
  • \$\begingroup\$ i'll have to have a closer look later, but as a general OOP rule, classes which just hold data and nothing else should be looked upon with a good degree of skepticism. \$\endgroup\$ – BKSpurgeon Jun 8 '17 at 3:03
  • \$\begingroup\$ Would you rather return a KeyValuePair<TKey, TValue> or a Tuple<T1, T2>? None of those have meaningful names for they're properties. \$\endgroup\$ – Denis Jun 8 '17 at 3:21
  • \$\begingroup\$ hi there - i had a closer look but couldn't get your code to compile? \$\endgroup\$ – BKSpurgeon Jun 8 '17 at 5:10
2
\$\begingroup\$

I don't think there should be two validators as they are nearly identical. The only difference between them is the parsing part. They are validators so they should get a value ready for validation and not try to parse anything. It's the responsibility of a parser to know how to convert/parse one type into another one.

The validators also mix the builder pattern with a normal object. Methods like WithAllowedItems should be only used by a builder and not the actual object that should either have properties for that values or be immutable and require all parameters via a constructor.

I find that the semi-builder pattern makes it confusing to use because at the end I expect to call ToInputValidator or Build which are not there because I'm constructing the final object with the WithX methods. What makes it even more confusing is that some arguments are already required by the constructor which means that the WithX parameters are optional and could be simply properties which are (at least to me) more natural to initialize with an object initializer then by calling methods as if it was a builder.

What I expect is either this API

var validator = 
    InputValidator<string>
        .Builder
        .Condition(Console.ReadLine, t => !string.IsNullOrEmpty(t))
        .WithAllowedItems(new []{"value"})
        .WithFailedAction(() => Console.WriteLine("Invalid input please try again."))
        .Build(); // Throws InvalidOperationException if Condition not specified.

or that one

var validator = new InputValidator<string>(Console.ReadLine, t => !string.IsNullOrEmpty(t))
{
    AllowedItems = new []{"value"},
    FailedAction = () => Console.WriteLine("Invalid input please try again.")
};
\$\endgroup\$
  • \$\begingroup\$ I'd like to see an example of a single class doing both things without making the initialization looking horrible, maybe you can handle it better, but I couldn't come up with good design for that. Absolutely every WithX method is optional, it wont be a good idea to force the user to enter them in the constructor due to this very reason. An object intializer with all properties being set there will particularly solve the previous problem but it can cause harm to the class. For example avoiding null checks, unless you prefer to over-couple the property setter. I do agree for the semi-builder. \$\endgroup\$ – Denis Jun 8 '17 at 15:31

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.