IEnumerable vs IReadonlyCollection vs ReadonlyCollection for exposing a list member

I have spent quite a few hours pondering the subject of exposing list members. In a similar question to mine, Jon Skeet gave an excellent answer. Please feel free to have a look.

ReadOnlyCollection or IEnumerable for exposing member collections?

I am usually quite paranoid to exposing lists, especially if you are developing an API.

I have always used IEnumerable for exposing lists, as it is quite safe, and it gives that much flexibility. Let me use an example here:

public class Activity
{
    private readonly IList<WorkItem> workItems = new List<WorkItem>();

    public string Name { get; set; }

    public IEnumerable<WorkItem> WorkItems
    {
        get
        {
            return this.workItems;
        }
    }

    public void AddWorkItem(WorkItem workItem)
    {
        this.workItems.Add(workItem);
    }
}

Anyone who codes against an IEnumerable is quite safe here. If I later decide to use an ordered list or something, none of their code breaks and it is still nice. The downside of this is IEnumerable can be cast back to a list outside of this class.

For this reason, a lot of developers use ReadOnlyCollection for exposing a member. This is quite safe since it can never be cast back to a list. For me I prefer IEnumerable since it provides more flexibility, should I ever want to implement something different than a list.

I have come up with a new idea I like better. Using IReadOnlyCollection:

public class Activity
{
    private readonly IList<WorkItem> workItems = new List<WorkItem>();

    public string Name { get; set; }

    public IReadOnlyCollection<WorkItem> WorkItems
    {
        get
        {
            return new ReadOnlyCollection<WorkItem>(this.workItems);
        }
    }

    public void AddWorkItem(WorkItem workItem)
    {
        this.workItems.Add(workItem);
    }
}

I feel this retains some of the flexibility of IEnumerable and is encapsulated quite nicely.

I posted this question to get some input on my idea. Do you prefer this solution to IEnumerable? Do you think it is better to use a concrete return value of ReadOnlyCollection? This is quite a debate and I want to try and see what are the advantages/disadvantages that we all can come up with.

Thanks in advance for your input.

EDIT

First of all thank you all for contributing so much to the discussion here. I have certainly learned a ton from each and every one and would like to thank you sincerely.

I am adding some extra scenarios and info.

There are some common pitfalls with IReadOnlyCollection and IEnumerable.

Consider the example below:

public IReadOnlyCollection<WorkItem> WorkItems
{
    get
    {
        return this.workItems;
    }
}

The above example can be casted back to a list and mutated, even though the interface is readonly. The interface, despite it's namesake does not guarantee immutability. It is up to you to provide an immutable solution, therefore you should return a new ReadOnlyCollection. By creating a new list (a copy essentially), the state of your object is safe and sound.

Richiban says it best in his comment: a interface only guarantees what something can do, not what it cannot do.

See below for an example:

public IEnumerable<WorkItem> WorkItems
{
    get
    {
        return new List<WorkItem>(this.workItems);
    }
}

The above can be casted and mutated, but your object is still immutable.

Another outside the box statement would be collection classes. Consider the following:

public class Bar : IEnumerable<string>
{
    private List<string> foo;

    public Bar()
    {
        this.foo = new List<string> { "123", "456" };
    }

    public IEnumerator<string> GetEnumerator()
    {
        return this.foo.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }
}

The class above can have methods for mutating foo the way you want it to be, but your object can never be casted to a list of any sort and mutated.

Carsten Führmann makes a fantastic point about yield return statements in IEnumerables.

Thank you all once again.

Solution 1:

One important aspect seems to be missing from the answers so far:

When an IEnumerable<T> is returned to the caller, they must consider the possibility that the returned object is a "lazy stream", e.g. a collection built with "yield return". That is, the performance penalty for producing the elements of the IEnumerable<T> may have to be paid by the caller, for each use of the IEnumerable. (The productivity tool "Resharper" actually points this out as a code smell.)

By contrast, an IReadOnlyCollection<T> signals to the caller that there will be no lazy evaluation. (The Count property, as opposed to the Count extension method of IEnumerable<T> (which is inherited by IReadOnlyCollection<T> so it has the method as well), signals non-lazyness. And so does the fact that there seem to be no lazy implementations of IReadOnlyCollection.)

This is also valid for input parameters, as requesting an IReadOnlyCollection<T> instead of IEnumerable<T> signals that the method needs to iterate several times over the collection. Sure the method could create its own list from the IEnumerable<T> and iterate over that, but as the caller may already have a loaded collection at hand it would make sense to take advantage of it whenever possible. If the caller only has an IEnumerable<T> at hand, he only needs to add .ToArray() or .ToList() to the parameter.

What IReadOnlyCollection does not do is prevent the caller to cast to some other collection type. For such protection, one would have to use the class ReadOnlyCollection<T>.

In summary, the only thing IReadOnlyCollection<T> does relative to IEnumerable<T> is add a Count property and thus signal that no lazyness is involved.

Solution 2:

Talking about class libraries, I think IReadOnly* is really useful, and I think you're doing it right :)

It's all about immutable collection... Before there were just immutables and to enlarge arrays was a huge task, so .net decided to include in the framework something different, mutable collection, that implement the ugly stuff for you, but IMHO they didn't give you a proper direction for immutable that are extremely useful, especially in a high concurrency scenario where sharing mutable stuff is always a PITA.

If you check other today languages, such as objective-c, you will see that in fact the rules are completely inverted! They quite always exchange immutable collection between different classes, in other words the interface expose just immutable, and internally they use mutable collection (yes, they have it of course), instead they expose proper methods if they want let the outsiders change the collection (if the class is a stateful class).

So this little experience that I've got with other languages pushes me to think that .net list are so powerful, but the immutable collection were there for some reason :)

In this case is not a matter of helping the caller of an interface, to avoid him to change all the code if you're changing internal implementation, like it is with IList vs List, but with IReadOnly* you're protecting yourself, your class, to being used in not a proper way, to avoid useless protection code, code that sometimes you couldn't also write (in the past in some piece of code I had to return a clone of the complete list to avoid this problem).

Solution 3:

It seems that you can just return an appropriate interface:

...
    private readonly List<WorkItem> workItems = new List<WorkItem>();

    // Usually, there's no need the property to be virtual 
    public virtual IReadOnlyList<WorkItem> WorkItems {
      get {
        return workItems;
      }
    }
...

Since workItems field is in fact List<T> so the natural idea IMHO is to expose the most wide interface which is IReadOnlyList<T> in the case

Solution 4:

My take on concerns of casting and IReadOnly* contracts, and 'proper' usages of such.

If some code is being “clever” enough to perform an explicit cast and break the interface contract, then it is also “clever” enough to use reflection or otherwise do nefarious things such as access the underlying List of a ReadOnlyCollection wrapper object. I don’t program against such “clever” programmers.

The only thing that I guarantee is that after said IReadOnly*-interface objects are exposed, then my code will not violate that contract and will not modified the returned collection object.

This means that I write code that returns List-as-IReadOnly*, eg., and rarely opt for an actual read-only concrete type or wrapper. Using IEnumerable.ToList is sufficient to return an IReadOnly[List|Collection] - calling List.AsReadOnly adds little value against “clever” programmers who can still access the underlying list that the ReadOnlyCollection wraps.

In all cases, I guarantee that the concrete types of IReadOnly* return values are eager. If I ever write a method that returns an IEnumerable, it is specifically because the contract of the method is that which “supports streaming” fsvo.

As far as IReadOnlyList and IReadOnlyCollection, I use the former when there is 'an' implied stable ordering established that is meaningful to index, regardless of purposeful sorting. For example, arrays and Lists can be returned as an IReadOnlyList while a HashSet would better be returned as an IReadOnlyCollection. The caller can always assign the I[ReadOnly]List to an I[ReadOnly]Collection as desired: this choice is about the contract exposed and not what a programmer, “clever” or otherwise, will do.