37
\$\begingroup\$

I can't decide whether my method that returns an IEnumerable<T> should itself be lazy or whether it should build a list and return that. I often opt for the latter so I can be sure the enumeration of each value isn't performed multiple times. For example:

public IEnumerable<UserProfile> GetUsers()
{
    var allDepartments = GetAllDepartments(active: true); // Returns IEnumerable<Department>
    var allUsers = GetAllUserDepartments(active: true); // Returns IEnumerable<User>
    var users
        = allDepartments
            .Join(allUsers, x => x.Department, y => y.Department, (x, y) => y, StringComparer.OrdinalIgnoreCase)
            .Distinct()
            .Select(x => GetUserProfile(x))
            .ToList(); // Is this recommended or not

    return users;
}

The key part is each enumeration is doing something (GetUserProfile) non trivial, it might be expensive, it might not, but what's the recommendation for a method that returns an IEnumerable<T>? Should it care about whether the caller might enumerate it several times or not?

Assume only IEnumerable<T> functionality is required by the caller and my question can be reworded into:

How do I signify to the caller that each enumeration may be expensive?

How expensive may change their implementation decisions, if they enumerate the whole lot several times then a ToList() makes sense to them to perform, if they don't enumerate the whole lot, then a ToList() would be a waste for me (or the caller) to perform?

\$\endgroup\$
  • 10
    \$\begingroup\$ What would you expect if you look at the return type? Personally if I see IEnumerable I expect a lazy evaluated collection. If you call ToList() just return an IList or ICollection. Don't try to handle too much in a method. Just make it clear. \$\endgroup\$ – dusky Oct 15 '14 at 12:50
  • 2
    \$\begingroup\$ @dusky this makes sense, but remember that IList and ICollection are both mutable contracts. IEnumerable has the dual purpose of having lazy-evaluation semantics but also being a immutable structure to users. \$\endgroup\$ – Dan Pantry Oct 15 '14 at 16:59
  • 1
    \$\begingroup\$ @DanPantry .net 4.5 List implements IReadOnlyList and IReadOnlyCollection. But if you want a proper readonly collection return a proper immutable list. \$\endgroup\$ – dusky Oct 15 '14 at 21:04
  • 1
    \$\begingroup\$ @dusky Thanks, I think possibly IReadOnlyList is what I should return in my eager methods (that call ToList()). I try and avoid IList because ultimately I felt uncomfortable with the result being modified afterwards. IEnumerable for the lazy methods makes perfect sense. \$\endgroup\$ – user2000095-tim Oct 16 '14 at 14:16
  • \$\begingroup\$ depends on the what the criteria for "good" are. See also immutable list msdn.microsoft.com/en-us/library/dn467185(v=vs.111).aspx \$\endgroup\$ – phil soady Oct 16 '14 at 14:19
24
\$\begingroup\$

Does code that calls the method always expect List functionality (access by index, etc.)? Return a List. Does code that calls the method only expect to iterate over it? Return an IEnumerable.

You shouldn't care about what the caller does with it, because the return type clearly states what the returned value is capable of doing. Any caller that gets an IEnumerable result knows that if they want indexed access of the result, they will have to convert to a List, because IEnumerable simple isn't capable of it until it's been enumerated and put into an indexed structure. Don't assume that the callers are stupid, otherwise you end up taking functionality away from them. For example, by returning a List, you've taken away the ability to stream results which can have its own performance benefits. Your implementation may change, but the caller can always turn an IEnumerable into a List if they need to.

Now that you've decided what the return type should be, you have to decide what the implementation should use. Deferred execution has benefits, but can also be confusing. Take this example:

public static IEnumerable<int> GetRecordIds()
{
    return dbContext.Records.Select(r => r.Id);
}

IEnumerable<int> currentRecordIds = GetRecordIds();

dbContext.Add(new Record { Id = 7 });

// Includes "7", despite GetRecordIds() being called before the Add():
currentRecordIds.Dump();

This can be "remedied" by GetRecordIds calling ToList before returning. The "correct" use here simply depends on what is expected of the class ("live" results, or time of call results).

Again, don't assume the caller is stupid, and don't take away functionality from the caller by making assumptions about how it will be used. Remember, the interface tells you what is expected to be returned. IEnumerable only means you are getting something that can be iterated over (potentially streaming results and making use of deferred execution), and List only means you're getting an in-memory collection that can be added to, removed from, accessed by index, etc.

Edit - To address your "Update 1":

If you are really concerned, you might add some information into the documentation, but I don't think it's necessary. The IEnumerable interface doesn't claim to make any promises about performance on repeat enumeration (or even each iteration for that matter), so it's on the caller to handle it intelligently.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your point of view is very interesting. I'm just bothered by the defered execution of the enumerable.. But still, it's a great answer. \$\endgroup\$ – IEatBagels Oct 15 '14 at 16:03
  • \$\begingroup\$ Thanks for your answer, but I'm a bit confused, you're asking what the code that calls it does then saying I shouldn't care about what the caller does. \$\endgroup\$ – user2000095-tim Oct 16 '14 at 14:38
  • \$\begingroup\$ The first lines mean to say: does the API you're trying to expose to callers wish to imply that the results are useful as an indexed set that can be modified or not? \$\endgroup\$ – Ocelot20 Oct 17 '14 at 14:44
  • 1
    \$\begingroup\$ I am also not saying "don't care about what the caller does". I am saying "don't assume you know better than the caller by forcing them to use List (unless the intended use of the method is always need indexed access, etc.). \$\endgroup\$ – Ocelot20 Oct 17 '14 at 15:07
  • \$\begingroup\$ @Ocelot20 OK, I think my Update 1 clarifies what I'm trying to achieve. Does it make more sense? \$\endgroup\$ – user2000095-tim Oct 17 '14 at 16:37
14
\$\begingroup\$

I think the real question here is can the return value be lazily evaluated? This is essentially what this comes down to.

IEnumerable is lazily evaluated in that any operations you apply to it (through Linq, or any sensible author) will be deferred until the last possible moment i.e, until you try and observe the IEnumerable by iterating or by using a fold (First(), FirstOrDefault()). This is a good thing in most cases because it means you can completely avoid doing any work until you need to - which sometimes means that a potentially expensive operation never actually occurs. Awesome!

However, this doesn't work so well when it comes to sequences that are retrieved from some unmanaged resource. Your example of this is perfect - a database. An IEnumerable backed by a database should not be lazily evaluated, as the connection it was bound to can potentially be closed by the time it is enumerated.

The exception to this is if you can find some way of binding the lifecycle of a connection to an IEnumerable - in other words, can you teach the IEnumerable how to instantiate a connection/other resource, use it, and then dispose it. In some applications this solution might not be viable due to the performance impact, and you might feel uneasy about the loss of control here (this is where IoC comes in handy).

There is a framework called Rx (Reactive Extensions) for .NET that actually has a feature in this built in called Observable.Using(). An observable is a class that can be observed, and is lazily evaluated - much like IEnumerable. In fact, IObservable implements IEnumerable. By using Observable.Using(), you can bind the creation of an object through a factory method to the lifetime of an IObservable. When the IObservable is first observed, the resource is created. When the IObservable is finished being observed, the resource is Dispose()d. Neat and tidy.

Back to the crux of your question, though; in this case, I would say, yes, ToList() the IEnumerable before returning the control to the caller; you will run into concurrency issues otherwise, and you have no guarantee that the connection will still be alive when you return control to the caller. But make sure to return an IEnumerable still - IEnumerable has a dual purpose in that it is very good at representing a read-only collection.

\$\endgroup\$
  • \$\begingroup\$ "you will run into concurrency issues" is a bit harsh. There are plenty of perfectly acceptable reasons to return a lazily evaluated IEnumerable from a db. It all comes down to what that individual class claims to be responsible for. Also, "you have no guarantee the connection will be alive" is a concern of the class returning the IEnumerable, not the caller. \$\endgroup\$ – Ocelot20 Oct 15 '14 at 17:21
  • \$\begingroup\$ I am assuming OP is using EntityFramework. If he is, then he will run into concurrency issues if he does not ToList() the enumerable, and I am only saying that out of pain born from doing exactly the opposite. I did point out though that if you can bind the lifetime of the resource to the IEnumerable that this is circumvented. \$\endgroup\$ – Dan Pantry Oct 15 '14 at 17:22
  • \$\begingroup\$ To be clear, it was not meant to be harsh. But I would say that having a lazily evaluated structure coming straight from the DB layer and leaving the DB layer is likely to be a bad thing if it talks directly to the database connection. As I mentioned, certainly in Ef, you will run into a lot of problems by doing this. \$\endgroup\$ – Dan Pantry Oct 15 '14 at 17:24
  • \$\begingroup\$ "harsh" was a poor choice of words on my part. I guess I meant "overly generalized statement". Whether or not he'll run into concurrency issues highly depends on what he's exposing/doing with the results. Like I mentioned before, there are plenty of acceptable reasons to return a lazily evaluated IEnumerable from a db, but without seeing more code we can't really say what problems he'll run into. It's certainly a potential problem worth mention though. \$\endgroup\$ – Ocelot20 Oct 15 '14 at 17:47
  • \$\begingroup\$ @DanPantry thanks for your answer, there is no DB, the Users and Departments are cached and the UserProfile may reside in cache, may reside in DB, may reside in table storage or disk or somewhere else. I've not used IObservable<T>, does it complicate the code compared with using the LINQ extension methods against an IEnumerable<T>. It does sound like one to look into. Do you have any ideas how it could be implemented? \$\endgroup\$ – user2000095-tim Oct 16 '14 at 14:51
5
\$\begingroup\$

You shouldn't use var if it is to put a comment stating the return type, if it isn't clear without the comment, put the real type instead of var, it is more readable this way.

To answer you question, I think that yes you should call ToList before returning the enumeration mainly because your IEnumerable<UserProfile> could be an EF query and there's no way for the user of your class to know this, meaning the person could use this enumeration multiple times and launch tons of SQL queries without knowing it (until there would be performance problems and diagnostic would lead to that IEnumerable launching queries).

The performance cost of using ToList is the same as using new List<>([your enumerable]) constructor, which is an O(n) operation since you need to copy the whole array. If you have a very big enumerable that you don't want to copy all the time, maybe you should consider adding a parameter to your method that would let you decide if you execute the IEnumerable or not this way :

public IEnumerable<UserProfile> GetUsers(bool deferExecution)
{
    IEnumerable<Department> allDepartments = GetAllDepartments(active: true);
    IEnumerable<User> allUsers = GetAllUserDepartments(active: true);
    IEnumerable<UserProfile> users
        = allDepartments
            .Join(allUsers, x => x.Department, y => y.Department, (x, y) => y, StringComparer.OrdinalIgnoreCase)
            .Distinct()
            .Select(x => GetUserProfile(x));

    if(!deferExecution) 
    {
        users = users.ToList();
    }

    return users;
}

Though honestly, I've never tried it with the boolean in real life so I can't tell if it is a really good idea, but I feel it wouldn't be bad!

If you want more info about the performance cost of ToList, check this post.

\$\endgroup\$
  • 1
    \$\begingroup\$ Interesting idea with the deferExecution argument. \$\endgroup\$ – xDaevax Oct 15 '14 at 12:53
  • 1
    \$\begingroup\$ That is not how IEnumerable works. Returning an IQueryable as an IEnumerable or List will be no different WRT how many times the query is executed. The number of times the method is called, as well as whether or not the consumer iterates over the result would be the determining factor. \$\endgroup\$ – Ocelot20 Oct 15 '14 at 14:03
  • \$\begingroup\$ What does WRT means? \$\endgroup\$ – IEatBagels Oct 15 '14 at 15:59
  • \$\begingroup\$ "With Regards To" I assume \$\endgroup\$ – Dan Pantry Oct 15 '14 at 16:58
  • \$\begingroup\$ Yes, I did mean "with regards to". \$\endgroup\$ – Ocelot20 Oct 15 '14 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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