CODEWITHSUNDEEP
c#.net
Michael Stum
Michael Stum

Reputation: 180964

Properly exposing a List<T>?

I know I shouldn't be exposing a List<T> in a property, but I wonder what the proper way to do it is? For example, doing this:

public static class Class1
{
    private readonly static List<string> _list;

    public static IEnumerable<string> List
    {
        get
        {
            return _list;
            //return _list.AsEnumerable<string>(); behaves the same
        }
    }

    static Class1()
    {
        _list = new List<string>();
        _list.Add("One");
        _list.Add("Two");
        _list.Add("Three");
    }
}

would allow my caller to simply cast back to List<T>:

    private void button1_Click(object sender, EventArgs e)
    {
        var test = Class1.List as List<string>;
        test.Add("Four"); // This really modifies Class1._list, which is bad™
    }

So if I want a really immutable List<T> would I always have to create a new list? For example, this seems to work (test is null after the cast):

    public static IEnumerable<string> List
    {
        get
        {
            return new ReadOnlyCollection<string>(_list);
        }
    }

But I'm worried if there is a performance overhead as my list is cloned every time someone tries to access it?

Upvotes: 12

Views: 3553

Answers (8)

Svish
Svish

Reputation: 158071

I asked a similar question earlier:

Based on that I would recommend that you use the List<T> internally, and return it as a Collection<T> or IList<T>. Or if it is only necessary to enumerate and not add or antyhing like that, IEnumerable<T>.

On the matter of being able to cast what you return in to other things, I would just say don't bother. If people want to use your code in a way that it was not intended, they will be able to in some way or another. I previously asked a question about this as well, and I would say the only wise thing to do is to expose what you intend, and if people use it in a different way, well, that is their problem :p Some related questions:

Upvotes: 2

jeroenh
jeroenh

Reputation: 26782

If you expose your list as IEnumerable, I wouldn't worry about callers casting back to List. You've explicitly indicated in the contract of your class that only the operations defined in IEnumerable are allowed on this list. So you have implicitly stated that the implementation of that list could change to pretty much anything that implements IEnumerable.

Upvotes: 1

Akash Kava
Akash Kava

Reputation: 39916

AsEnumerable and ReadOnlyCollection have problem when your enumeration is at midway and collection gets modified. These things are not thread safe. Returning them as an array and caching them at time of calling can be much better option.

For example,

public static String[] List{
   get{
      return _List.ToArray();
   }
} 

//While using ...

String[] values = Class1.List;

foreach(string v in values){
  ...
}

// instead of calling foreach(string v in Class1.List)
// again and again, values in this context will not be
// duplicated, however values are cached instance so 
// immediate changes will not be available, but its
// thread safe
foreach(string v in values){
  ...
}

Upvotes: 0

Marc Gravell
Marc Gravell

Reputation: 1062915

Exposing a List<T> as a property isn't actually the root of all evil; especially if it allows expected usage such as foo.Items.Add(...).

You could write a cast-safe alternative to AsEnumerable():

public static IEnumerable<T> AsSafeEnumerable<T>(this IEnumerable<T> data) {
    foreach(T item in data) yield return item;
}

But your biggest problem at the moment is thread safety. As a static member, you might have big problems here, especially if it is in something like ASP.NET. Even ReadOnlyCollection over an existing list would suffer from this:

        List<int> ints = new List<int> { 1, 2, 3 };
        var ro = ints.AsReadOnly();
        Console.WriteLine(ro.Count); // 3
        ints.Add(4);
        Console.WriteLine(ro.Count); // 4

So simply wrapping with AsReadOnly is not enough to make your object thread-safe; it merely protects against the consumer adding data (but they could still be enumerating it while your other thread adds data, unless you synchronize or make copies).

Upvotes: 8

Robban
Robban

Reputation: 6802

If the class has no other purpose you could inherit from list and override the add method and have it throw an exception.

Upvotes: 3

Martin v. L&#246;wis
Martin v. L&#246;wis

Reputation: 127467

You don't need to worry about the overhead of cloning: wrapping a collection with a ReadOnlyCollection does not clone it. It just creates a wrapper; if the underlying collection changes, the readonly version changes also.

If you worry about creating fresh wrappers over and over again, you can cache it in a separate instance variable.

Upvotes: 2

Ruben Bartelink
Ruben Bartelink

Reputation: 61795

Use AsReadOnly() - see MSDN for details

Upvotes: 2

Henrik
Henrik

Reputation: 23324

Yes and No. Yes, there is a performance overhead, because a new object is created. No, your list is not cloned, it is wrapped by the ReadOnlyCollection.

Upvotes: 4

Related Questions