Exposing Generic Lists

[Guest]

We have a class that has a public property that is of type List<T>. FXCop
generates a DoNotExposeGenericLists error, indicating

"System.Collections.Generic.List<T> is a generic collection designed for
performance not inheritance and, therefore, does not contain any virtual
members. The following generic collections are designed for inheritance and
should be exposed instead of System.Collections.Generic.List<T>.

* System.Collections.ObjectModel.Collection<T>
* System.Collections.ObjectModel.ReadOnlyCollection<T>
* System.Collections.ObjectModel.KeyedCollection<TKey, TItem> "

Our property is not "virtual" and thus cannot be overridden. We used
List<T> becasue we wanted a generic array. A collection is unordered and
thus not as good a match, even if it is better designed for inheritance.

Can anyone explain in more depth why having a non-virtual, List<t> property
is bad? Why should I care is List<T> has any virtual members when I am
simply using an instance of and _not_ inheriting from List<T>?

Thanks,
--BJ

 

[Dustin Campbell]

FXCop generates a DoNotExposeGenericLists error, indicating

"System.Collections.Generic.List<T> is a generic collection designed
for performance not inheritance and, therefore, does not contain any
virtual members. The following generic collections are designed for
inheritance and should be exposed instead of
System.Collections.Generic.List<T>.

* System.Collections.ObjectModel.Collection<T>
* System.Collections.ObjectModel.ReadOnlyCollection<T>
* System.Collections.ObjectModel.KeyedCollection<TKey, TItem> "
Our property is not "virtual" and thus cannot be overridden. We used
List<T> becasue we wanted a generic array. A collection is unordered
and thus not as good a match, even if it is better designed for
inheritance.

Can anyone explain in more depth why having a non-virtual, List<t>
property is bad? Why should I care is List<T> has any virtual members
when I am simply using an instance of and _not_ inheriting from
List<T>?

List<T> is intended to be a workhorse class used in implementation. By exposing
a List<T> in the interface of your class, you are breaking encapsulation
by giving client code access to an implementation detail of your class--especially
if your property provides direct access to the internal List<T>. For example,
this breaks encapsulation:

public class EmployeeCensus
{
private List<Employee> m_Employees;

public EmployeeCensus(IEnumerable<Employee> employees)
{
m_Employees = new List<Employee>(employees);
}

public List<Employee> Employees { get { return m_Employees; } }
}

Any client code of the EmployeeCensus class could easily corrupt its state
by calling methods on the List<Employee> exposed by the Employees property.
And, if client code is responsible for maintaining the state of the EmployeeCensus
class, there's a design issue.

Proper encapsulation is to *not* expose the List<T> but to expose a Collection<T>,
ReadOnlyCollection<T> or KeyedCollection<TKey, TItem> like this:

public class EmployeeCensus
{
private List<Employee> m_Employees;

public EmployeeCensus(IEnumerable<Employee> employees)
{
m_Employees = new List<Employee>(employees);
}

public ReadOnlyCollection<Employee> Employees { get { return m_Employees.AsReadOnly();
} }
}

It's a shame that these classes were included in a different namespace (System.Collections.ObjectModel)
because many developers haven't discovered them yet or realize that they
should be using them.

Best Regards,
Dustin Campbell
Developer Express Inc.

 

[David Browne]

We have a class that has a public property that is of type List<T>. FXCop
generates a DoNotExposeGenericLists error, indicating

"System.Collections.Generic.List<T> is a generic collection designed for
performance not inheritance and, therefore, does not contain any virtual
members. The following generic collections are designed for inheritance
and
should be exposed instead of System.Collections.Generic.List<T>.

* System.Collections.ObjectModel.Collection<T>
* System.Collections.ObjectModel.ReadOnlyCollection<T>
* System.Collections.ObjectModel.KeyedCollection<TKey, TItem> "

Our property is not "virtual" and thus cannot be overridden. We used
List<T> becasue we wanted a generic array. A collection is unordered and
thus not as good a match, even if it is better designed for inheritance.

Can anyone explain in more depth why having a non-virtual, List<t>
property
is bad? Why should I care is List<T> has any virtual members when I am
simply using an instance of and _not_ inheriting from List<T>?

List<T> is a particular concrete type, and as you rev your library you will
never be able to replace the return value with another type without breaking
your clients. The basic problem is that you are exposing too much internal
detail in the public contract of your class.

I would simply change the return value from List<T> to IList<T>.

David

 

[Guest]

Dustin,

This makes little sense to me. Are you really improving encapsulation by
exposing this list as a Collection<T> instead of List<T>? I can buy the
arguement for a read only version, but we know that many things within the
framework (like databinding) would want to get to the AddNew() methods.

If you really want to hide the implementation, I would say an argument could
be made for declare your property as IList<T> instead of the concrete
List<T>.

-Casey

 

[Dustin Campbell]

This makes little sense to me. Are you really improving encapsulation

by exposing this list as a Collection<T> instead of List<T>? I can
buy the arguement for a read only version, but we know that many
things within the framework (like databinding) would want to get to
the AddNew() methods.

If you really want to hide the implementation, I would say an argument
could be made for declare your property as IList<T> instead of the
concrete List<T>.

Yes, you can improve encapsulation by exposing a Collection<T>. Collection<T>
is designed for inheritance. It provides virtual methods that can be overridden
to have complete control over what will happen when items are added and removed.
Because of that, it *can* be safe to expose. If there is a need to expose
a list-type data structure that a client can manipulate (e.g. add and remove
items from), a Collection<T> descendent is probably the way to go. Granted,
I think the need to do that is extremely rare so I would normally opt for
a ReadOnlyCollection<T>. Besides, there's methods on List<T> and Array to
produce a ReadOnlyCollection<T> so it's easy.

As for IList<T>, if you expose that, you are really only protecting one thing:
the type of data structure that you used internally. While it's true that
you could change the type of data structure that you are using later, you
have exposed an interface that allows clients to manipulate the state of
your class in a way that the class doesn't have any control over it. IList<T>
has Add and Remove methods that won't notify your class when called. So,
you're still breaking encapsulation by giving clients direct access to your
class's private parts. Just because you use an interface doesn't mean it's
OK.

Best Regards,
Dustin Campbell
Developer Express Inc.