3

Are there any guidelines to return an object of a class? I have a class which has a List and a method which do something with the list and returns that list:

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

  public List<Bar> DoSomething()
  {
    // Add items to the list
    return _myList;
  }

}

I don't think this is a good way to return the list, because now the calling method can modify the list and thus the list in the object Foo is updated. This can lead to unexpected and unwanted behaviour.

How do you handle this kind of situations? Do you make a copy of the object (in this case the list) and return that object, or.. ? Are there any best practices or tricks?

Martijn
  • 24,441
  • 60
  • 174
  • 261
  • Do you trust your caller to do the "right thing", if not the cost of writing a class becomes a lot more. Hence framework code costs a lot more to write then app code. – Ian Ringrose Nov 05 '10 at 10:25

6 Answers6

7

Return a new ReadOnlyCollection:

public ReadOnlyCollection<Bar> DoSomething()
{
  // Add items to the list
  return new ReadOnlyCollection<Bar>(_myList);
}

This is a wrapper for the list and the type is explicitly a read only type.

As @Freed notes, this is not thread safe, as it is only a wrapper and the list can change within the Foo class.

For better thread safety, make a copy before returning it (though if this is a requirement you should design the class for that to begin with):

public ReadOnlyCollection<Bar> DoSomething()
{
  // Add items to the list
  return new ReadOnlyCollection<Bar>(new List<Bar>(_myList));
}
Oded
  • 489,969
  • 99
  • 883
  • 1,009
  • 1
    Note however that the class itself can still modify the collection, resulting in the read only collection being modified, making it unsuitable for use in multithreaded applications. – SoftMemes Nov 05 '10 at 09:58
  • @freed that's why i do not like `ReadOnlyCollection` –  Nov 05 '10 at 09:58
  • 2
    There's a lot more to thread safety than this, either design for thread safety all the way, or don't do it at all, the overhead isn't worth it just to be "more thread-safe" – Lasse V. Karlsen Nov 05 '10 at 10:05
2

If you want to make sure that a collection of elements cannot be modified, use a ReadOnlyCollection to communicate this intent.

Alternatively, you could instance a new list and return the elements in that new list. Then, your class does not have to care whether that list is modified or not.

flq
  • 22,247
  • 8
  • 55
  • 77
1

the real problem is not about a copy of your private _myList, instead it would be: do you hand out a copy of the items as well?

for returning a copy of the list, you have several options

  • ReadOnlyCollection<T>
  • IEnumerable<T> (propably a real conversion, otherwise the attacker could cast back to a List<T> on the outside)
  • a copy via .ToList()
  • calling the ctor of List<T> with the private list

i'm not that fan of ReadOnlyCollection<T> as it just strips the ability for the consumer to add and delete something, but the connection to the private list is not cut. so when you change your private list, it affects the outhanded readonly-collection... which might be a bad thing!

i suggest to choose an option, where the return-value is fully isolated of your internal list and items!

  • Is there any difference in performance between `.ToList()` or the use of the constructor of `List`? – Martijn Nov 05 '10 at 10:08
  • dunno ... you would have to test it against a stopwatch ... actually never thought about that ... –  Nov 05 '10 at 10:11
  • I just experienced that the constructor version keeps the reference. When I do `Collection newList = new Collection(_myList);` and then add an object to the newList, _myList is also updated. How is that possible? – Martijn Nov 05 '10 at 10:49
0

how about

return new List<Bar>(_myList);

would that work? Sorry - too slow ;)

Jakob
  • 4,784
  • 8
  • 53
  • 79
0

You might want to consider ReadOnlyCollection<T> - see the answers to Best practice when returning an array of values (.NET)

Community
  • 1
  • 1
Ian Griffiths
  • 14,302
  • 2
  • 64
  • 88
0

You could use the AsReadOnly extension

Richard Friend
  • 15,800
  • 1
  • 42
  • 60