12

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)

csharptest.net
  • 62,602
  • 11
  • 71
  • 89
  • 1
    I've seen the errors of my ways. As many state below it's obvious developers do not see T[] properties as immutable. I guess my abuse of them has taught me some bad habits. I never really cared if they modified the array since it was a copy; however, I've come to realize the confusion it produces. IEnumerable is a hands-down winner I think. – csharptest.net Sep 21 '09 at 21:00

8 Answers8

18

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
  • 733,204
  • 149
  • 1,241
  • 1,454
  • 3
    +1 in any application I've worked on, at least, `IEnumerable` is all you need 98% of the time. – Rex M Sep 21 '09 at 18:47
  • 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 Sep 21 '09 at 18:48
  • 1
    @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::Values) or 3) being returned a complete copy. It's simply not obvious from the use. Returning IEnumerable on the other hand fairly unambiguously says "you're not touching my data". – JaredPar Sep 21 '09 at 18:50
  • Don't you still copy the List 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 and modify the private instance. – csharptest.net Sep 21 '09 at 18:53
  • 2
    @Jared: You put a `Contract.Ensures(Contract.Result>().IsReadOnly)` in the contract classs ;) – Jon Skeet Sep 21 '09 at 18:54
  • @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 Sep 21 '09 at 18:59
  • @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` 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 Sep 21 '09 at 19:07
  • @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 Sep 21 '09 at 19:15
  • 1
    @csharptest.net I think Code contracts is the new in 4.0 – eglasius Sep 21 '09 at 20:21
  • 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 Sep 21 '09 at 20:40
  • @eglasius: No, just use IEnumerable. There is no reason anymore to use ReadOnlyCollection (see Jon's comment above, as well as his answer [here](http://stackoverflow.com/questions/491375/readonlycollection-or-ienumerable-for-exposing-member-collections/491591#491591)) – BlueRaja - Danny Pflughoeft Dec 20 '10 at 19:56
15

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
  • 344,730
  • 71
  • 640
  • 635
8

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.

Sam Harwell
  • 97,721
  • 20
  • 209
  • 280
  • +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 Sep 21 '09 at 20:43
1

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
  • 399,467
  • 113
  • 570
  • 794
  • 1
    ... if I already have some that implements IList, 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(). – csharptest.net Sep 21 '09 at 19:05
  • 1
    @csharptest.net if you argue that a IList 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 Sep 21 '09 at 19:14
  • 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 Sep 22 '09 at 03:25
0

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
  • 6,729
  • 2
  • 39
  • 47
0

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
  • 122,218
  • 32
  • 205
  • 338
0

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
  • 11,359
  • 8
  • 37
  • 46
0

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
  • 35,831
  • 5
  • 65
  • 110