Return collection as read-only

Question

I have an object in a multi-threaded environment that maintains a collection of information, e.g.:

public IList<string> Data 
{
    get 
    {
        return data;
    }
}

I currently have return data; wrapped by a ReaderWriterLockSlim to protect the collection from sharing violations. However, to be doubly sure, I'd like to return the collection as read-only, so that the calling code is unable to make changes to the collection, only view what's already there. Is this at all possible?

Solution 1

If your underlying data is stored as list you can use List(T).AsReadOnly method.
If your data can be enumerated, you can use Enumerable.ToList method to cast your collection to List and call AsReadOnly on it.

Solution 2

I voted for your accepted answer and agree with it--however might I give you something to consider?

Don't return a collection directly. Make an accurately named business logic class that reflects the purpose of the collection.

The main advantage of this comes in the fact that you can't add code to collections so whenever you have a native "collection" in your object model, you ALWAYS have non-OO support code spread throughout your project to access it.

For instance, if your collection was invoices, you'd probably have 3 or 4 places in your code where you iterated over unpaid invoices. You could have a getUnpaidInvoices method. However, the real power comes in when you start to think of methods like "payUnpaidInvoices(payer, account);".

When you pass around collections instead of writing an object model, entire classes of refactorings will never occur to you.

Note also that this makes your problem particularly nice. If you don't want people changing the collections, your container need contain no mutators. If you decide later that in just one case you actually HAVE to modify it, you can create a safe mechanism to do so.

How do you solve that problem when you are passing around a native collection?

Also, native collections can't be enhanced with extra data. You'll recognize this next time you find that you pass in (Collection, Extra) to more than one or two methods. It indicates that "Extra" belongs with the object containing your collection.

Solution 3

If your only intent is to get calling code to not make a mistake, and modify the collection when it should only be reading all that is necessary is to return an interface which doesn't support Add, Remove, etc.. Why not return IEnumerable<string>? Calling code would have to cast, which they are unlikely to do without knowing the internals of the property they are accessing.

If however your intent is to prevent the calling code from observing updates from other threads you'll have to fall back to solutions already mentioned, to perform a deep or shallow copy depending on your need.

Solution 4

I think you're confusing concepts here.

The ReadOnlyCollection provides a read-only wrapper for an existing collection, allowing you (Class A) to pass out a reference to the collection safe in the knowledge that the caller (Class B) cannot modify the collection (i.e. cannot add or remove any elements from the collection.)

There are absolutely no thread-safety guarantees.

• If you (Class A) continue to modify the underlying collection after you hand it out as a ReadOnlyCollection then class B will see these changes, have any iterators invalidated, etc. and generally be open to any of the usual concurrency issues with collections.
• Additionally, if the elements within the collection are mutable, both you (Class A) and the caller (Class B) will be able to change any mutable state of the objects within the collection.

Your implementation depends on your needs: - If you don't care about the caller (Class B) from seeing any further changes to the collection then you can just clone the collection, hand it out, and stop caring. - If you definitely need the caller (Class B) to see changes that are made to the collection, and you want this to be thread-safe, then you have more of a problem on your hands. One possibility is to implement your own thread-safe variant of the ReadOnlyCollection to allow locked access, though this will be non-trivial and non-performant if you want to support IEnumerable, and it still won't protect you against mutable elements in the collection.

Solution 5

One should note that aku's answer will only protect the list as being read only. Elements in the list are still very writable. I don't know if there is any way of protecting non-atomic elements without cloning them before placing them in the read only list.

View more solutions

Comments

[alastairs]

IEnumerable sounds like a good way forward actually. As you suggest, my intention is only to protect the collection from direct modification; the collection members are not going to be modified (or needed to be modified).

[DarkWanderer]

Disagree. As long as the domain model is good, "pay all unpaid invoices" or similar operations are trivial in linq. Putting logic to collections usually leads to smudged responsibilities

[Bill K]

And where do you suggest putting logic that manipulates mulitple instances of an object from multiple places without copying and pasting? Systems like Linq can be frightening because they make it so easy to just copy and paste a little snip of business logic rather than putting it somewhere it can be reused. Now you've got 10 copies of a tiny bug instead of just writing a simple method. When you fix the bug how do you find the rest of them? Maybe a text search will work, maybe not. NEVER copy business logic, the definition of DRY. Brevity is not the same thing.

[DarkWanderer]

I was not saying anything about copying business logic. Your suggestion, on the other hand, smells of SRP violation - "logic that manipulates multiple objects from multiple places", must be in own separate class

[Bill K]

Yes, and the place to put logic that manipulates a collection is in the "Collection Holder", like if you have a collection of invoices and want to get the "closed" invoices, you don't just iterate over the collection and select "Closed" because that's business logic and might change to "Closed" || "Paid", and you don't put it in some shared utility--you put it ON the collection--if your collection is being passed around naked without a class, where do you put it? Once you've made the "Invoice Holder" class, then having linq expressions inside to select from the contained collection is great.

[Bill K]

I should qualify this with the concept of developing scripts and tiny apps vs larger apps. If you are the only consumer of your own code, then distributing business logic throughout your code in the form of linq expressions is totally fine, it's when you have to share your code with others that it becomes a problem--like when you modify your linq expression to be closed||paid because it comes to you in a bug report but you miss the other 4 copies distributed around your codebase because implementing it in linq was "trivial" and there was no obvious class to place the logic.

[DarkWanderer]

I am developing large financial applications, so I'm not "the only consumer of the code". And the answer is simple - collections must not exist in code without an "owner" of some kind. "InvoiceCollection" is senseless. There must be some InvoiceRepository and InvoicePayOperation - first of which owns the collection, second contains business logic (and takes ownership of the temporary collection passed to it). Of course, if you are used to code duplication and "naked collections", as you say, you won't be able to see how it can be avoided.

[DarkWanderer]

Also, in the provided case, the best center of knowledge on whether the invoice can be paid is the invoice itself, so it must have a property CanBePaid - so, a bit of cleverness eliminates the need to change "in 4 places"

[Crono]

Gotta agree with DarkWanderer here. Collections should be just that: collections. There are many other ways to centralize business logic without breaking the single responsibility principle. Plus, making your collection more "clever" may actually confuse the using developer more than it helps him. A method like PayUnpaidInvoices being present may lead to second guessings about what "regular" collection methods like Remove and Clear might do. Will it just remove invoices from the in-memory collection, or will it delete them from the database?

[Bill K]

@DarkWanderer are you suggesting that encapsulation invalidates SRP? A collection (responsible for holding a list of things--not a business function but still valid as a utility) is contained inside something responsible for managing invoices which may be contained in a module managing accounting functions. This is not at all a violation of SRP, it's how encapsulation functions. Passing a collection off to another class to be operated on violates encapsulation and the rule that you "ask an object to do something to it's data, don't ask for it's data" that goes along with encapsulation.