Is it OK for property setters to have side effects with the MVVM pattern

Question

I'm writing WPF app which intend to solve standard problems. I'm really new to WPF and MVVM pattern, so there's a little mess in my head after reading tons of different approaches to MVVM on the internet. I'd like to know how my simple operation of DataGrid's itemsource refresh operation is "idiomatic" to MVVM.

Let's say I have one datagrid and one combobox. The combo contains list of all coaches. The datagrid shows you all sportsmen trained by selected coach so the combo acts like a filter for data in datagrid:

<ComboBox ItemsSource="{Binding ListCoach}" DisplayMemberPath="last_name" SelectedValue=
"{Binding SelectedCoach}" SelectedValuePath="Id"/>

<DataGrid ItemsSource="{Binding Path=ListSportsman}" .....  </DataGrid>

My ViewModel class change content of the DataGrid in the setter of SelectedCoach property (this property is a target for Combobox's value):

  private int _selectedCoach;
  public int SelectedCoach
  {
     get { return _selectedCoach; }
     set
     {
        _selectedCoach = value;
        ListSportsman = new ObservableCollection<sportsmanset>(_serviceAgent.ListSportsmanOfCoach(value));
        NotifyPropertyChanged(vm => vm.SelectedCoach);
     }
  }

Doesn't such code smell? Or It would be more appropriate to subscribe for change in SelectedCoach property and set ListSportsman in separate function? (by the way, how to subscribe for NotifyPropertyChanged event manually?)

Answer 1

It is not wrong by definition but there is one thing to consider:

In general developers expect setters and getters to be fast. So if you are adding logic that takes a considerable amount of time, you might want to execute that logic asynchronously or replace the property with a Set method so it is clear that there is processing involved.

Part of the processing could be setting a property that the View could bind to.

Answer 2

My answer is "No it's not OK". Well for simple app/prototype it's fine, however for more advanced scenarios or to develop good habits / set example - it's not.

What to do instead? There's no "one size fits all answer" but I would recommend to, based on my experience:

1. always keep setter dumb i.e. only set value and NotifyPropertyChanged.
2. try to encapsulate expensive business logic & other side effects where it supposed to be - in commands. If it's impossible and it has to be triggered by setter, then create observable Rx stream from PropertyChanged and subscribe to it. It may sound like overkill but benefits of Rx for MVVM are enormous. Here's a possible (not necesserily the best) way to do it: http://blog.leifbattermann.de/2015/06/21/viewmodel-property-to-observable-stream/
3. (offtopic but not really) also do not raise change of simple dependent read-only properties inside setter, instead use https://github.com/StephenCleary/CalculatedProperties or similar (I doubt there's anything better at the moment)

Answer 3

No, this code does not 'smell'!

A view-model is very closely associated with the view that it 'backs'. In the view you describe, these two properties are very closely coupled, therefore it would make sense that they are tightly coupled in your view model.

Also, just ask yourself, what benefit would it give to have the logic, that populates the data grid, loosely coupled by event handling? This could allow you to more easily execute this logic as the result of some other event, or perhaps separate into two classes for re-use elsewhere in your code. Are any of these scenarios likely? If not, making your code more complex for no reason is a code smell in itself!

By the way if you do want to handle this manually, you need to add an event handler to the classes PropertyChanged event.