# Tag Info

15

Instead of using regular expressions to manipulate the expression string I prefer to do expression manipulation. While this can be a little daunting at first it turns out to be fairly simple. It handles a lot more of the oddball cases. For instance, when someone does this: var builder = Validator<String>.Builder; var stringValidator = builder ....

12

builder .LessThen(() => x.Length < y.Length || x.Price < y.Price) .Equal(() => x.Length == y.Length || x.Price == y.Price) .GreaterThan(() => x.Length > y.Length || x.Price > y.Price); gives me a very bad feeling, which turned out to be justified when I saw public int Compare(T x, T y) ...

10

You should be able to cut the code length and improve performance, by simplifying the algorithm. Here are some of the problems I've noticed in your implementation: You're currently processing all of the type's properties, this is not necessary as you only need few concrete ones. There are few other problems deriving from this one. You're creating the ...

10

I really admire your efforts and I read the question as more about Expressions than comparison. Anyway: as for the comparison, you should be aware that the result is different if the initial order of the Products is changed: this: var products = new[] { new Product {Name = "Car", Price = 7 }, new Product {Name = "Table", Price = 3 }, new ...

9

Very nice implementation; I always like seeing your code here. I really only have five very minor opinions on this implementation: I'm not sold on expr as an abbreviation for expression. I'd recommend expression in its various incarnations. 1a. For that matter, t in the Create method should probably be called composedExpressions. The class constants ...

8

This is great work and very useful. I have the following comments/suggestions: 1) The usual suspects: a) return "[" + string.Join(", ", values.Select(FormatValue)) + "]"; should/could be: return $"[ {string.Join(", ", sample.Select(FormatValue))} ]"; b) if (Type.GetTypeCode(value?.GetType()) == DBNull.Value.GetTypeCode()) return$"{nameof(...

8

I'd say this is far too complicated. It took me some time to figure out what those expressions are getting rewritten to, and the results do not look very efficient: (x => new { x.Name.Length, x.Price }).Invoke(left).Length < (x => new { x.Name.Length, x.Price }).Invoke(right).Length || ... What are the benefits here compared to a simple Comparer&...

7

Don't one line things that shouldn't be one lined. Just because you can do something, doesn't mean that you should. Extract the duplicated query into a method of it's own that returns an IQueryable Don't query the database twice for the same count, execute the query and store it in a variable. private IQueryable<foo> ExecutorOrdersInLast30Days(...

7

You use IList<> where you should use ICollection<>. I've rarely encountered a scenario where IList<> actually needs to be used. The ICollection<> interface has most of the list's methods, but without everything related to indexing, which you don't use anyway. It's not that big of a deal, but I think it's good knowledge. When you ...

6

I just noticed a couple of small issues. First, IsValid(obj) may return true on null, whereas Validate(obj) has a special check for null. I would rewrite IsValid this way: public bool IsValid(T obj) { bool anyErrors = Validate(obj).Any(); return !anyErrors; } Second, your regex replacement might produce odd results in certain cases. Consider: e =&...

6

Note that your code will return null in case if the sort order is not defined. I've preserved this logic, but you may want to change it to return original query (and thus change return type to IQueryable<T_PO>). You are doing 2 separate actions in this method, so if you split it into 2 parts you can get more compact and readable code: public static ...

6

It looks alright to me, although it seems odd having a young man of age 20 to equal an "elderly" man of age 30 :-). I have 3 minor things: 1) One MemoryStream can be used to serialize all expressions: public Func<T, byte[]> Build(Func<byte[], byte[]> computeHash) { return obj => { var binaryFormatter = new BinaryFormatter()...

6

I don't like the concept that you have to chain the .With()-calls for every property you want to modify in the copy process because you create a new instance for each call. A simple non-generic solution could be to use the System.Runtime.InteropServices.OptionalAttribute and named parameters: public class Phone { public Phone() { } Phone(Phone ...

6

Very fun game you created here, I like it. Your calculations for the probabilities are also impressive, and well tested. Some small suggestions: UI improvement: Print a message when "leveling up". Answer.Wrong.also { } has a redundant .also that can be removed. Enum constants are usually written in uppercase, making it AND, OR, XOR, IMPLIES. The usage of ...

5

I wouldn't worry too much about the amount of things that aren't supported (yet/if ever) - it's impossible to cover everything in a scenario like this. One thing I would suggest is that you throw exceptions so the caller knows they're doing something unexpected: protected override Expression VisitMethodCall(MethodCallExpression m) { if (m.Method....

5

Not much to say here. Your code looks clean and is easy to read. There is just a little bit what I would change, namely the "default" rule of the Validator<T>. If you ever would have the need to validate that a passed T obj is null you couldn't do it with the Validator<T> in its current state. Maybe having a "default" rule as a property ...

5

Apart from the improvements suggested by Henrik Hansen and a couple of null checks I changed the list with tuples into SortedDictionary to avoid repetitive OrderBy and added a null check before calling getFingerprint for a property. This is the updated builder: public class FingerprintBuilder<T> { private readonly Func<byte[], byte[]> ...

5

I'd personally like DebuggerDisplayHelper.ToString() to be an extension method, so I finagled it up as such: public static class DebuggerDisplayHelper { public static string ToString<T>(this T obj, Action<DebuggerDisplayBuilder<T>> builderAction) { return DebuggerDisplayHelperInternal<T>.ToString(obj, builderAction); ...

5

I like the idea, but I'm in line with dfhwze meaning it's a little too verbose and complicated to follow, especially when unable to debug. I would prefer a more simple pattern like the one dfhwze suggests: var result = Tester // the person .Validate() .NotNull(p => p.LastName, "LastName is Null") .IsTrue(p => p.FirstName.Length > ...

4

Don't hang on to an IEnumerable public class Validator<T> { private readonly IEnumerable<ValidationRule<T>> _rules; public Validator(IEnumerable<ValidationRule<T>> rules) { _rules = rules; } ... } It's generally recommended to immediately materialize an enumerable if you're going to keep the ...

4

That looks nice. A few notes: Enums can have different underlying types, and some are larger than int, which can result in subtle bugs. Use propertyType.GetEnumUnderlyingType() instead. Some documentation in EqualityPropertyAttribute would be useful. For example, only public properties that are decorated with this attribute are taken into account, which ...

4

It's a shame that you have to use such a complicated mechanism because the language doesn't support strong enough type constraints. Given the limitations of the language, this looks like an elegant solution. I find the naming slightly curious. Given GeometricSequence, I expect the linear one to be called ArithmeticSequence. Alternatively, LinearSequence ...

4

As developer consuming your API .. Usability I find this a verbose way of constructing validation rules. var rules = ValidationRuleCollection .For<Person>() .Add(x => ValidationRule .Require .NotNull(x)) .Add(x => ValidationRule .Require ...

3

One other suggestion is that the expression you are building don't change based on the value you pass into the constructors (besides GeometricSequence which you still could make a parameter instead of a constant). You could build your expression in the static constructor since it only changes by type. Or make a static Lazy field that builds the ...

3

Since you are reading attributes I would put the work in the static constructor. That will run only once per type otherwise you should make the Create method lazy as there is no point in building the expressions multiple times. Also you can clean up your code quite a bit if you always just use IEqualityComparer for every property. You wouldn't need ...

3

Nice work! One thing you could do is leverage polymorphism for the Validation class so that you have a separate type for Valid and Invalid results. And then you can re-use the validation "loop" in the IsValid method, to make sure the two don't diverge (e.g. you don't have to have a separate null check in the IsMet method as well). Mind you, I am not sure ...

3

I might be missing something, but from what I can see IsSatisfiedBy is only ever used by IsSatisfiedBy, and can be removed. Consider making ToExpression protected, since with the implicit operator I don't see how exposing it is helpful. Consider exposing operators & and | to make composition of more complicated specifications easier to read and write. ...

3

Here are a few things I noticed: public PropertyComparer(params string[] properties) { var type = typeof(T); Properties = properties .Select(name => type.GetProperty(name)) .ToArray(); } There is no need for the lambda and it could be written as: Properties = properties .Select(type.GetProperty) .ToArray(); but for ...

3

This still looks fairly tedious: config.SetValue(() => x.PublicProperty); config.SetValue(() => x.PublicField); config.SetValue(() => x.PublicReadOnlyProperty); If you are going with reflection, I'd go all the way and implement automatic serialization/deserialization. //pseudocode for property deserialization var targetObject = ...; foreach(var ...

3

One of the things I think when I see a large case statement is would this be better off as some kind of lookup table. I think yours might have some scope for doing this since they all seem to do some processing on a viewSets and a filter. Using a simple lookup table to convert the string "Where", "Single" etc into a method call would allow you to separate ...

Only top voted, non community-wiki answers of a minimum length are eligible