The consequence of a.NET Framework API design flaw

Last week, a user reported a bug on NDepend (hopefully it happens in a very rare situation!). After mulling over a bit on the bug stack trace, it appears that the bug is the consequence of a call to ICollection<T>.Add() while the underlying collection is an array!

Here I felt bad, because really the bug is a consequence of a .NET Framework API design flaw. Or better said, to take our responsibility, the bug is the consequence of us, not providing a work around seriously enough on this  .NET Framework API design flaw. Why the hell some read-only collection implements the Add() method? It is even getting worse if considering the documentation of Array.Add() (as a IList<T> explicit implementation method) and ReadOnlyCollection.ICollection<T>.Add().

 This implementation always throws NotSupportedExceptionOUCH!

It appears that finally, a decade after .NET 1.0 has been release, 3 interfaces IReadOnlyCollection<T>IReadOnlyList<T>IReadOnlyDictionary<T> will be presented by the .NET v4.5. As commonly said, it is never too late to do things right, but in the case of an API with ascendant compatibility used by millions of developers world-wide, this proverb doesn’t fit that well.

The fact is that the brand new NDepend.API actually uses the interface ICollection<T> to return read-only lists. I wouldn’t like us or some users stumble on this API design flaw again! Hence a decision was taken to change the API policy, even though this will introduce breaking-change (the API is still young enough for that). First, I wrote the following CQLinq query to evaluate the problem:

Here is the result, 40 methods (or property getters) returns something that implement IEnumerable<T> and many of them returns a ICollection<T>.

So what the API should return instead of ICollection<T>? Knowing that NDepend is still compatible with VS2008 and hence is compiled with .NET 3.5, it cannot use the brand new .NET v4.5 IReadOnlyList<T> (but it will certainly use these in several years, when we will compile against .NET v4.5 and above).

  • Returning arrays instead of ICollection<T> could be an idea. The problem is that array is an implementation, and it is not read-only since the user can update each slot of the array.
  • I never really liked the class ReadOnlyCollection<T> that wraps a IList<T>. First it is a class and not an interface. Second it has the property getter Items that can returns the wrapped IList<T>, so it is not that read-only and not a good wrapper at all! Third it implements IList<T>, and all mutable members are not supported, so it can lead to the same kind of confusion we are now faced with.
  • Returning some IEnumerable<T> would be a better choice than the two above. It is not ideal through. While IEnumerable<T> conveys well the read-only idea, it has no Count nor an item access by index property. Hence it is not really suited to return a well-sized list. LINQ provides the adequate extension methods Count() and ElementAt(), but  IEnumerable<T> has a strong taste of sequence, while we’d really like to provide users with proper lists!

So we choose to provide our own interfaces IReadOnlyCollection<T> and IReadOnlyList<T>! Same as the ones released soon in .NET v4.5, but defined in another namespace NDepend.Helpers, this will discard most naming collisions for clients. Thanks to this choice, the API is as clean as possible for now, and the transition will be simplified once NDepend will be compiled for .NET v4.5 and above, sooner or later.

As more and more often, I found inspiration in a stackoverflow.com response here. Find below the whole interfaces declarations and implementations. This is not an ideal solution, since List<T> and Array of course doesn’t implement our interfaces, but I found it to be a good design compromise.

Notice the four factory extension methods, that make a clear distinction at creation time, between Wrapped and Cloned read-only collection. Notice as well that the Cloned factories, take any sequence as argument, while the Wrapped factories, need collections and lists as argument:

  • ToReadOnlyWrappedCollection(this ICollection<T>), 
  • ToReadOnlyClonedCollection(this IEnumerable<T>),
  • ToReadOnlyWrappedList(this IList<T>), 
  • ToReadOnlyClonedList(this IEnumerable<T>),

Also, we’ve added the IndexOf() extension method over our two interfaces, because we needed it, and because the .NET Framework doesn’t provide IndexOf(this IEnumerable<T>) because a sequence is not necessarily ordered (like hashset or dictionary).

And finally, I did a CQLinq rule to check that NDepend.API will never return anymore a ICollection<T>!

This entry was posted in .NET Framework, API usage, breaking changes, Code Rule. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Anonymous

    Dave, do you really think that having a IsReadOnly property make this ICollection API clean? 
    No it is not, this API is highly error prone, so much that I stumbled on it . If a method Add() is present, I can use it, I don’t want to check if I can or cannot use it. IReadOnlyCollection just discards the Add() method and remove any ambiguity.

  • Dave Van den Eynde

    It’s easy to blame the .NET Framework when you miss the fact that ICollection has a IsReadOnly property that you can check before you call Add().

  • http://www.facebook.com/leontivero Lucas Ontivero

    Of course I agree with what you say, it depends on the usage requirements. And that is a good reason to avoid them, it is just that I found your point against the arrays approach not very solid.

  • http://blog.tcx.be/ Tommy Carlier

    I wrote a proposition for read-only collections back in 2008. My IReadableCollection also had the method Contains. I just basically took the existing collection interfaces and removed all members that modified the collection. I named my interfaces IReadableCollection and IReadableList, because they can be implemented by classes that are read/write (so they’re not “read-only”).

    See: http://blog.tcx.be/2008/12/net-bcl-proposition-read-only.html

  • Anonymous

    >if developers want to modify its slots, what´s the problem?
    The problem is that for that, you constantly need to return cloned collections to ensure that modifying slots doesn’t have any underlying impact. Cloned collections can be something costly in term of performance and memory consumption, and often returning a “really” read-only wrapped (as opposition to cloned) collection is a better choice.

  • http://www.facebook.com/leontivero Lucas Ontivero

    I´ve had the same problem and what I do is return arrays instead of ICollection, if developers want to modify its slots, what´s the problem? I think that even when it is a mutable object, the important point is that modifying its slots doesn´t change the internal API state (and that´s the reason we always return readonly collections). 

  • Anonymous

    >You seriously think forcing clients to use Enumerable.ElementAt() for random access would be a good idea? 
    No, and as a matter of fact I don’t choose this solution.

    >
    Inventing your own interface for this? That’s a really dumb idea! :)  

    I don’t invent it, we just define the ones existing in .NET Fx v4.5, put them in our namespace system, and  will switch back to these standard ones once we’ll compile against .NET v4.5 sooner or later.

    >Interfaces are generally worse than abstract base classes

    I disagree, interfaces represent a well defined simple responsability.  Base class represents a backbone responsability that can be specialized, and still shows a lot more than an interface to the user.

  • http://blog.andrei.rinea.ro/ Andrei Rinea

    Java does the same thing too.

  • http://barrkel.blogspot.com/ barrkel

    You seriously think forcing clients to use Enumerable.ElementAt() for random access would be a good idea? OK, so ElementAt() has a special case optimization; but depending on it is really brittle, the implementation (the value returned) may change at any time. If the IEnumerable property is guaranteed to support IList (so you can depend on O(1)), then it should return IList, not IEnumerable.

    Creating your own interface is even worse though. Interfaces are generally worse than abstract base classes – this is another of your objections I don’t really agree with – for everything except very narrow slices of functionality that are never going to have their API set change. Usability alone generally demands a broader set of methods and properties that can themselves be implemented in terms of a simpler underlying interface. This by itself justifies ReadOnlyCollection.

    Inventing your own interface for this? That’s a really dumb idea! :) You’ve just guaranteed that your code won’t interface well with all the other third-party code that expects to work with the common stuff in the BCL. Defective by design…

  • Anonymous

    +1 for the  Items property protected on  ReadOnlyCollection, but still I would never use this class, in place where I really need an interface.

    -1 for the IEnumerable, actually if the IEnumerable object is also a IList, the implementation of the extension method Enumerable.ElementAt() is smart enough to retrieve the element by index in O(1). But this is an implementation detail, and this finally advocates that having our own IReadOnlyList with a clearly O(1) indexer property is a good idea.

  • http://barrkel.blogspot.com/ barrkel

    You’re overstating how poor ReadOnlyCollection is. The Items property is protected; it is only accessible to descendants, and they can already access the wrapped IList through the constructor they need to provide. Returning an IEnumerable instead of a ReadOnlyCollection for something that supports O(1) random access is a *terrible* idea; it is not better at all.