current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

I have a generic extension method for validating my entities. The main idea is to be able to specify at runtime (context related) the criteria for validating a specific entity (with the end goal of saving it in the data base).

public static Boolean IsValidEntity<T>(this T entity, List<Predicate<T>> predicates)
{
    Boolean res = true;
    predicates.ForEach(p => res &= (Boolean)p.DynamicInvoke(entity));
    return res;
}

Inside my repository I can then use (simplified as this is not the scope but is somewhat necessary for the question context): 

public void SaveIntProperty(int myProperty, List<Predicate<int>> predicates)
{
    if (myProperty.IsValidEntity<int>(predicates))
    {
        //save to data base or other actions
    }
}

A simple example on a list of ints would be to search for elements which (in a particular context) need to be both even and less than 8. 

List<int> numbers = new List<int>() { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

//dynamically create the validation criteria
List<Predicate<int>> predicates = new List<Predicate<int>>();// List<Delegate>();
Predicate<int> lessThan = x => x < 8;
predicates.Add(lessThan);
Predicate<int> even = x => x % 2 == 0;
predicates.Add(even); 

numbers.ForEach(n => SaveIntProperty(n, predicates));

The IsValidEntity extension method works and looks simple enough. However, I'm having doubts about the performance and the extensibility. Is the use of the ForEach ok? Should I pass the predicates differently? Should I use something else instead of predicates?

share|improve this question
2  
There is no reason to use List.ForEach(), just use a normal foreach, it's actually even few characters shorter. – svick Mar 13 '14 at 13:24
up vote 5 down vote accepted

You can simplify your extension method a bit as so:

public static Boolean IsValidEntity<T>(this T entity, IEnumerable<Predicate<T>> predicates)
{
    return predicates.All(p => (bool)p.DynamicInvoke(entity));
}

This is a bit better for readability, and shouldn't have any cost in efficiency as LINQ short-circuits with All. I also switched your List for IEnumerable, just to save an unnecessary ToList call if you end up with some other kind of collection.

You should also check if you really need that DynamicInvoke. If not, you can further simplify to

public static Boolean IsValidEntity<T>(this T entity, IEnumerable<Predicate<T>> predicates)
{
    return predicates.All(p => p(entity));
}

Depending on your wider design, this might be enough. However, since you asked about extensibility, you can see that having to build predicates isn't the nicest business. For example, you had your 'less than 8' predicate. What if elsewhere in the code you wanted 'less than 7'? You'd have to repeat the (albeit very simple in this example) logic, just with a different number.

If you tried to refactor this to adhere to the Don't Repeat Yourself principle, you might then end up with something like a PredicateFactory class which exists purely to build common predicates based on parameters. But this isn't fantastic design either. It would likely end up as a monolith of public methods without much cohesion, and violate the open/closed and single responsibility principle.

A better option might be to create an interface like:

interface IEntityValidator<T>
{
    bool IsValid(T Entity);
}

Instead of predicates, you can now create and use classes which implement this interface. This gives better adherence to the design principles I mentioned before, and allows you take full advantage of polymorphism (for more complicated validation you might find that inheritance is useful, for example). At this point your original method would be:

public static Boolean IsValidEntity<T>(this T entity, IEnumerable<IEntityValidator<T>> validators)
{
    return validators.All(v => v.IsValid(entity));
}

To my mind, that line is simple and readable enough that it may no longer warrant its own method, especially its own extension method which will apply to every single type. Validation doesn't seem like something that would be done in too many places, so even if you didn't like that line, you might just move it to a private method somewhere. That's more a matter of taste, though.

share|improve this answer
    
I don't see how does the interface help anything. I think that pretty much everything you can do with a single method interface, you can also do with a delegate. For example, you can still have each validator in a separate class, you would just use MyValidator.Validate (or new MyValidator().Validate) instead of new MyValidator(). – svick Mar 13 '14 at 13:21
    
@svick I don't think there's anything you absolutely couldn't do with a delegate instead of an interface, especially since any class could just return a delegate which references private members. But at that point it just seems like a weirder way of doing the same thing. Why have a class that returns a function as a delegate rather than just have that function be a public member? I didn't really understand what you meant with that example though, so I could be missing your point – Ben Aaronson Mar 13 '14 at 13:28
    
I don't mean a method returning a delegate, I mean using that method as a delegate. Where you would have a class that implements IEntityValidator<T>, I would have a (possibly static) method and use that method as the Predicate. E.g. Predicate<int> validator = new MyValidator().Validate; (where Validate() is the validation method) instead of IEntityValidator<int> validator = new MyValidator();. – svick Mar 13 '14 at 13:32
    
@svick Oh, right. Well I don't know Andrei's overall design, but having an interface means you don't have to hard-code dependencies. Adhering to the Dependency Inversion Principle means you can inject them, mock them in a test, use an IoC container, whatever. It's not always something that's important, but given what a lightweight thing the interface is and how little I know of the wider project design, it seems like a sensible thing to suggest by default. As a personal preference, I'd also find an interface like that very expressive in terms of readability – Ben Aaronson Mar 13 '14 at 13:38
1  
@AndreiV I'd say the IEntityValidator is appropriate then. – Ben Aaronson Mar 13 '14 at 14:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.