By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
432,275 Members | 936 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 432,275 IT Pros & Developers. It's quick & easy.

Exposing Generic Lists

P: n/a
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<Tis 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<TKe y, TItem"

Our property is not "virtual" and thus cannot be overridden. We used
List<Tbecasue 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<tproperty
is bad? Why should I care is List<Thas any virtual members when I am
simply using an instance of and _not_ inheriting from List<T>?

Thanks,
--BJ
Feb 19 '07 #1
Share this Question
Share on Google+
4 Replies


P: n/a
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<Tis 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<TKe y, TItem"
Our property is not "virtual" and thus cannot be overridden. We used
List<Tbecasue 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<Thas any virtual members
when I am simply using an instance of and _not_ inheriting from
List<T>?
List<Tis intended to be a workhorse class used in implementation. By exposing
a List<Tin 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<Employeem_Employees;

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

public List<EmployeeEmployees { get { return m_Employees; } }
}

Any client code of the EmployeeCensus class could easily corrupt its state
by calling methods on the List<Employeeexposed 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<Tbut to expose a Collection<T>,
ReadOnlyCollection<Tor KeyedCollection<TKey, TItemlike this:

public class EmployeeCensus
{
private List<Employeem_Employees;

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

public ReadOnlyCollection<EmployeeEmployees { 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.
Feb 19 '07 #2

P: n/a


"BJ Safdie" <MS***********@nospam.nospamwrote in message
news:1D**********************************@microsof t.com...
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<Tis 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<TKe y, TItem"

Our property is not "virtual" and thus cannot be overridden. We used
List<Tbecasue 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<Thas any virtual members when I am
simply using an instance of and _not_ inheriting from List<T>?
List<Tis 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<Tto IList<T>.

David

Feb 19 '07 #3

P: n/a
Dustin,

This makes little sense to me. Are you really improving encapsulation by
exposing this list as a Collection<Tinstead 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" wrote:
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<Tis 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<TKe y, TItem"
Our property is not "virtual" and thus cannot be overridden. We used
List<Tbecasue 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<Thas any virtual members
when I am simply using an instance of and _not_ inheriting from
List<T>?

List<Tis intended to be a workhorse class used in implementation. By exposing
a List<Tin 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<Employeem_Employees;

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

public List<EmployeeEmployees { get { return m_Employees; } }
}

Any client code of the EmployeeCensus class could easily corrupt its state
by calling methods on the List<Employeeexposed 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<Tbut to expose a Collection<T>,
ReadOnlyCollection<Tor KeyedCollection<TKey, TItemlike this:

public class EmployeeCensus
{
private List<Employeem_Employees;

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

public ReadOnlyCollection<EmployeeEmployees { 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.
Feb 19 '07 #4

P: n/a
This makes little sense to me. Are you really improving encapsulation
by exposing this list as a Collection<Tinstead 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<Tdescendent 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<Tand Array to
produce a ReadOnlyCollection<Tso 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.
Feb 20 '07 #5

This discussion thread is closed

Replies have been disabled for this discussion.