Best Practices for Linq Enumerables and Queryables | Hacker News Reader

Best Practices for Linq Enumerables and Queryables

code.jonwagner.com

27 pointsjonwagner27 comments

It is a terrible article.

1. Why would the author recommend using extension method instead of using the 'query' syntax? The main purpose behind creating the query syntax was to make your code easier to read. Query syntax is not at all about 'pretty' or 'clever'. It's about readability. [And why call the query syntax as 'language extension'?]

2. Why invent terms like 'seal' LINQ queries? By default, all LINQ querie execution is deferred. And yes, you have to understand this deferred execution and how it affects your code. But his code example is TERRIBLE.

var allCustomers = customers; var waCustomers = allCustomers.Where (c => c.Region == "WA"); var waCustomerIDs = waCustomers.Select (c => c.ID);

Why copy the variable to allCustomers? And if you need Id and name, you can write code like this:

var customerIdAndNames = from c in customers where c.Region.Equals("WA", StringComparison.OrdinalIgnoreCase) select new { ID = c.ID, Name = c.Name };

There was absolutely no reason to create two different IEnumerables for a scenario where you needed different properties.

3. Again, one has to understand that result of a query is just that, a 'query'. And you don't have to call 'ToList' method to execute the query. In fact, if you want to iterate the query results only once, it's better to use foreach and enumerate over the query results instead of calling ToList method as you will be not have to consume memory to store the entire list.

4. Horrible idea to suggest that you should always return List instead of IEnumerable. This choice should be left up to API caller in most cases.

This whole article is garbage. It is OPPOSITE of best practices in LINQ.

About your first point, sometimes using method syntax is more readable than query. When you are not reliying heavily on LINQ and just use some commands, is easier to write

    var males = customers.Where(c => c.Gender == "male");

than

    var males = from c in customers where c == "male" select c;

Not only because it's longer, but also because it can feel strange if you're not using it continously.

Readability is definitely subjective. For simple scenarios, I do prefer the extension method.

E.g. in my code, it would actually be like

   var males = customers.Where(c => c.IsMale);

   var males = from c in customers where c.IsMale select c;

But often, queries are not that simple and in such scenario query syntax offers far more readability:

e.g.

   var filteredCustomers = 
                        from c in customers

                        join o in orders on o.customerid = c.customerid

                        where c.IsMale && c.Age > 30

                        where o.IsPending

                        select new {Customer = c, Order = o};

The corresponding extension method syntax will not be readable. In fact, I don't even know how to write that code off top of my head.

My point was that the whole purpose of query syntax was to improve readability and it was not about 'pretty' or 'clever'.

The corresponding extension method syntax is:

  var filteredCustomers = customers
      .Join(orders,
          c => c.customerid,
          o => o.customerid,
          (c, o) => new { Customer = c, Order = o }
      )
      .Where(x => x.Customer.IsMale && x.Customer.Age > 30)
      .Where(x => x.Order.IsPending);

I've used two from's for inner joins:

    var filteredCustomers =
       from c in customers
       from o in orders
       where o.customerid = c.customerid &&
             c.IsMale && c.Age > 30 &&
             o.IsPending
       select new {Customer = c, Order = o};

This is one of those cases where I'll favor query syntax over extensions. OTOH, query syntax requires an explicit `select` where it's often optional in extensions. Also, `.ToList()` doesn't have a query syntax counterpart so you'll often mix both if you tend to favor query.

Assuming you have a foreign key set up between customers and orders, your 'order' entity will automatically have a 'customer' property so you can avoid the 'join' syntax.

If your objective is to find orders that belong to male customers over 30:

filteredCustomers = orders.Where(o => o.IsPending && o.customer.IsMale && o.customer.Age > 30);

Some more about avoiding joins (which makes it easier to use the extension syntax): http://blogs.teamb.com/craigstuntz/2010/01/13/38525/

I agree--I'm not sure how he was lead to believe this is best practice. If you mean to process the results of the query with a loop, its silly to enumerate over the query to create a List then enumerate over it again to modify those objects, which sounds like what he's suggesting is best practice.

I think perhaps he is confusing best practices for public interfaces with general-purpose best practices.

It is a good idea to prefer ToList()ing any data you're passing out of a library. An 'open' LINQ query might represent a whole lot of work, and that work will get repeated every time someone re-enumerates the query. And the query might be holding on to any number of resources that the end-user can't know about. Returning a data structure instead of an unexecuted query makes it much easier for people who are working with your library to know what they're working with, because what they're working with is simply the contents of the data structure. To that end, it's preferable according to the "pit of success" principle.

But that flip-flops when you're only dealing with the inside an assembly. None of the concerns listed above really apply in that case, so it's generally preferable to avoid petrifying your LINQ expressions unless you absolutely have to.

> I think perhaps he is confusing best practices for public interfaces with general-purpose best practices.

Exactly. Most of the advice in the article is sensible when viewed from that perspective.

I use LINQ to Entity in my company's service API. If you fail to "seal" the query (as he calls it) with ToList() before you leave the 'using' block for your DB context, your callers get a runtime error later since the IQueryable isn't run until the caller enumerates it (i.e. after the 'using' block has disposed the DB context). (On the other hand, if you call ToList() too early in your method chain, you're preventing L2E from composing your expression tree into optimal SQL, since everything after ToList() is run client-side.)

As an API provider you have to treat the behavior of your return values as part of the contract, and if your caller is expecting a plain IEnumerable (like you claim to return in your method signature), you'd best make sure it isn't really an IEnumerableThatDependsOnNondeterministicContext or an IEnumerableWithUnpredictableSideEffects.

His point is spot on about returning IQueryable only when your intention is for the caller to compose your result with other queries. If you're just returning an IQueryable because you think it's cool to defer execution, you're probably missing the point. Deferred execution isn't 100% win; you can just as easily defer yourself into a timeslot when there's more contention for a resource as into one where there's less contention. In many cases it's just as well to declare your need for the data (e.g. by calling ToList() as early as possible and let the system manage the execution.

That reminds me of another one I ran into. It's not specifically LINQ related, but does play into the problem with returning generators. A homegrown data access layer that would grab a SqlDataReader and then yield each item.

That ended up creating all sorts of problems, up to and including a serious deadlocking issue at the database end of things.

But if you're ToListing it, your return type might as well just be List, not IEnumerable (or IQueryable).

I feel that by declaring your return type as IEnumerable, you're implicitly saying to any caller that the return object is something that can iterate (and potentially generate) through results when requested, and so care should be taken with its use (to avoid getting multiple IEnumerator objects, and iterating unnecessarily).

As I've said elsewhere, this functionality should be embraced.

One of the ways the caller might prevent iterating unnecessarily may be to call ToList or ToArray. Alternatively, they might structure their calling code better. Either way, it should be the caller's choice, instead of being imposed by the underlying method.

But if you're ToListing it, your return type might as well just be List, not IEnumerable

Perhaps, it really depends. One nice advantage that returning IEnumerable<T> has over returning List<T> is that it gives better flexibility and maintainability.

If you return List<T>, you're tying yourself to that specific class now and forever. Any change will be a breaking change.

If you return IEnumerable<T>, all you're guaranteeing is that you'll return something that the caller can enumerate over to get their data. Meaning if you later discover that you have some compelling reason to switch to using a HashSet<T> internally, and that it would also be most convenient if you could just pass back that HashSet<T>, well, there's nothing to stop you.

You don't get that flexibility by typing your return value as List<T> because you've tied yourself to that specific class. You also don't get that flexibility by passing back IList<T>. IList<T> defines an ordered, positionally-indexed collection, and hashes are not that. ICollection<T> might work, but it defines an interface for a mutable collection, which might also be a restriction you don't want to commit to now and forever.

So in general it's best to pass back the most flexible type you can. Partially because YAGNI, but mostly because trying to create a pit of success for your users doesn't mean you can't also try to create a pit of success for yourself as well.

(Forgot to mention - the semantics that you're claiming for IEnumerable doesn't really line up with how it's actually used. IEnumerable has been around since .NET 1.1, and IEnumerable<T> has been around since .NET 2.0. There were years and years where IEnumerable simply defined an object that could be enumerated before LINQ came on the scene and introduced us to ubiquitous examples of lazily-generated IEnumerables, or introduced all these useful extension methods that take IEnumerable<T> and return a lazily-generated IEnumerable<T>.)

Definitely. Should have left that first line out, as it wasn't what I was trying to argue. The rest of my point still stands.

Edit: I see you've appended to your comment. The problem is you could always have lazily-evaluated IEnumerables by implementing an IEnumerator. It was just a pain in the arse until C#2 brought us generator support through the yield keyword. This was long before Linq came along.

Edit2:

  > There were years and years where IEnumerable simply defined an object that could be enumerated...

Which is my point exactly. And nothing has changed with IEnumerable (generics excluded). It certainly doesn't say that all the objects are already held in-memory (as enforcing ToList would do). By returning an IEnumerable, you're just saying here's an object that can produce you a sequence of results. In a public API, it should be documented (at least some vague allusion to) whether this will be produced by trivially pulling them out of an in-memory list, or whether something a bit more clever is going on, as there'll certainly be occasions where streaming the results through a generator is more desirable than holding them all in memory.

Ah, I hadn't been in that situation or thought about that. Makes perfect sense though.

Absolutely. You saved me writing the same thing.

Regarding point 1, I would add there are also things that can be done using the query syntax that simply can't with the extension methods. For instance, I'm not sure how you would scope things in the same way as the 'let' clause when using extension methods.

Regarding the other points, I would only add that deferred execution and lazy evaluation are things to be embraced, not hidden away and overriden. Should we also not bother with the yield keyword?

Of course, there's always some occasion where you'll want to ToList your IEnumerable. I'd argue this should be of concern to the consumer of the IEnumerable though, not the producer.

Point 5 at least tries to allude to something useful. Given Linq's grounding in functional programming, of course you want to try to avoid side effects.

Regarding the let keyword; the following code:

  var x = from post in posts
          let keywords = post.split(' ')
          ...

Is compiled* into:

  var x = posts
          .Select(post => new { keywords = post.split(' '), post })
          ...

* If you didn't know already, the compiler transforms query syntax into extension method syntax.

Oh yes, I did actually realise that. Thank you though.

I worded it badly. I should have been clearer in that I was following on from solutionyogi's argument about readability.

The compiler example is a bit on the ugly side, wouldn't you say? To then access 'keywords', it becomes

  ...
  .Where(anon => anon.keywords[0] == "verybadexample")
  .Select(anon => anon.post);

What I should have said was that I'm not sure how you would scope things in the same way as the 'let' clause when using extension methods with the same level of readability.

Like this:

  var thing = 
    from x in stuff
    let derp = x.herp
    select { x.name }

Equals this:

  var thing = stuff.Select( x => {
    var derp = x.herp;

    return new { x.name };
  } );

edit: formatting.

I think each have their place, but this absolutely enrages me:

  var things = ( from x in thingList select x ).ToList()

Regarding point 1, I would add there are also things that can be done using the query syntax that simply can't with the extension methods.

Quite the opposite. Everything in the query syntax gets translated into method calls, even if the way in which it is done so is sometimes non-obvious.[1] However, there are a lot of extension methods that don't have an equivalent in the query syntax - many of the aggregation functions, for example.

[1]:http://stackoverflow.com/questions/1092687/code-equivalent-t...

Yes, Eric Lippert, principal developer of C# compiler, has been drumming for a long time that query should never have a side effect. It should be about filtering and sorting the data. See: http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/forea...

I prefer using extension methods over query syntax; I personally find extension methods easier to read. I guess it's a matter of personal preference.

Uhg. Always defer materializing the query until you need it. If you are using LINQ against something like EF it changes how the SQL is generated.

Consider var query = (from customer in context.Customers select customer).ToList().Where( c => c.Id == 3);

vs var query = (from customer in context.Customers where customer.Id == 3 select customer);

The first LINQ query results in "SELECT * FROM customer". The WHERE clause is applied after the result set is returned. The second generates a real WHERE clause. The result set is filter on the server before being sent to the client.

Thanks for the feedback. I've made some updates to the article to hopefully improve where my meaning was not as clear as I intended, and where I should have more strongly written "it depends on your situation".

My next controversial post will be titled "Proper Spacing Around Parentheses in C#" ;)

"Sealing the chain" as you say, is indeed important for scenarios such as lookups within a loop. If you haven't .ToList()ed your lookup LINQ var, you will be requerying with every iteration. A simple change that can slow down code dramatically.

The article does a good job of exposing common pitfalls to those new to deferred execution and offers some solid advice. However, I agree that it's overly defensive to recommend ToList'ing almost every IEnumerable to avoid having to think about it, but this seems like a decent idea for more "software conservative" developers.

These are "Best Practices for Linq Enumerables and Queryables when You're Not Really Sure How Linq Works but Still Want to Use It".

Good to see some C# on HN

Reply on news.ycombinator.com