21

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)?

goenning
  • 6,514
  • 1
  • 35
  • 42
  • 1
    +1, **surely** Microsoft had some reason for doing this, rather than using separate interfaces-- after all, they're smart people. I'm curious to hear an explanation of why they did. – Philip Guin Nov 18 '12 at 21:06

8 Answers8

9

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.

Timwi
  • 65,159
  • 33
  • 165
  • 230
  • 1
    There is IsReadOnly property you could check before calling those methods. Is it also a bad design decision for Insert method of the same interface to throw ArgumentOutOfRangeException when index is negative or bigger or equal than Count property? Because, by your argumentation, it sure is. – Ivan Ferić Oct 08 '10 at 23:59
  • Couldn't they at least add an `[Obselete]` attribute above the `Add()` etc methods so it throws a compiler error instead of letting the problem code be caught at runtime? – Callum Rogers Oct 09 '10 at 00:26
  • 1
    You can't know wherever your IList variable is a readonly or not List at compile time. – goenning Oct 09 '10 at 00:29
  • @Ivan: Where does my answer talk about `Insert` or about `ArgumentOutOfRangeException`? Please don’t put things into my mouth that I didn’t say. – Timwi Oct 09 '10 at 00:49
  • @Timwi I didn't say that you said it but that your argumentation on NotSupportedException would lead to that conclusion. – Ivan Ferić Oct 09 '10 at 00:51
  • @Callum: `ReadOnlyCollection<>` hides the unsupported members using explicit interface implementation anyway. If your variable is typed as a `ReadOnlyCollection<>` then those members will be inaccessible. The problem lies when your variable is typed as `IList<>`: the unsupported members then become accessible, and even if the underlying implementation did mark them as obsolete the compiler couldn't/wouldn't warn about it. – LukeH Oct 09 '10 at 01:39
  • @Ivan: No, it doesn’t. It’s a false analogy. – Timwi Oct 09 '10 at 15:02
  • 1
    @Timwi: Is it? They both have a way to be checked if it's gonna throw an exception, they are both documented to be thrown (with conditions when they are to be thrown) and they are both in the same interface. What's the difference? – Ivan Ferić Oct 09 '10 at 15:16
  • 2
    @Ivan: One difference is that it *makes sense* and is *reasonably practical* to have a read-only interface and to reserve the writable interface to instances that are actually writable. Another difference is that it is *reasonably practical* for the client code to check array bounds before using the indexer, but to have to call `Add` just to check whether it throws is completely ridiculous and only serves to cause buggy code. (The `IsReadOnly` property is not reliable: `((IList)new byte[0]).IsReadOnly` returns `false`, but `((IList)new byte[0]).Add((byte)0)` still throws!) – Timwi Oct 09 '10 at 17:15
  • 1
    @Ivan: In general, just because you can’t think of a difference, doesn’t mean there isn’t any, and in particular, it doesn’t mean that someone’s argumentation somehow “leads to a conclusion”. You should be more careful making claims about what someone else said and implied. – Timwi Oct 09 '10 at 17:26
  • 1
    @Ivan: I just noticed that `IList` actually has a separate `IsFixedSize` property. How many more of these boolean properties should an interface have? Why don’t we have an interface with 100 methods and then 100 separate properties to say whether each of them throws? Because it’s bad design. – Timwi Oct 09 '10 at 17:28
  • @Timwi: First I want to say that I didn't mean to offend you in any way. Second, I didn't say that you said nor implied those things - I simply drew a parallel between your assertion and something that we both now is not true (that `IList.Insert(int, T)` is a bad design example for throwing `ArgumentOutOfRange` exception). And third, if what you say about `byte[]` is true, it still doesn't imply that `IList` is badly designed, but rather that implementation of `IList` in `Array` is bad. – Ivan Ferić Oct 09 '10 at 19:43
  • @Timwi, re: the 100 methods / 100 separate properties -- aren't you suggesting that the alternative is 100 separate interfaces? Not sure it's obvious that solution is better. – Kirk Woll Oct 09 '10 at 22:18
  • @Kirk: No, I’m not; see my first comment on bde’s answer. – Timwi Oct 10 '10 at 07:23
  • @Timwi: How about having a single `Attributes` property that returns a `ListAttrbributes` enum? That would easily allow for dozens of distinct characteristics to be handled with a single method declaration. – supercat Mar 07 '13 at 19:51
9

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.

Ivan Ferić
  • 4,725
  • 11
  • 37
  • 47
5

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.

LukeH
  • 263,068
  • 57
  • 365
  • 409
  • +1 .That's a good suggestion, I liked it. – goenning Oct 09 '10 at 00:21
  • 1
    The Objective-C solution is good (except it does not have Generics/Templates). There, you have ArrayList (immutable) and MutableArrayList (mutable). That way, you instinctively use an immutable variant and actively decide when you really need a mutable version, which usually turns out to be quite rare. – Ricky Helgesson Jul 12 '12 at 14:01
  • 1
    @RickyHelgesson: Is `ArrayList` immutable, or merely read-only? I would posit that the right approach is to have a read-only interface, and mutable and immutable interfaces that derive from it. Code which expects to encapsulate the contents of a collection merely by holding a reference could take a parameter of immutable interface type, while code which needs to read something now and doesn't care that it might (or might not) change later could accept the readable interface. – supercat Mar 07 '13 at 19:49
  • @supercat I never thought about that pattern but I like it. The only concern for me would be that the average user might not understand the important difference between the readonly and immutable interfaces as they seem identical. I have written an article about mutable/immutable model data btw http://rickyhelgesson.wordpress.com/2012/07/17/mutable-or-immutable-in-a-parallel-world/ I might do another pass on my Model architecture with your pattern in mind if I can get it simple enough. – Ricky Helgesson Mar 09 '13 at 11:35
  • @supercat Re: Objective-C, I have not used it for a while but if I remember correctly, ArrayList is totally immutable. – Ricky Helgesson Mar 09 '13 at 11:41
2

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.

Shay Erlichmen
  • 31,691
  • 7
  • 68
  • 87
  • Yes, but that doesn’t contradict the observation that it is bad design for `ReadOnlyCollection` to implement `IList`. – Timwi Oct 08 '10 at 23:25
2

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.

WildCrustacean
  • 5,896
  • 2
  • 31
  • 42
  • 3
    I agree that it would be bad design to have a separate interface for every single capability (`IAddableCollection`, `IRemovableCollection`, `IIndexableCollection`, ...), but I would have thought that *readonlyness* is a sufficiently fundamental property of a collection type to warrant separation. (Besides, it would enable covariance!) – Timwi Oct 08 '10 at 23:30
  • @Timwi: Maybe so. It would be interesting to hear about the design decisions that went into the collection interfaces. – WildCrustacean Oct 08 '10 at 23:33
  • I wouldn't say that the path you mention is 'crazy' in any way. A well designed framework would expose `NSArray` as well as `NSMutableArray`, `NSDictionary` and `NSMutableDictionary`, `NSSet` and `NSMutableSet` etc etc... – Remus Rusanu Oct 08 '10 at 23:40
  • @Remus: I'm not saying that it is crazy to expose mutable and nonmutable versions of things, and in this case that would probably have been better. I am saying that you can take things too far by making specialized interfaces, and the result is very confusing. – WildCrustacean Oct 08 '10 at 23:53
1

Its bad design IMO. Probably forced by backward compatibility issues, missing co-variance and contra-variance etc. etc. Now luckily they addressed it in .NET 4.5 with the:

IReadOnlyList<out T>
IReadOnlyCollection<out T>
IReadOnlyDictionary<TKey, TValue>

However I am missing "read-only" interface with "bool Contains(T)".

Panayot Cankov
  • 11
  • 2
  • Add an extension method if you want the functionality. You can easily implement it using the interface's other members. – Servy Jul 03 '13 at 13:57
1

I would say it's a bad design. Even if one accepts the concept of a moderately large interface with capability queries, there should have been other interfaces which inherited from them which would guarantee that certain behaviors be allowed. For example:

  1. IEnumerable (much like existing one, but without reset, and no promise of what will happen if the collection is changed during enumeration)
  2. IMultipassEnumerable (adds reset, and guarantees repeated enumerations of a changing collection will either return the same data or throw and exception)
  3. ICountableEnumerable (a multipass enumerable, plus a 'count' property, and a method to get an enumerator and count simultaneously)
  4. IModifiableEnumerable (an IEnumerator which will not throw if a collection is modified during enumeration by the thread doing the enumerating. The precise behavior would not be specified, but items which are unchanged during enumeration must be returned exactly once; those which are modified during enumeration must be returned at most once for each addition or modification, plus one if they existed at enumeration start. This interface itself does not provide any mutations, but would be used in conjunction with others that do).
  5. ICopyableAsEnumerable (includes a count property, and a method to return an IEnumerable which represents a snapshot of the list; not actually an IEnumerable itself, but a useful feature for an IEnumerable to provide).
  6. IImmutable (no members, but inheritable to create guaranteed-immutable interfaces)
  7. IImmutableEnumerable
  8. IImmutableCountableEnumerable
  9. IList (could be readable, read-write, or immutable)
  10. IImmutableList (no new members, but inherits IImmutable)
  11. IWritableList (no new members, but is guaranteed writable)

That's just a small sampling, but should convey the design idea.

supercat
  • 77,689
  • 9
  • 166
  • 211
-3

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.

Kirk
  • 4,431
  • 1
  • 15
  • 8
  • 4
    I disagree. The interface is poor as it contains both Add and ReadOnly. A good design would be IReadOnlyList, which ReadOnlyCollection would implement and having IList inherit from IReadOnlyList. The ReadOnly bodge property would no longer be needed and anything implementing IList would then support Add etc. – David Arno May 25 '12 at 15:50
  • 1
    I disagree. You want the compiler to help you find bad usage of classes and interfaces. This is not the case with ReadOnlyCollection. – Ricky Helgesson Mar 09 '13 at 11:02