Would you abstract your LINQ queries into extension methods

Question

On my current project we set ourselves some goals for the code metrics "Maintainability Index" and "Cyclometic Complexity". Maintainability Index should be 60 or higher and Cyclometic Complexity 25 or less. We know that the Maintainability Index of 60 and higher is a pretty high one.

We also use a lot of linq to filter/group/select entities. I found out that these linq queries aren't scoring that high on Maintainability Index. Abstracting this queries into extension methods is giving me a higher Maintainability Index, which is good. But in most of the cases the extension methods are not generic anymore because I use them with my Types instead of generic types.

For example the following linq-query vs extension method:

Linq query

List.Where(m => m.BeginTime >= selectionFrom && m.EndTime <= selectionTo)

Extension method:

public static IEnumerable<MyType> FilterBy(this IEnumerable<MyType> source, DateTime selectionFrom, DateTime selectionTo)
{
    return (IEnumerable<MyType>)source.Where(m => m.BeginTime >= selectionFrom && m.EndTime <= selectionTo);
}

List.FilterBy(selectionFrom, selectionTo);

The extension method gives me a Maintainability Index improvement of 6 points, and gives a nice fluent syntax. On the other hand I have to add a static class, it's not generic.

Any ideas on what approach would have your favor? Or maybe have different ideas about how to refactor the linq queries to improve Maintainability Index?

Answer 1

You shouldn't add classes for the sake of metrics. Any metrics are meant to make your code better but following rules blindly, even the best rules, may in fact harm your code.

I don't think it's a good idea to stick to certain Maintainability and Complexity indexes. I believe they are useful for evaluating old code, i.e. when you inherited a project and need to estimate its complexity. However, it's absurd to extract a method because you haven't scored enough points.

Only refactor if such refactoring adds value to the code. Such value is a complex human metric inexpressible in numbers, and estimating it is exactly what programming experience is about—finding balance between optimization vs readability vs clean API vs cool code vs simple code vs fast shipping vs generalization vs specification, etc.

This is the only metric you should follow but it's not always the metric everyone agrees upon...

As for your example, if the same LINQ query is used over and over, it makes perfect sense to create an EnumerableExtensions in Extensions folder and extract it there. However, if it used once or twice, or is subject to change, verbose query is so much better.

I also don't understand why you say they are not generic with somewhat negative connotations. You don't need generics everywhere! In fact, when writing extension methods, you should consider the most specific types you can choose as to not pollute other classes' method set. If you want your helper to only work with IEnumerable<MyType>, there is absolutely no shame in declaring an extension method exactly for this IEnumerable<MyType>. By the way, there's redundant casting in your example. Get rid of it.

And don't forget, tools are stupid. So are we, humans.

Answer 2

My advice to you would be ... don't be a slave to your metrics! They are machine generated and only intended to be used as guidance. They are never going to be a replacement for a skilled experienced programmer.

Which do you think is right for your application?

Answer 3

I for one agree with the extension method strategy. I've used it without a problem in a handful of real-world apps.

To me, it is not only about the metrics, but also the re-usability of the code there. See the following psuedo-examples:

var x = _repository.Customers().WhichAreGoldCustomers();

var y = _repository.Customers().WhichAreBehindInPayments();

Having those two extension methods accomplishes your goal for metrics, and it also provides "one place for the definition of what it is to be a gold customer." You don't have different queries being created in different places by different developers when they need to work with "gold customers."

Additionally, they are composable:

var z = _repository.Customers().WhichAreGoldCustomers().WhichAreBehindInPayments();

IMHO this is a winning approach.

The only problem we've faced is that there is a ReSharper bug that sometimes the Intellisense for the extension methods goes crazy. You type ".Whic" and it lets you pick the extension method you want, but when you "tab" on it, it puts something completely different into the code, not the extension method that you selected. I've considered switching from ReSharper for this, but... nah :)

Answer 4

NO: in this case I would ignore the cyclomatic complexity - what you had originally was better.

Ask yourself what is more explanatory. This:

List.Where(m => m.BeginTime >= selectionFrom && m.EndTime <= selectionTo)

or this:

List.FilterBy(selectionFrom, selectionTo);

The first clearly expresses what you want, whereas the second does not. The only way to know what "FilterBy" means is to go into the source code and look at its implementation.

Abstracting query fragments into extension methods makes sense with more complex scenarios, where it's not easy to judge at a glance what the query fragment is doing.

Answer 5

I have used this technique in places, for example a class Payment has a corresponding class PaymentLinqExtensions which provides domain specific extensions for Payments.

In the example you give I'd choose a more descriptive method name. There is also the question of whether the range is inclusive or exclusive, Otherwise it looks OK.

If you have multiple objects in your system for which the concept of having a date is common then consider an interface, maybe IHaveADate (or something better :-)

public static IQueryable<T> WithinDateRange(this IQueryable<T> source, DateTime from, DateTime to) where T:IHaveADate

(IQueryable is interesting. I don't think IEnumerable can cast to it which is a shame. If you're working with database queries then it can allow your logic to appear in the final SQL that is sent to the server which is good. There is the potential gotcha with all LINQ that your code is not executed when you expect it to be)

If date ranges are an important concept in your application, and you need to be consistent about whether the range starts at midnight on the end of "EndDate" or midnight at the start of it, then a DateRange class may be useful. Then

public static IQueryable<T> WithinDateRange(this IQueryable<T> source, DateRange range) where T:IHaveADate

You could also, if you feel like it, provide

public static IEnumerable<T> WithinDateRange(this IEnumerable<T> source, DateRange range, Func<DateTime,T> getDate)

but this to me feels more something to do with DateRange. I don't know how much it would be used, though your situation may vary. I've found that getting too generic can make things hard to understand, and LINQ can be hard to debug.

var filtered = myThingCollection.WithinDateRange(myDateRange, x => x.Date)