7
\$\begingroup\$

Inspired by this question by t3chb0t and as an elaboration of my own answer, I have written the following solution. My goal was to reduce complexity both in implementation and use. Eventually - I have to admit - the implementation ended up being rather complex - but in my flavor; but in terms of ease of use, I think I succeeded. My original idea was inspired by Railway Oriented Programming, but I don't think I can claim to conform to that in the following.

The use case is as follows:

private static void ValidationTest()
{
  var validator = Validator.For<Person>(ValidationStopConditions.RunAll)
    .WarnIfTrue(p => p.Age > 50, "Person is older than 50")
    .WarnIfFalse(p => p.Age < 50, "Person is older than 50")
    .NotNull(p => p.LastName, "LastName is null")
    .MustBeNull(p => p.LastName, "LastName should be null")
    .IsTrue(p => p.FirstName.Length > 3, "First Name is too short")
    .IsFalse(p => p.FirstName.StartsWith("Cos"), "First Name starts with Coo")
    .Match(p => p.Address.Street, @"^Sesa(m|n)e Street$", "Street Name doesn't conform to the pattern");

  DoTheValidation(validator, Tester);
}

private static void ValidationTestDefaultErrorMessages()
{
  var validator = Validator.For<Person>(ValidationStopConditions.RunAll)
    .WarnIfTrue(p => p.Age < 50, null)
    .WarnIfFalse(p => p.Age < 50, null)
    .NotNull(p => p.LastName, null)
    .MustBeNull(p => p.LastName, null)
    .IsTrue(p => p.FirstName.Length < 3, null)
    .IsFalse(p => p.FirstName.StartsWith("Coo"), null)
    .Match(p => p.Address.Street, @"^Sesa(m|n)e Street$", null);

  DoTheValidation(validator, Tester);
}

private static void DoTheValidation<T>(Validator<T> validator, T source)
{
  var result = source.ValidateWith(validator);

  Console.WriteLine("The following Errors were found: ");
  foreach (ValidateResult<T> failure in result.Where(r => (r as Success<T>) is null))
  {
    Console.WriteLine(failure);
  }
}

private class Person
{
  public string FirstName { get; set; }

  public string LastName { get; set; }

  public Address Address { get; set; }

  public int Age { get; set; }
}

private class Address
{
  public string Street { get; set; }
}

private static readonly Person Tester = new Person
{
  FirstName = "Cookie",
  LastName = "Monster",
  Age = 45,
  Address = new Address
  {
    Street = "Sesame Street"
  }
};

As shown, it's possible to add validation rules in an easy fluent manner.


The ValidationStopConditions is defined as:

  public enum ValidationStopConditions
  {
    RunAll = 1,
    StopOnFailure = 2,
    StopOnWarning = 3
  }

and determines if all rules should be run no matter what happens or if the validation stops on first failure or warning.


The Validator class looks like:

  public static class Validator
  {
    public static Validator<TSource> For<TSource>(ValidationStopConditions stopCondition = ValidationStopConditions.RunAll) => new Validator<TSource>(stopCondition);
  }

  public class Validator<T>
  {
    List<Func<T, ValidateResult<T>>> m_rules = new List<Func<T, ValidateResult<T>>>();

    public Validator(ValidationStopConditions stopCondition)
    {
      StopCondition = stopCondition;
    }

    public ValidationStopConditions StopCondition { get; }

    public IReadOnlyList<ValidateResult<T>> Validate(T source)
    {
      if (source == null) return Enumerable.Empty<ValidateResult<T>>().ToList();

      switch (StopCondition)
      {
        case ValidationStopConditions.RunAll:
          return m_rules.Select(rule => rule(source)).ToList();
        case ValidationStopConditions.StopOnFailure:
          {
            List<ValidateResult<T>> results = new List<ValidateResult<T>>();
            foreach (var rule in m_rules)
            {
              var result = rule(source);
              results.Add(result);
              if (result is Failure<T>)
                return results;
            }
            return results;
          }
        case ValidationStopConditions.StopOnWarning:
          {
            List<ValidateResult<T>> results = new List<ValidateResult<T>>();
            foreach (var rule in m_rules)
            {
              var result = rule(source);
              results.Add(result);
              if (result is Warning<T>)
                return results;
            }
            return results;
          }
        default:
          throw new InvalidOperationException($"Invalid Stop Condition: {StopCondition}");
      }
    }

    internal void AddRule(Predicate<T> predicate, string errorMessage)
    {
      Func<T, ValidateResult<T>> rule = source =>
      {
        if (predicate(source))
          return new Success<T>(source);
        return new Failure<T>(source, errorMessage);
      };
      m_rules.Add(rule);
    }

    internal void AddWarning(Predicate<T> predicate, string warningMessage)
    {
      Func<T, ValidateResult<T>> rule = source =>
      {
        if (predicate(source))
          return new Success<T>(source);
        return new Warning<T>(source, warningMessage);
      };
      m_rules.Add(rule);
    }
  }

And the rules are defined as extension methods as:

  public static class ValidationRules
  {
    // Helper method - not a rule
    private static string GetDefaultMessage(this Expression expression, string format)
    {
      ValidateExpressionVisitor visitor = new ValidateExpressionVisitor();
      visitor.Visit(expression);

      return string.Format(format, visitor.Message);
    }

    public static Validator<T> NotNull<T, TMember>(this Validator<T> validator, Expression<Func<T, TMember>> expression, string errorMessage)
    {
      errorMessage = errorMessage ?? expression.GetDefaultMessage("{0} is null");

      var getter = expression.Compile();
      Predicate<T> predicate = source => getter(source) != null;
      validator.AddRule(predicate, errorMessage);
      return validator;
    }

    public static Validator<T> MustBeNull<T, TMember>(this Validator<T> validator, Expression<Func<T, TMember>> expression, string errorMessage)
    {
      errorMessage = errorMessage ?? expression.GetDefaultMessage("{0} is not null");

      var getter = expression.Compile();
      Predicate<T> predicate = source => getter(source) == null;
      validator.AddRule(predicate, errorMessage);
      return validator;
    }

    public static Validator<T> IsTrue<T>(this Validator<T> validator, Expression<Predicate<T>> predicate, string errorMessage)
    {
      errorMessage = errorMessage ?? predicate.GetDefaultMessage("{0} is not true");

      validator.AddRule(predicate.Compile(), errorMessage);
      return validator;
    }

    public static Validator<T> WarnIfTrue<T>(this Validator<T> validator, Expression<Predicate<T>> predicate, string message)
    {
      message = message ?? predicate.GetDefaultMessage("{0} is true");

      validator.AddWarning(src => !predicate.Compile()(src), message);
      return validator;
    }

    public static Validator<T> IsFalse<T>(this Validator<T> validator, Expression<Predicate<T>> predicate, string errorMessage)
    {
      errorMessage = errorMessage ?? predicate.GetDefaultMessage("{0} is not false");

      validator.AddRule(src => !predicate.Compile()(src), errorMessage);
      return validator;
    }

    public static Validator<T> WarnIfFalse<T>(this Validator<T> validator, Expression<Predicate<T>> predicate, string message)
    {
      message = message ?? predicate.GetDefaultMessage("{0} is false");

      validator.AddWarning(predicate.Compile(), message);
      return validator;
    }

    public static Validator<T> Match<T>(this Validator<T> validator, Expression<Func<T, string>> expression, string pattern, string errorMessage)
    {
      errorMessage = errorMessage ?? $@"{expression.GetDefaultMessage("")} doesn't match pattern: ""{pattern}""";

      var getter = expression.Compile();
      Predicate<T> predicate = source => Regex.IsMatch(getter(source), pattern);
      validator.AddRule(predicate, errorMessage);
      return validator;
    }
  }

New rules can easily be added when needed.

The result of each validation can either be Success<T>, Warning<T> or Failure<T>:

  public abstract class ValidateResult<T>
  {
    public ValidateResult(T source)
    {
      Source = source;
    }

    public T Source { get; }
  }

  public class Success<T> : ValidateResult<T>
  {
    public Success(T source) : base(source)
    {
    }

    public override string ToString()
    {
      return "Everything is OK";
    }
  }

  public class Failure<T> : ValidateResult<T>
  {
    public Failure(T source, string message) : base(source)
    {
      Message = message;
    }

    public string Message { get; }

    public override string ToString()
    {
      return $"Error: {Message}";
    }
  }

  public class Warning<T> : ValidateResult<T>
  {
    public Warning(T source, string message) : base(source)
    {
      Message = message;
    }

    public string Message { get; }

    public override string ToString()
    {
      return $"Warning: {Message}";
    }
  }

The message member of Warning and Failure will be either the provided message argument to the rule or an auto generated default.


A convenient api:

  public static class ValidationExtensions
  {
    public static IReadOnlyList<ValidateResult<T>> ValidateWith<T>(this T source, Validator<T> validator)
    {
      if (source == null) throw new ArgumentNullException(nameof(source));
      if (validator == null) throw new ArgumentNullException(nameof(validator));

      return validator.Validate(source);
    }
  }

The default error/warning messages are found using a simple ExpressionVisitor:

  internal class ValidateExpressionVisitor : ExpressionVisitor
  {
    public ValidateExpressionVisitor()
    {
    }

    public string Message { get; private set; }

    protected override Expression VisitLambda<T>(Expression<T> node)
    {
      Message = node.Body.ToString();

      return base.VisitLambda(node);
    }
  }

This is very basic, and is intended only for test, development and debugging.


Any comments are welcome.

\$\endgroup\$
  • 3
    \$\begingroup\$ the implementation ended up being rather complex - it virtually always does ;-] \$\endgroup\$ – t3chb0t Jun 23 at 18:51
  • 3
    \$\begingroup\$ @t3chb0t: But I naively hope every time.. \$\endgroup\$ – Henrik Hansen Jun 23 at 18:54
  • \$\begingroup\$ What is the intended behaviour when StopOnWarning encounters an error? Currently it continues. That's not what I would have expected. \$\endgroup\$ – JAD Jun 24 at 10:02
  • \$\begingroup\$ @JAD: Good catch. Is seems counter intuitive to let an error pass but stepping out on a following warning. I nearly regret that I introduced this flag type, and I have another concept in mind. \$\endgroup\$ – Henrik Hansen Jun 24 at 10:09
4
\$\begingroup\$

Cleaner consumer interface

WarnIfTrue / WarnIfFalse

.WarnIfTrue(p => p.Age > 50, "Person is older than 50")
.WarnIfFalse(p => p.Age < 50, "Person is older than 50")

I don't see a need to create two methods for this. "if true" and "if false" is a matter of thinking like a programmer, instead of thinking like a consumer. You can achieve the same by only having one function:

.WarnIf(p => p.Age > 50, "Person is older than 50")
.WarnIf(p => p.Age < 50, "Person is younger than 50")

Any developer who wants to use your method and would be choosing between WarnIfTrue and WarnIfFalse can just as well choose to logically invert their lambda.

IsTrue / IsFalse

The same applies here:

.IsTrue(p => p.FirstName.Length > 3, "First Name is too short")
.IsFalse(p => p.FirstName.StartsWith("Cos"), "First Name starts with Coo")

which can be shortened to

.Require(p => p.FirstName.Length > 3, "First Name is too short")
.Require(p => !p.FirstName.StartsWith("Cos"), "First Name starts with Cos")

I used Require instead of Is because in my opinion Is suffers from making it unclear whether the message applies to when the statement is true or when it is false. Using Require, it's clearer that the lambda defines what must be the case, and the message applies to when the requirement is not met.

MustBeNull / NotNull

.NotNull(p => p.LastName, "LastName is null")
.MustBeNull(p => p.LastName, "LastName should be null")

I don't think you need these methods. Compared to the above IsTrue/IsFalse (or Require) methods, all you're providing to the consumer is that they don't have to write their own null check.
Comparatively, the Match method is really bringing something new to the table that would not be trivial to have to write yourself (as the consumer). But a null check is nowhere near complex enough to warrant expanding the interface.

The effort of knowing that these two additional methods exist add more complexity to your consumer's life than writing the null check does. So my suggestion is to stick to what you already had:

.Require(p => p.LastName == null, "LastName should be null")
.Require(p => p.LastName != null, "LastName cannot be null")

Tangentially, since you're checking a string, a null check usually isn't enough anyway:

.Require(p => String.IsNullOrWhitespace(p.LastName), "LastName should be null")
.Require(p => !String.IsNullOrWhiteSpace(p.LastName), "LastName cannot be null")

Keeping it DRY

Take a good look at these methods:

internal void AddRule(Predicate<T> predicate, string errorMessage)
{
  Func<T, ValidateResult<T>> rule = source =>
  {
    if (predicate(source))
      return new Success<T>(source);
    return new Failure<T>(source, errorMessage);
  };
  m_rules.Add(rule);
}

internal void AddWarning(Predicate<T> predicate, string warningMessage)
{
  Func<T, ValidateResult<T>> rule = source =>
  {
    if (predicate(source))
      return new Success<T>(source);
    return new Warning<T>(source, warningMessage);
  };
  m_rules.Add(rule);
}

the only difference between them is that you either return a Failure<T> or Warning<T> when the condition is not met. The rest of the logic is the same. This can be abstracted further.

internal void AddRule(Predicate<T> predicate, string message, bool isWarning)
{
    Func<T, ValidateResult<T>> rule = source =>
    {
        if (predicate(source))
            return new Success<T>(source);

        return isWarning
               ? new Warning<T>(source, message)
               : new Failure<T>(source, message);
    };
    m_rules.Add(rule);
}

The example I gave suffers from a potential OCP weakness. If you expand on the possibilities and add variants to the Success/Warning/Failure pattern, then you're going to have to modify this method.
It is possible to avoid that. However, I consider it quite unlikely as the green/yellow/red principle of error checking is a well-defined principle that is very commonly used.

That being said, if you do want to avoid the OCP weakness, you can do something like

public enum FailureType { Failure, Warning, NuclearLaunch }

internal void AddRule(Predicate<T> predicate, string message, FailureType failureType)
{
    Func<T, ValidateResult<T>> rule = source =>
    {
        if (predicate(source))
            return new Success<T>(source);

        return GetFailureResult(failureType, source, message);
    };
    m_rules.Add(rule);
}

private ValidateResult<T> GetFailureResult(FailureType failureType, T source, string message)
{
    switch(failureType)
    {
        case FailureType.Warning:
            return new Warning<T>(source, message);
        // ...
    }
}

Other solutions are possible too. However, the focus of this improvement was to DRY all other logic except the "failure object picking" logic, since all other logic was exactly the same.


Extension methods?

And the rules are defined as extension methods as:

Why are these rules defined as extension methods? Why not just include them in the class?

I get the feeling that you split them up to keep the class shorter. But that's not how/why you should use extension methods. It seems like you're using extension methods as a clever way to hide the additional complexity of your class.

This also leads to a compromise in accessibility (albeit minor). You've defined AddRule and AddWarning as internal. Had you added the extension methods to the class directly, you could've made them private. The difference is that by making them internal, other classes from the same assembly now have access to something they shouldn't have access to.

Following the earlier advice to reduce the methods made available to the consumer, you should end up with a shortened ruleset which makes it well acceptable to add these to the class itself and make the internal methods private.

\$\endgroup\$
  • \$\begingroup\$ Why are these rules defined as extension methods? Why not just include them in the class? - I can answer that. Because this way anyone can add their own convenience extensions. Writing each condition like x == null is counter productive. The most common cases must have shortcuts and the user must be able to add their own domain related APIs if necessary. The main class should optimally have only a single method that allows you to add validation rules. The user is the one to define them. Otherwise it would be closed for extension. \$\endgroup\$ – t3chb0t Jun 24 at 9:14
  • \$\begingroup\$ @t3chb0t: The use for extension methods to extend a class is to do so from a different assembly. OP's extension methods rely on access to internal class methods, which will not be accessible from another assembly. \$\endgroup\$ – Flater Jun 24 at 9:17
  • \$\begingroup\$ ok, that internal I didn't notice. Then it's obviously a bug. I also disagree with most of your suggestions. APIs like .Require(p => String.IsNullOrWhitespace(p.LastName), "LastName should be null") are generally necessary but having only this one makes using this utility extremely difficult and verbose. The whole purpose of having these extensions is to not having to write all that boilerplate code over and over again. \$\endgroup\$ – t3chb0t Jun 24 at 9:18
  • \$\begingroup\$ Tanks for the input. Many good considerations. You could argue for a succinct set of rules directly on the Validator class, but I thought it was more "democratic" to have them as extensions, but there are really no design reasons for that. And the internal access modifier on AddRule and AddWarning is certainly a flaw that has survived from an early stage in the process. \$\endgroup\$ – Henrik Hansen Jun 24 at 9:35
  • 2
    \$\begingroup\$ @t3chb0t: In that case, I'd still advocate Is/IsNot rather than IsTrue/IsFalse (and similar). If readable boilerplating is the goal, then the readbility needs to favor semantics (i.e. what you'd say in English) over technicalities (i.e. how a programmer describes it). \$\endgroup\$ – Flater Jun 24 at 9:36
8
\$\begingroup\$

Fluent API

Fluent APIs are generally very useful but one has to be very careful with them as there is a chance of making them overfluent. This means that you try to create an API for every possible combination like:

  var validator = Validator.For<Person>(ValidationStopConditions.RunAll)
    .WarnIfTrue(p => p.Age > 50, "Person is older than 50")
    .WarnIfFalse(p => p.Age < 50, "Person is older than 50")
    .NotNull(p => p.LastName, "LastName is null")
    .MustBeNull(p => p.LastName, "LastName should be null")
    .IsTrue(p => p.FirstName.Length > 3, "First Name is too short")
    .IsFalse(p => p.FirstName.StartsWith("Cos"), "First Name starts with Coo")
    .Match(p => p.Address.Street, @"^Sesa(m|n)e Street$", "Street Name doesn't conform to the pattern");

Instead, I think it's better to make them composable so that end-users have the freedom of creating expressions not anticipated by the API creator. (I made this mistake in my utility too (by having Null and NotNull instead of using a modifier) so I have redesigned it since).

This would both reduce the number of available APIs and the learning curve for the end-user and make also coding and testing easier because there would be much less combinations.

Consider this:

Validator
   .For<Person>()
   .True(p => p.Age > 50)
      // then modifiers can be chained...
      .Exclude() // <- or Exclude/Not/Negate etc,
      .Require() // <- upgrades this check to yield an error instead of a warning

Without such modifiers like Exclude/Not or Warn you would need to create these versions for each and every rule. Then you add a new one... and you can create it three or four times again. Now, what happens if you create a new modifier? You'll have to create even more versions of all existing APIs. You would end up with so many of them...

Consistency

There should be more consistency between the APIs. So, when there is MustBeNull then there should also be MustBeTrue instead of just IsTrue, etc.

Validation levels

I like that idea of having results other than just black-n-white but also a gray Warning inbetween. This opens a bunch of whole new possibilities such as fixing property values.

Handling validations

I think the first switch is (might be) dagerous:

  public enum ValidationStopConditions
  {
    RunAll = 1,
    StopOnFailure = 2,
    StopOnWarning = 3
  }

I haven't exactly analyzed how rules are handled but it might crash when person.FirstName is null and later person.FirstName > 3 is used. The idea of having Error rule was to break here because it's pointless to check other conditions that rely on that one. This should signal an unrecoverable validation error. But I guess it just yields through all other rules (according to ROP).

Creating & compiling expressions

Expressions can be very tricky but they are at the same time super useful for generating error messages and it's nice to see that model here too. However some of them are less useful than other. Let's take a look at this one:

  var getter = expression.Compile();
  Predicate<T> predicate = source => Regex.IsMatch(getter(source), pattern);

The generated expression string won't show the Regex.IsMatch because it's not part of the expression. Unless it's by design, I suggest the follwing approach (taken from my new APIs). Here, you build a new expression containing all calls so that they are rendered into the final string.

    public static LambdaExpression Match<T>(Expression<Func<T, string>> expression, string pattern, RegexOptions options)
    {
        var isMatchMethod = typeof(Regex).GetMethod(nameof(Regex.IsMatch), new [] { typeof(string), typeof(string), typeof(RegexOptions) });
        return
            Expression.Lambda(
                Expression.Call(
                    isMatchMethod,
                    expression.Body,
                    Expression.Constant(pattern),
                    Expression.Constant(options)),
                expression.Parameters
            );
    }

Naming

I would rename the ValidateExpressionVisitor to something more intuitive like ValidationMessageCreator. It doesn't have to have the Visitor ending as it rarely fits into what a visitor is actually doing. I suggest dropping that suffix.

\$\endgroup\$
  • \$\begingroup\$ Good thoughts. My objective was to "flatten" the use, so the api is easier to use, but you have a point. My rules isn't that thought through, but when writing them I considered to have as few as possible, for instance is the complementary IsTrueand IsFalse really both necessary? The two level chaining might be the best solution. \$\endgroup\$ – Henrik Hansen Jun 24 at 7:11
  • \$\begingroup\$ @HenrikHansen I think in case of IsTrue and IsFalse there could be made an exception as there is no negation preffix added. I would complain when I saw IsNotTrue, this actually would be pretty weird. Occasional excpetions from rules are fine if they have good reason to exist. My rules isn't that thought through - I usually start coding (on the end-user level) in notepad until I'm happy with the API and only then start real coding which then becomes much easier and faster. I can recommend this approach ;-] \$\endgroup\$ – t3chb0t Jun 24 at 7:17
4
\$\begingroup\$

This API does feel fluent for consumers to use. You have also included some features I missed in the post you were inspired by.

  • various severity levels [warning, error]
  • custom error messages (although t3chb0t did comment he was working on this)

What I'm still missing is a way to throw an exception if I want to. Currently, your API is a sand-box. You could foresee ThrowOnError and ThrowOnWarning. Perhaps also with overloads that take an exception type. If multiple errors/warnings are found, they should be wrapped in an AggregateException.

private static void DoTheValidation<T>(Validator<T> validator, T source)
{
    var result = source.ValidateWith(validator).ThrowOnError().Result;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ OK, I understand, what you mean, and I will let it be a candidate for further development, but I intentionally try to avoid throwing - adapted from the Railway pattern. But your suggested example could easily be implemented as an extension to IEnumerable<ValidateResult<T>> \$\endgroup\$ – Henrik Hansen Jun 23 at 20:21
  • 1
    \$\begingroup\$ I got the impression that there is a misunderstanding to the intention of such validators. To me, their main purpose is to validate data-objects so throwing custom exceptions is in my opinion rarely required and it's not really necessary to open this API to the end-user (although easy to code). This means that a DTO is either valid, invalid or is something inbetween where data could be corrected so the validation yields just warnings which is a new concept (to me) - but I like it very much as this is a point where possible fixing extensions could be implemented. \$\endgroup\$ – t3chb0t Jun 24 at 5:43
  • 1
    \$\begingroup\$ I also think that AggregateException is for something else. Here, we don't produce exceptions but validation results and since they are not real exceptions, they shouldn't be aggregated. Instead, there should be only one final exception telling me that a dto is invalid. The only useful thing about exceptions is their name and message anyway so they sholdn't contain any other data (based on experience). \$\endgroup\$ – t3chb0t Jun 24 at 5:55
  • 1
    \$\begingroup\$ In case of DTOs it's often even desirable to not throw; when you got hundereds of dtos where a great deal of them might be invalid it could turn out to a huge bottleck so it's good to be able to avoid exceptions. \$\endgroup\$ – t3chb0t Jun 24 at 5:55
  • 1
    \$\begingroup\$ @t3chb0t You guys are on the verge of building a compiler here. At least an 'optimizer' part handling and manipulating expressions. \$\endgroup\$ – dfhwze Jun 24 at 6:38

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.