5 ms·

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

by aggronn 14y ago

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.

bunderbunder 14y ago
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.
- aggronn 14y ago
  Ah, I hadn't been in that situation or thought about that. Makes perfect sense though.
- upthedale 14y ago
  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.
  - bunderbunder 14y ago
    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>.)
    - upthedale 14y ago
      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.
- davidp 14y ago
  > 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.
  - bunderbunder 14y ago
    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.

by aggronn 14y ago

bunderbunder 14y ago

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.

aggronn 14y ago
Ah, I hadn't been in that situation or thought about that. Makes perfect sense though.
upthedale 14y ago
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.
- bunderbunder 14y ago
  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>.)
  - upthedale 14y ago
    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.
davidp 14y ago
> 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.
- bunderbunder 14y ago
  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.