CODEWITHSUNDEEP
c#mvvm
Goran
Goran

Reputation: 6518

Subscribing to PropertyChanged on current item in ViewModel

Is it bad practice to subsribe to PropertyChanged event of the currently selected entity. Lets say that I have a grid that is bound to List and I have binded Grid's SelectedItem to ViewModel's SelectedItem property.

private Customer _selectedItem;
public Customer SelectedItem
{
    get {return _selectedItem;}
    set
    {
        if (!ReferenceEquals(_selectedItem, value))
        {
            _selectedItem = value;
            RaisePropertyChanged(()=>SeletedItem);
            _selectedItem.PropertyChanged += OnCustomerPropertyChanged;
        }
    }
}

Does this code has any drawbacks, performance wise, memory-leak wise, or it is safe to use it like this?

Upvotes: 1

Views: 477

Answers (1)

Chris Shain
Chris Shain

Reputation: 51329

You are leaking event subscriptions. There is nothing wrong with the idea of binding to SelectedItem, but you need to remember to unsubscribe later. You also should be null-checking your value before subscribing and unsubscribing:

private Customer _selectedItem;
public Customer SelectedItem
{
    get {return _selectedItem;}
    set
    {
        if (!ReferenceEquals(_selectedItem, value))
        {
            if (!ReferenceEquals(null, _selectedItem))
                _selectedItem.PropertyChanged -= OnCustomerPropertyChanged;
            _selectedItem = value;
            RaisePropertyChanged(()=>SelectedItem);
            if (!ReferenceEquals(null, _selectedItem))
                _selectedItem.PropertyChanged += OnCustomerPropertyChanged;
        }
    }
}

Otherwise, as you go through selecting item after item, all of your previously selected items will have their PropertyChanged event handlers hooked to OnCustomerPropertyChanged. That in turn could lead to memory leaks (because event handlers can prevent the event-raising object from being GC'd) or unexpected behavior.

Upvotes: 3

Related Questions