« Subtle Differences | Main | Creating Mixins with T4 in Visual Studio »
March 17, 2009
ReadOnlyObservableCollection anti-pattern
I recently fixed a bug that was a result of a subtle misuse of the ReadOnlyObservableCollection<T> class. The code in question was structured as follows: The Source class has a collection of Items. Clients should be able to observe but not modify the collection, so it’s exposed as a ReadOnlyObservableCollection. The Client class adds an event handler to this collection, and removes it at the appropriate time (to prevent a possible memory leak).
class Source
{
Source()
{
m_collection = new ObservableCollection<int>();
}
public ReadOnlyObservableCollection<int> Items
{
get
{
// this is the bug
return new ReadOnlyObservableCollection<int>(m_collection);
}
}
readonly ObservableCollection<int> m_collection;
}
class Client
{
// obtain Source instance from somewhere
Source m_source;
void Subscribe()
{
((INotifyCollectionChanged) m_source.Items).CollectionChanged += SourceItems_CollectionChanged;
}
void Unsubscribe()
{
((INotifyCollectionChanged) m_source.Items).CollectionChanged += SourceItems_CollectionChanged;
}
void SourceItems_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) { }
}
The bug is that the Source.Items property creates a new ReadOnlyObservableCollection instance each time it’s accessed, which means that the calls to add and remove the event handler happen on two different objects; the event handler is never actually removed.
One solution is for the Client class to cache the exact INotifyCollectionChanged object it subscribed to, so it can be sure to unsubscribe from the same object. The other (and better) solution is for the Source class to create and hold a ReadOnlyObservableCollection, and return the same object each time the Items property is accessed:
public class Source
{
Source()
{
m_collection = new ObservableCollection<int>();
m_collectionReadOnly = new ReadOnlyObservableCollection<int>(m_collection);
}
public ReadOnlyObservableCollection<int> Items
{
get { return m_collectionReadOnly; }
}
readonly ObservableCollection<int> m_collection;
readonly ReadOnlyObservableCollection<int> m_collectionReadOnly;
}
This solves the initial bug and also has the benefit of allowing all clients to share the same ReadOnlyObservableCollection instance (instead of creating one per client).
Posted by Bradley Grainger at March 17, 2009 9:02 AM
Trackback Pings
TrackBack URL for this entry:
http://blog.logos.com/mt-cgi/mt-tb.cgi/279
Comments
Where is (anti)patten there?
Writing buggy code even in repetitive way, doesn't mean that it is (anti)pattern.
Of course if "anti-pattern" is new cool, funky buzz word that "everyone" should use to be trendy ;-)
Posted by: Wojciech Gebczyk at March 17, 2009 11:02 AM
I agree that it's much smaller in scope than any of the example anti-patterns. However, I do think that it is a structure that "initially appears to be beneficial, but ultimately produces more bad consequences than beneficial results" and has "a refactored solution that is clearly documented, proven in actual practice and repeatable." Additionally, a similar code pattern that uses ReadOnlyCollection (not ReadOnlyObservableCollection) works just fine; this seemed to be the "anti-pattern" to that "pattern".
But yes, "anti-pattern" is a bit of a buzzword and putting it in the title is a lot more attention-grabbing than just "bug".
Posted by: Bradley Grainger 
at March 17, 2009 11:11 AM