Collection properties should be read only - Loopho

Question

In the process of adhering to code analysis errors, I'm changing my properties to have private setters. Then I started trying to understand why a bit more. From some research, MS says this:

A writable collection property allows a user to replace the collection with a completely different collection.

And the answer, here, states:

Adding a public setter on a List<T> object is dangerous.

But the reason why it's dangerous is not listed. And that's the part where I'm curious.

If we have this collection:

public List<Foo> Foos { get; set; }

Why make the setter private? Apparently we don't want client code to replace the collection, but if a client can remove every element, and then add whatever they want, what's the point? Is that not the same as replacing the collection entirely? How is value provided by following this code analysis rule?

Answer 1

If you have a Customer class with a List Property then this property should always have a private setter else it can be changed from outside the customer object via:

customer.Orders = new List<Order> 
//this could overwrite data.

Always use the add and remove methods of the collection.

The Orders List should be created inside the Customer constructor via:

Orders = new List<Order>();

Do you really want to check everywhere in your code wether the customer.Orders != null then operate on the Orders?

Or you create the Orders property in your customer object as suggested and never check for customer.Orders == null instead just enumerate the Orders, if its count is zero nothing happens...

Answer 2

Not exposing the setter prevents a situation where the collection is assigned a value of null. There's a difference between null and a collection without any values. Consider:

for (var value in this.myCollection){ // do something

When there are no values (i.e. someone has called Remove on every value), nothing bad happens. When this.myCollection is null, however, a NullReferenceException will be thrown.

Code Analysis is making the assumption that your code doesn't check that myCollection is null before operating on it.

It's probably also an additional safeguard for the thread-safe collection types defined in System.Collections.Concurrent. Imagine some thread trying to replace the entire collection by overwritting it. By getting rid of the public setter, the only option the thread has is to call the thread-safe Add and Remove methods.

Answer 3

In addition to SimpleCoder's null checking (which is, of course, important), there's other things you need to consider.

• Someone could replace the List, causing big problems in thread safety
• Events to a replaced List won't be sent to subscribers of the old one
• You're exposing much, much more behavior then you need to. For example, I wouldn't even make the getter public.

To clarify point 3, don't do cust.Orders.clear(), but make a function called clearOrders() instead.

What if a customer isn't allowed to go over a credit limit? You have no control over that if you expose the list. You'd have to check that (and every other piece of business logic) every place where you might add an order. Yikes! That's a lot of potential for bugs. Instead, you can place it all in an addOrder(Order o) function and be right as rain.

For almost every (I'd say every, but sometimes cheating feels good...) business class, every property should be private for get and set, and if feasible make them readonly too. In this way, users of your class get only behaviors. Protect as much of your data as you can!

Answer 4

ReadOnlyCollection and ReadOnlyObservableCollection exists only for read only collection scenearios.

ReadOnlyObservableCollection is very useful for one way binding in WPF/Silverlight/Metro apps.

Answer 5

If you're exposing an IList (which would be better practice) the consumer could replace the collection with an entirely different class that implements IList, which could have unpredictable effects. You could have subscribed to events on that collection, or on items in that collection that you're now incorrectly responding to.