Filtering and Validating value in SETTER

Question

I have been working on a side project for input filtering/Validation.

First I would like to go over my interface.

public interface IValidatable<T> : IValidatable
{
 MyAction<T> ModifyInput { get; set; }
 Predicate<T> PreCheck { get; set; }
 bool Equals(object obj);
 int GetHashCode();
}
public interface IValidatable
{
}

The action is used to filter or modify the input data, the predicate is used to check the input meets a bool condition, and standard overrides.

Next up is the implementation of the class. This class provides the backing field and proxy to input of that field. The private data field becomes the instance of the type you pass in with builder class function AddColumnToHeader.

The new thing here is the private function SetData that is ran on sets of the Data Property, which in turn gets a private field, and sets a private field based on configuration passed in, by the builder class function AddPredicateToColumn and AddHandlerToColumn.

public class Validatable<T> : IValidatable<T>
{
 public Validatable()
 {
 }
 private T data = default;
 public T Data { get => data; set => data = SetData(value); } 
 public Predicate<T> PreCheck { get; set; } = default;
 public MyAction<T> ModifyInput { get; set; } = default;
 private T SetData(T _value)
 {
 if (ModifyInput == default)
 {
 if (PreCheck != default)
 {
 if (PreCheck(_value) == true)
 {
 return _value;
 }
 return default;
 }
 return _value;
 }
 else
 {
 ModifyInput(ref _value);
 if (PreCheck != default)
 {
 if (PreCheck(_value) == true)
 {
 return _value;
 }
 return default;
 }
 return _value;
 }
 }
 public override bool Equals(object other)
 {
 Validatable<T> column = other as Validatable<T>;
 return column != null &&
 EqualityComparer<T>.Default.Equals(Data, column.Data);
 }
 public override int GetHashCode()
 {
 return HashCode.Combine(Data);
 }
 public static bool operator ==(Validatable<T> column1, Validatable<T> column2)
 {
 return EqualityComparer<Validatable<T>>.Default.Equals(column1, column2);
 }
 public static bool operator !=(Validatable<T> column1, Validatable<T> column2)
 {
 return !(column1 == column2);
 }
 } 

The next class is my builder class. This class builds out the Validatable objects that will be used to input validate the underlying object. It does this using the builder pattern.

The pattern I used for this follows. AddColumnToHeader<T> returns the this, so you can chain call it until the number of types fits your needs, and in the last call we have to set true to the 2nd parameter _typeCompleted, to signal the builder can move on to adding predicates and handlers.I do have a need for it to be split up in this way. I want GetTypedRow to return a deep clone of the HeaderContainer object you have defined, enforcing a immutable type on to it, and returning it.. You could remove 2nd parameter from AddColumnToHeader<T> and create and add dynamically, without issue I suppose.

public delegate void MyAction<T>(ref T Parameter);
public sealed class ValidatableBuilder
{
 private Validatable<T> HeaderColumnFactory<T>() => new Validatable<T>();
 public IQueryable<(string Name, IValidatable Value, Type ColumnType)> QueryableHeader { get => HeaderContainer.AsQueryable(); }
 private List<(string Name, IValidatable Value, Type ColumnType)> HeaderContainer { get; set; } = default;
 private bool TypesCompleted = false;
 private bool SetupCompleted = false;
 public ValidatableBuilder()
 {
 HeaderContainer = new List<(string Name, IValidatable Value, Type ColumnType)>();
 }
 public ValidatableBuilder AddColumnToHeader<T>(string _columnName, bool _typesComplete = false) 
 {
 if (!TypesCompleted)
 {
 if (typeof(T).IsValueType)
 {
 if (!HeaderContainer.Exists(x => x.Name == _columnName))
 {
 HeaderContainer.Add((_columnName, HeaderColumnFactory<T>(), typeof(T)));
 if (_typesComplete)
 {
 TypesCompleted = true;
 }
 } 
 }
 }
 return this;
 }
 public ValidatableBuilder AddPredicateToColumn<T>(string _columnName, Predicate<T> _predicate)
 {
 if(TypesCompleted)
 {
 Validatable<T> validatable = QueryableHeader.FirstOrDefault(x => x.Name == _columnName).Value as Validatable<T>;
 validatable.PreCheck = _predicate;
 }
 return this;
 }
 public ValidatableBuilder AddHandlerToColumn<T>(string _columnName, MyAction<T> _action)
 {
 if (TypesCompleted)
 {
 Validatable<T> validatable = QueryableHeader.FirstOrDefault(x => x.Name == _columnName).Value as Validatable<T>;
 validatable.ModifyInput = _action;
 }
 return this;
 }
 private void SetupComplete()
 {
 if (HeaderContainer.Count > 0)
 {
 if (TypesCompleted)
 {
 SetupCompleted = true;
 }
 }
 }
 public IQueryable<(string Name, IValidatable Value, Type DataType)> GetTypedRow()
 {
 if (!SetupCompleted)
 {
 SetupComplete();
 if (SetupCompleted)
 {
 return QueryableHeader;
 }
 return default;
 }
 else
 {
 return QueryableHeader;
 }
 } 
} 

Testing class, is not much in the way of finished, but it shows usage of the code and that is important.

First up, I create a ValidatableBuilder then I use it to add 2 columns of type ReadonlyMemory and int. I then add a condition to the int setter to say this value must be 106 to be set, and for the ReadOnlyMemory setter I tell it length must be in range 0(exclusive) to 11(inclusive). I then iterate over the Queryable I return from GetTypedRow and try to set the Property Data to incorrect values and assert against those values.

public class ValidationTests
{
 public ValidationTests()
 {
 ValidatableBuilder builder = new ValidatableBuilder();
 void PredicateTests()
 {
 builder
 .AddColumnToHeader<ReadOnlyMemory<byte>>("Test")
 .AddColumnToHeader<int>("Test2", _typesComplete: true)
 .AddPredicateToColumn<int>("Test2", x => x == 106)
 .AddPredicateToColumn<ReadOnlyMemory<byte>>("Test", x => x.Length > 0 & x.Length <= 11);
 foreach( var (Name, Value, DataType) in builder.GetTypedRow())
 {
 if(Value is IValidatable<ReadOnlyMemory<byte>>)
 {
 var ourValue = Value as Validatable<ReadOnlyMemory<byte>>;
 ourValue.Data = new ReadOnlyMemory<byte>(Encoding.UTF8.GetBytes("Hello World!"));
 Debug.Assert(ourValue.Data.Span.SequenceEqual(Encoding.UTF8.GetBytes("Hello World!")), "Not Equal");
 }
 else if(Value is IValidatable<int>)
 {
 var ourValue = Value as Validatable<int>;
 ourValue.Data = 105;
 Debug.Assert(ourValue.Data == 105, "Not Equal");
 }
 }
 }
 PredicateTests();
 }
}

I tried to incorporate everything, but if I left something out or something is unclear, please let me know. All input is welcomed. :)

Edit: In response to Pieter's questions, I got further insight into where I could improve and the goals and completeness of my tool. I have decided to rename it ProgrammableSetter and instead of working directly towards validation before setting a property, I will change direction and work towards making it as Configurable and reusable as possible, it will probably take me a few days to retool/rewrite. Thanks for the valued input.

• 200_success: This code seems rather sketchy and unfinished. It's hard to say what exactly we are to review.
• BanMe: It is finished up to the point I want to have someone else review it. The code semantics, the code layout, you can review any part of it. What can I do to make the code less sketchy?
• pacmaninbw: Does the code work, we can only review working code, also it would be best to have full classes. Please see codereview.stackexchange.com/help/dont-ask.
• 200_success: There are a lot of TODO placeholders. We can't really review your intentions; I'd prefer to see the placeholders filled in or removed.
• Pieter Witvoet: @BanMe: in what context is this validation code meant to be used? What's the point of ModifyInput: why can't a caller do that before calling SetData? And how are you going to detect validation failures if PreCheck failures are not reported?

Answer 1

IValidatable<T>

• ModifyInput and PreCheck are problematic: their public setters allow any code to modify or disable sanitizing and validation. Surely that's something that only the initializing code should be able to do? I also don't see why these properties need to be exposed at all: they're implementation details.
• Why does IValidatable<T> contain an Equals and GetHashCode method? They're not used anywhere.
• Because of the above two issues, IValidatable<T> is rather pointless in its current shape. Why doesn't it contain a T Data { get; set; } property? And if you're only ever going to have one implementation then I don't see much use for an interface. Validatable<T> seems flexible enough.

Validatable<T>

• You don't need to initialize properties with = default - that already happens by default.
• I'd expect Validatable<T>'s constructor to accept modifyInput and preCheck as arguments, and to store them in private fields.
• Why does ModifyInput take a ref parameter instead of returning the result? This prevents you from using lambda's.
• SetData is a confusing name: it doesn't actually set Data - it returns a sanitized and validated value.
• Most people would expect a setter to store the value given to it. Getting a different value back would be rather surprising - and not in a good way. Consider using a clearly named method instead: v.Value = 105 versus v.SanitizeAndStore(105).
• Returning default when validation fails is problematic: is 0 an intended, valid value, or the result of a validation failure? I'd expect an exception to be thrown instead, or some other failure-handling mechanism. I would also clearly document the chosen behavior.
• SetData can be simplified to just ModifyInput?.Invoke(ref _value); return (PreCheck?.Invoke(_value) == false) ? default : _value;.
• A boolean is already either true or false, so the == true in your code is superfluous. Note that ?. returns a nullable bool, which is why the == false part above is necessary.
• In Equals, instead of using as and a null-check, you can use pattern matching instead: return other is Validatable<T> column && ...;.

ValidatableBuilder

• Why do QueryableHeader and GetTypesRow return an IQueryable<>? It's an in-memory list, so IEnumerable<> would make more sense.
• GetTypedRow always returns the same row, including data that was set previously. I can't imagine that to be the intended behavior.
• Property { get => ...; } can be simplified to Property => ...;.
• Why explicitly initialize HeaderContainer to default, only to override that in the constructor? Just initialize it to a new list directly.
• The various Add methods are very brittle: they fail silently depending on whether TypesCompleted is true or not. If a method cannot be called when an object is in a certain state, then I'd expect an exception to be thrown, but I'd much rather look for a better design. Perhaps a CreateRowBuilder method that returns a RowBuilder with the current configuration, which can then be used to create rows.
• Likewise, silently ignoring reference types is brittle. Use a generic type constraint instead, so you can prevent the use of reference types at compile-time.
• Why have separate methods for adding columns, handlers and predicates? Why not add optional arguments for handlers and predicates to AddColumnToHeader?

ValidationTests

• Inside that foreach loop, you're using both is and as, but with different types. An IValidatable<T> is not necessarily a Validatable<T> - it could be any other type that implements IValidatable<T>. Note also that pattern matching can be used here: switch (Value) { case Validatable<ReadOnlyMemory<byte>> vmem: vmem.Data = ...; case Validatable<int> vint: vint.Data = ...; }.

Other notes

• Leading underscores are typically used for private fields. Parameter names are written in camelCase, without leading underscore. PascalCase is used for property names, not fields.
• I prefer to add a bit of whitespace between methods and properties, especially when they have different visibilities (public/private).
• There's a general lack of documentation/comments.

Alternative approach

A property that silently modifies or rejects data is quite surprising. I'd rather see code that is more obvious about what it does:

var sanitizedValue = column.Sanitize(inputValue);
if (!column.IsValid(sanitizedValue))
{
 // log, throw error, ...
}
else
{
 // store value
}

• BanMe: This is excellent stuff, thank you so much for spending the time. I will take all that you say and try and address each point as best as I can.
• BanMe: ModifyInput and PreCheck are problematic: Many are in agreement on this point. I have moved the function set to the contstructor for my class. I now have 4 optional parameters to address this issue. Why does IValidatable<T> contain an Equals and GetHashCode method? Not sure where I was going, removed from next Implementation. I was just updating the Interface definitions now. new one will have T Data and T Set(T Value) defined, this gives me the option to setup the validation and not use the inner field to receive the value
• BanMe: Validatable<T>: 1st point: Interesting I did not know that about C#, this can definitely reduce my typing. 2nd point: Addressed last comment, 3rd: ModifyInput has been adjusted to not take a reference but to return the modified value to a local variable for future use.4th I definitely agree it is misleading, I have a hard time naming things, I will try to Describe functions by their Impl better.5th this makes a good point, SanitizeOrSetAndStore? 6th: this should be addressed by the new Success/Fail Callbacks in my next version.