views:

416

answers:

8

Ok, I'm hoping the community at large will aid us in solving a workplace debate that has been ongoing for a while. This has to do with defining interfaces that either accept or return lists of some type. There are several ways of doing this:

public interface Foo
{
 Bar[] Bars { get; }
 IEnumerable<Bar> Bars { get; }
 ICollection<Bar> Bars { get; }
 IList<Bar> Bars { get; }
}

My own preference is to use IEnumerable for arguments and arrays for return values:

public interface Foo
{
 void Do(IEnumerable<Bar> bars);
 Bar[] Bars { get; }
}

My argument for this approach is that the implementation class can create a List directly from the IEnumerable and simply return it with List.ToArray(). However some believe that IList should be returned instead of an array. The problem I have here is that now your required again to copy it with a ReadOnlyCollection before returning. The option of returning IEnumerable seems troublesome for client code?

What do you use/prefer? (especially with regards to libraries that will be used by other developers outside your organization)

+14  A: 

My preference is IEnumerable<T>. Any other of the suggested interfaces gives the appearance of allowing the consumer to modify the underlying collection. This is almost certainly not what you want to do as it's allowing consumers to silently modify an internal collection.

Another good one IMHO, is ReadOnlyCollection<T>. It allows for all of the fun .Count and Indexer properties and unambiguously says to the consumer "you cannot modify my data".

JaredPar
+1 in any application I've worked on, at least, `IEnumerable` is all you need 98% of the time.
Rex M
Not saying you're wrong, but doesn't that assume you're not retuning a readonly list or a shallow copy (or otherwise making immutable)?
annakata
@annakata, but how to express this to your consumers? You cannot simply look at a return of say IList and know: 1) should I not modify this, 2) is modification disabled by throwing (Dictionary<TKey,TValue>::Values) or 3) being returned a complete copy. It's simply not obvious from the use. Returning IEnumerable<T> on the other hand fairly unambiguously says "you're not touching my data".
JaredPar
Don't you still copy the List<T> implementation member to an array first? If you do not you risk them enumerating after modification to the original. This doesn't mention the fact that they can cast it List<T> and modify the private instance.
csharptest.net
@Jared: You put a `Contract.Ensures(Contract.Result<IList<T>>().IsReadOnly)` in the contract classs ;)
Jon Skeet
@Jon good idea. my biggest issue with contracts is their visiblity within the IDE (they should be highlighted better). Then again, I work on an IDE team so ...
JaredPar
@JaredPar: Indeed. Of course, if we could get static checking in Visual Studio non-Team-System edition, that would help too... :) Just to keep this relevant: my objection to `ReadOnlyCollection<T>` is that it pins it down to a very particular collection type. I have `PopsicleList` as another read-only collection (one which is mutable but can then be "frozen" before you give it out; a bit like string). It would be nice to have some interface these types could implement. (Admittedly PopsicleList is only immutable after freezing, complicating things...)
Jon Skeet
@Joh, Kewl I got to learn something today... I was totally unaware of code contracts in 3.5. I guess I am getting left behind in my 2.0 world ;)
csharptest.net
+1 This is good advice.
Andrew Hare
@csharptest.net I think Code contracts is the new in 4.0
eglasius
Perhaps is IEnumerable for data that's by nature enumerated in a lazy way ... and ReadOnlyCollection for a list that's pre-loaded, see my answer on it.
eglasius
+12  A: 

I don't return arrays - they really are a terrible return type to use when creating an API - if you truly need a mutable sequence use the IList<T> or ICollection<T> interface or return a concrete Collection<T> instead.

Also I would suggest that you read Arrays considered somewhat harmful by Eric Lippert:

I got a moral question from an author of programming language textbooks the other day requesting my opinions on whether or not beginner programmers should be taught how to use arrays.

Rather than actually answer that question, I gave him a long list of my opinions about arrays, how I use arrays, how we expect arrays to be used in the future, and so on. This gets a bit long, but like Pascal, I didn't have time to make it shorter.

Let me start by saying when you definitely should not use arrays, and then wax more philosophical about the future of modern programming and the role of the array in the coming world.

Andrew Hare
Hadn't read that lippert link before - +1 for that alone
annakata
Ditto -- good read.
csharptest.net
Thanks for the shout-out. :)
Eric Lippert
+5  A: 

For property collections that are indexed (and the indices have necessary semantic meaning), you should use ReadOnlyCollection<T> (read only) or IList<T> (read/write). It's the most flexible and expressive. For non-indexed collections, use IEnumerable<T> (read only) or ICollection<T> (read/write).

Method parameters should use IEnumerable<T> unless they 1) need to add/remove items to the collection (ICollection<T>) or 2) require indexes for necesary semantic purposes (IList<T>). If the method can benefit from indexing availability (such as a sorting routine), it can always use as IList<T> or .ToList() when that fails in the implementation.

280Z28
+1 for using what's appropriate for the scenario ... its really not 1 to rule then all ... another way to say ReadOnlyCollection vs. IEnumerable is pre-loaded vs. lazy loaded.
eglasius
+1  A: 

I would prefer IEnumerable as it is the most highlevel of the interfaces giving the end user the opportunity to re-cast as he wishes. Even though this may provide the user with minimum functionality to begin with (basically only enumeration) it would still be enough to cover virtually any need, especially with the extension methods, ToArray(), ToList() etc.

Robban
A: 

I think about this in terms of writing the most useful code possible: code that can do more.

Put in those terms, it means I like to accept the weakest interface possible as method arguments, because that makes my code useful from more places. In this case, that's an IEnumerable<T>. Have an array? You can call my method. Have a List? You can call my method. Have an iterator block? You get the idea.

It also means I like my methods to return the strongest interface that is convenient, so that code that relies on the method can easily do more. In this case, that would be IList<T>. Note that this doesn't mean I will construct a list just so I can return it. It just means that if I already have some that implements IList<T>, I may as well use it.

Note that I'm a little unconventional with regards to return types. A more typical approach is to also return weaker types from methods to avoid locking yourself into a specific implementation.

Joel Coehoorn
... if I already have some that implements IList<T>, I may as well use it ... Yes I see your point; however, this can cause strange side-effects. One should at minimal wrap it in a ReadOnlyCollection<T>().
csharptest.net
@csharptest.net if you argue that a IList<T> should be packed in ReadOnly Collection why would you then return an Array your self which is far from readonly after all any element can be substituted with any thing assignable to the given type
Rune FS
You are correct here; my misguided belief was that it would be obvious you were getting a copy and there would be no point to modifying it. I admit I was clearly wrong there by this discussion... It never occurred to me that people would do "Foo.Bars[0] = x" or some such nonsense. Anyway thanks for helping enlighten me +1 :)
csharptest.net
A: 

IEnumerable<T> is very useful for lazy-evaluated iteration, especially in scenarios that use method chaining.

But as a return type for a typical data access tier, a Count property is often useful, and I would prefer to return an ICollection<T> with a Count property or possibly IList<T> if I think typical consumers will want to use an indexer.

This is also an indication to the caller that the collection has actually been materialized. And thus the caller can iterate through the returned collection without getting exceptions from the data access tier. This can be important. For example, a service may generate a stream (e.g. SOAP) from the returned collection. It can be awkward if an exception is thrown from the data access layer while generating the stream due to lazy-evaluated iteration, as the output stream is already partially written when the exception is thrown.

Joe
A: 

Since the Linq extension methods were added to IEnumerable<T>, I've found that my use of the other interfaces has declined considerably; probably around 80%. I used to use List<T> religiously as it had methods that accepted delegates for lazy evaluation like Find, FindAll, ForEach and the like. Since that's available through System.Linq's extensions, I've replaced all those references with IEnumerable<T> references.

Travis Heseman
A: 

I wouldn't go with array, its a type that allows modification yet doesn't have add/remove... kind of like the worst of the pack. If I want to allow modifications, then I would use a type that supports add/remove.

When you want to prevent modifications, you are already wrapping it/copying it, so I don't see what's wrong with a an IEnumerable or a ReadOnlyCollection. I would go with the later ... something I don't like about IEnumerable is that its lazy by nature, yet when you are using with pre-loaded data only to wrap it, calling code that works with it tends to assume pre-loaded data or have extra "unnecessary" lines :( ... that can get ugly results during change.

eglasius