Best practice when assigning a collection reference to a property

Question

I'm heavily geared towards C++ thinking and need some guidance on a specific C# matter. Let's assume we have the following class:

public class Foo
{
    private IList<Bar> _bars = new List<Bar>(); // Note IList<> vs List<>.

    public IList<Bar> Bars
    {
        get { return _bars; }
        set
        {
            ...
        }
    }
}

Now, in place of the ..., I'm leaning towards clearing _bars and AddRange the items from the set argument value, instead of just assigning value to _bars. The way I see it, is that I want to keep referencing the same items that the value items references, not the actual IList<Bar> that value references.

Is that wrong thinking on my side? How do you think here?

Edit: After some comments, I realized I must add that I want to be able to use an existing collection of Bars in the Foo ctor and initialize _bars from that collection. So, with that revision and the comments so far, this feels better:

public class Foo
{
    private readonly List<Bar> _bars = new List<Bar>();

    public Foo(IEnumerable<Bar> bars)
    {
        _bars.AddRange(bars);
    }

    public IList<Bar> Bars
    {
        get { return _bars; }
    }
}

Is it better?

Answer 1

Is it necessary to be able to make the Bars property of your instance of Foo refer to another List ?

If not, then I would prefer not to have a property-setter for Bars.

If it is necessary to be able to set the Bars property to another List instance, then I think I would find it strange to find that your setter modifies the list. I mean: when I assign another List of Bars to that property, I would assume that my instance of Foo then contains the Bars that were available in the list that I've assigned to that property. No more, no less. I would find it strange to find out that suddenly the collection contains more (or less) Bars...

Answer 2

Based on inferred requirements from your question:

public class Foo
{
    private readonly IList<Bar> _bars = new List<Bar>();

    public IList<Bar> Bars
    {
        get { return _bars; }
    }
}

HTH, Kent

Answer 3

If we're talking about business (domain) objects, I expose IEnumerable<T> and whatever methods are appropriate for the collection, typically Add, Remove, and Clear. I almost never expose a setter for a collection.

Answer 4

Absolutely, there is no need to expose a setter for the Bars property. Internally you hold a reference to the collection, which as Kent suggests could be marked as Readonly. Through the getter the caller can do what they want with the collection with the methods available (Add, Remove, Clear, AddRange, etc), but crucially they can never change the internal reference you hold to the collection object.

This then allows you to control what methods are allowed. As Jamie suggests, having the property return type IEnumerable would result in the Bars property exposing a readonly collection. Exposing IList means the contents of the collection could be modified. A setter on the property would leave it wide open for the caller to do what they want and you are no longer in control.

Edit

Following the question edit above. It really depends on how the Foo object will be used.

Since your main concern is to initialise Foo from an existing list of Bar objects...

IList<Bar> bars = ...some list of Bars previously constructed...

Your latest code example for Foo forces the caller to initialise via the constructor but then also allows them to change the collection via the property

Foo foo = new Foo(bars);
...
foo.Bars.Clear();
foo.Bars.AddRange(bars);

When allowing an object to be initialised via the constructor you need to ask yourself why you are doing this. Is it...

1. for the caller's convenience? To allow the calling code to supply values that can be subsequently changed via properties.
2. because you want to restrict how the object is used? Forcing values (or certain combinations of values) to be set on object construction and remain fixed throughout the lifetime of the object.

You need to ask yourself – Do you want the caller to be able to change the contents of the Bars collection after the Foo object has been constructed?

If No – Make the Bars property expose a read only collection.
If Yes – Add a default constructor to the Foo object so that the caller doesn’t have to supply a list to initialise it. But they will have the option to do so if they so choose via the overloaded constructor.

Answer 5

I'm ok with setters for such properties, but I usually disallow nulls because null rarely has different business meaning then empty list.

set { _bars = value ?? new List<Bar>();}

Answer 6

Please find a similar question here, with more clarity.

Answer 7

I think returning the _bars reference from the getter is a bit dodgy. Do you really want clients of the class being able to modify the contents of the list?

So I would do the following: make a copy on construction/setting and return a readonly view for the getter.