Is the ReadOnlyCollection class a good example of Bad Design?

Question

Look at the specification of the ReadOnlyCollection class, it does implements the IList interface, right.

The IList interface have Add/Update/Read methods, which we call it pre-conditions of the interface. Anywhere in my application if I have an IList I should be able to do all this kind of operations.

But what about if I return a ReadOnlyCollection somewhere in my code and try to call the .Add(...) method? It throws a NotSupportedException. Do you think this is a good example of a bad design? Additionally, is this class breaking the Liskov Substitution Principle?

Why did Microsoft implemented this way? Should it be easier (and better) to make this ReadOnlyCollection implements only the IEnumerable interface (which is, by the way, already readonly)?

Answer 1

IList has some read method and properties like Item, and IndexOf(..). If ReadOnlyCollection would implement IEnumerable only then you would miss out on those.

---

Whats the alternative? Having a readonly version of IList and a write version? That would complicate the entire BCL (not to talk about LINQ).

---

Also I don't think it violates the Liskov Substitution Principle because it is defined at the base level (of IList) that it can throw a not supported exception.

Answer 2

Yes, it is bad design indeed. The collection interfaces are lacking in .NET: there are no read-only interfaces.

Did you know that string[] implements IList<string> (and ditto for other types)? This has the same problem: you would expect that you can call Add and Remove on the interface, but it would throw.

Unfortunately, this cannot be changed anymore without breaking backwards compatibility, but I agree with you that it is very bad design. A better design would have seen separate interfaces for the read-only capabilities.

Answer 3

I think it's a good example of a trade off between abstraction and specialization.

You want the flexibility of IList, but you also want to impose some constraints, so what do you do? The way it's designed is a little awkward, and probably technically violates some design principles, but I'm not sure what would be better and still give you the same functionality and simplicity.

In this case it may have been better to have a separate IListReadOnly interface. However, it is easy to go down the path to crazy one time use interface proliferation land and make things very confusing.

Answer 4

It's not a great design, but a necessary evil in my opinion.

It's unfortunate that Microsoft didn't include something like IReadableList<> and IWriteableList<> in the framework and have IList<> itself implement both of those (or even skip IList<> altogether and have IWriteableList<> implement IReadableList<>). Problem solved.

But it's too late to change now, and if you have a situation where you need your collection to have list semantics and you'd prefer to throw an exception at runtime rather than allow mutations, then ReadOnlyCollection<> is, unfortunately, your best option.

Answer 5

Although IList<T> interface defines Add(T) and Insert(int,T) methods, it also defines IsReadOnly property and if you read carefully definition of IList.Insert(int,T) and IList.Add(T) methods on MSDN, you can see that they both specify that methods could throw NotSupportedException if list is read-only.

Saying that it's bad design for that reason is like saying that it is also bad design because Insert(int, T) can throw ArgumentOutOfRangeException when index is negative or bigger than the size of collection.

Answer 6

I think if there is a bad design going on it is a habit of adding to an IList without checking the ReadOnly property. The habit of programmers to ignore portions of an interface doesn't mean the interface is poor.

The truth is that few of us programmers ever bother to read the specifications. And truthfully there are many things that seem more exciting to me than sitting down and reading through the entire specification document. (Things like seeing if one really can hold the eyes open with toothpicks for example.) Besides, I have the limitation that I wouldn't remember everything anyway.

Having said that, one should not use an interface without at least looking at the list of properties and methods. And just what purpose do you think a boolean property named "ReadOnly" is for? Perhaps because the list can be read only for one reason or another. And if you are taking a list passed from someplace outside your own code you should check that the list is not read only before you try to add to it.