ReadOnlyCollection or IEnumerable for exposing member collections?

Is there any reason to expose an internal collection as a ReadOnlyCollection rather than an IEnumerable if the calling code only iterates over the collection?

class Bar
{
    private ICollection<Foo> foos;

    // Which one is to be preferred?
    public IEnumerable<Foo> Foos { ... }
    public ReadOnlyCollection<Foo> Foos { ... }
}


// Calling code:

foreach (var f in bar.Foos)
    DoSomething(f);

As I see it IEnumerable is a subset of the interface of ReadOnlyCollection and it does not allow the user to modify the collection. So if the IEnumberable interface is enough then that is the one to use. Is that a proper way of reasoning about it or am I missing something?

Thanks /Erik

Solution 1:

More modern solution

Unless you need the internal collection to be mutable, you could use the System.Collections.Immutable package, change your field type to be an immutable collection, and then expose that directly - assuming Foo itself is immutable, of course.

Updated answer to address the question more directly

Is there any reason to expose an internal collection as a ReadOnlyCollection rather than an IEnumerable if the calling code only iterates over the collection?

It depends on how much you trust the calling code. If you're in complete control over everything that will ever call this member and you guarantee that no code will ever use:

ICollection<Foo> evil = (ICollection<Foo>) bar.Foos;
evil.Add(...);

then sure, no harm will be done if you just return the collection directly. I generally try to be a bit more paranoid than that though.

Likewise, as you say: if you only need IEnumerable<T>, then why tie yourself to anything stronger?

Original answer

If you're using .NET 3.5, you can avoid making a copy and avoid the simple cast by using a simple call to Skip:

public IEnumerable<Foo> Foos {
    get { return foos.Skip(0); }
}

(There are plenty of other options for wrapping trivially - the nice thing about Skip over Select/Where is that there's no delegate to execute pointlessly for each iteration.)

If you're not using .NET 3.5 you can write a very simple wrapper to do the same thing:

public static IEnumerable<T> Wrapper<T>(IEnumerable<T> source)
{
    foreach (T element in source)
    {
        yield return element;
    }
}

Solution 2:

If you only need to iterate through the collection:

foreach (Foo f in bar.Foos)

then returning IEnumerable is enough.

If you need random access to items:

Foo f = bar.Foos[17];

then wrap it in ReadOnlyCollection.

Solution 3:

If you do this then there's nothing stopping your callers casting the IEnumerable back to ICollection and then modifying it. ReadOnlyCollection removes this possibility, although it's still possible to access the underlying writable collection via reflection. If the collection is small then a safe and easy way to get around this problem is to return a copy instead.