Self-subscribe to PropertyChanged or addition method call in setter?

Question

Maybe here is already such question, but I didn't find it.

I have MVVM application, and in my ViewModel I have to do some additional actions on changes of some properties (for example, if View changes them). Which approach is better on your mind and why?

1st - Add AdditionalAction call to setter

public class ViewModel: INotifyPropertyChanged
{
  private int _MyProperty;

  public int MyProperty
  {
    get { return _MyProperty; }
    set
    {
      if (_MyProperty == value) return;
      _MyProperty = value;
      RaisePropertyChanged(() => MyProperty);

      // --- ADDITIONAL CODE ---
      AdditionalAction();
    }
  }
}

2nd - Self-subscribe to INotifyPropertyChanged

public class ViewModel: INotifyPropertyChanged
{
  public ViewModel()
  {
    // --- ADDITIONAL CODE ---
    PropertyChanged += OnPropertyChanged;
  }

  private int _MyProperty;

  public int MyProperty
  {
    get { return _MyProperty; }
    set
    {
      if (_MyProperty == value) return;
      _MyProperty = value;
      RaisePropertyChanged(() => MyProperty);
    }
  }

  void PropertyChanged(object sender, PropertyChangedEventArgs e)
  {
    // --- ADDITIONAL CODE ---
    if (e.PropertyName == "MyProperty")
      AdditionalAction();
  }
}

Imagine, that I don't have performance problem or 10'000 objects. It's just View and ViewModel. What is better? First code is "smaller" and has less overhead, but the second (on my mind) is more clear and I can use code snippets for auto-generation properties' code. Even more - in the 2nd case I can write in event handler something like:

On.PropertyChanged(e, p => p.MyProperty, AdditionalAction);

where On is class-helper.

So, what is better on your mind and why?

UPDATED:

OK, it looks like I found yet one approach:

3rd - add "extension point" in RaisePropertyChanged:

public class NotificationObject : INotifyPropertyChanged
{
  void RaisePropertyChanged(Expression<...> property)
  {
    // ... Raise PropertyChanged event
    if (PropertyChanged != null)
      // blah-blah

    // Call extension point
    OnPropertyChanged(property.Name);
  }

  public virtual OnPropertyChanged(string propertyName)
  {
  }
}

public class ViewModel: NotificationObject
{
  private int _MyProperty;

  public int MyProperty
  {
    get { return _MyProperty; }
    set
    {
      if (_MyProperty == value) return;
      _MyProperty = value;
      RaisePropertyChanged(() => MyProperty);
    }
  }

  override OnPropertyChanged(string propertyName)
  {
    if (propertyName == "MyProperty")
      AdditionalAction();
  }
}

This way we don't use event, but all "additional actions" are called from the same "extension point". Is "one place for all addition actions" better than "not transparent workflow"?

http://tergiver.wordpress.com/2011/01/20/self-subscription-is-asinine/ — Tergiver, May 04 '12 at 14:47
sidenote: consider using a helper method to put the three lines in your properties's setter to one, returning a boolean if the property did change. Shorter and no duplication. eg `if( RaisePropertyChanged( ref _MyProperty, value, o => o.MyProperty ) ) AdditionalAction();` — stijn, May 04 '12 at 14:54
@Tergiver what is the differences between your way and my 2nd way? They are the same - you write additional code not in the "setter" but in the "event handler" - `OnXXX` method from this point of view is the same as self-subscription to event. So, from your point of view my question is - "Is it better to call `AdditionalAction` from setter or from OnPropertyChanged method?" (even if there is not method OnPropertyChanged actually) — chopikadze, May 04 '12 at 14:55
@chopikadze The difference is that in your second example, you are using your own multicast delegate for the purpose of executing your own code. Events are for external entities, not for one's self. There's nothing "wrong" with it per-say, it's just non-sensible. — Tergiver, May 04 '12 at 15:00
@chopikadze It will work, though I'm not sure what advantage you get by adding the string comparisons (or better yet a switch statement), over adding the specific code in the setter which is your first example. If you need the derived code to perform this task as in your third example, then you might do that, but why not add virtual OnMyProperty1, OnMyProperty2, etc? — Tergiver, May 04 '12 at 15:44
@Tergiver about 1st - I want to have separation between "properties" and "additional actions". I want to keep properties body as clean as possible - usually I wrap it with `#region`, so I look at such properties as "auto-properties". And I prefer to have some separate place where I can see all "additional actions". About 2nd - if I have property which is subscribed by a lot of clients (e.g. `Server.IsConnected`), I add even separate event - `OnIsConnectedChanged`. But if I have class with 20-30-40 properties? 20-30-40 additional methods, most of which will not be used? Arrr, I don't like it.. — chopikadze, May 05 '12 at 05:31
The main benefit (imho) of "single place for all additional actions" - I don't have to go through code of all my 40 properties (and this code isn't single line..) to know which additional actions class does after its properties' changes. If my class has 2 properties - no problem. But when it has some tens of properties - it's hard to go over that code. — chopikadze, May 05 '12 at 05:32

score 3 · Accepted Answer · edited May 04 '12 at 15:09

3

I would definitely go for the first method:

it's clear
it's explicit in its flow and intention
it avoids weird (imo) self subscription

The "benefits" of second, which lets you use autogenerated properties is not worth the clearness of the execution flow of the firs case, imo.

Hope this helps.

edited May 04 '12 at 15:09

BrokenGlass

158,293
28
286
335

answered May 04 '12 at 14:58

Tigran

61,654
8
86
123

what do you think about 3rd way? – chopikadze May 04 '12 at 15:29
@chopikadze: seems good, cause it *could* support refactoring of the code, if you get rid of the string:propertyName in some way. – Tigran May 04 '12 at 15:34

score 3 · Answer 2 · answered May 04 '12 at 15:53

Here is the "usual" pattern. This allows you to put property-specific code inside the OnX method, and allows derived classes to do the same. No need for a big switch statement, unless of course you're the external listener, but that is par for the course for INotifyPropertyChanged.

public class NotificationObject : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
    protected void FirePropertyChanged(PropertyChangedEventArgs e)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null)
            handler(this, e);
    }
}

public class ViewModel : NotificationObject
{
    private int _MyProperty1;
    public int MyProperty1
    {
        get { return _MyProperty1; }
        set
        {
            if (value != _MyProperty1)
            {
                _MyProperty1 = value;
                OnMyProperty1Changed(new PropertyChangedEventArgs("MyProperty1"));
            }
        }
    }

    protected virtual void OnMyProperty1Changed(PropertyChangedEventArgs e)
    {
        FirePropertyChanged(e);
    }

    private int _MyProperty2;
    public int MyProperty2
    {
        get { return _MyProperty2; }
        set
        {
            if (value != _MyProperty2)
            {
                _MyProperty2 = value;
                OnMyProperty2Changed(new PropertyChangedEventArgs("MyProperty2"));
            }
        }
    }

    protected virtual void OnMyProperty2Changed(PropertyChangedEventArgs e)
    {
        FirePropertyChanged(e);
    }
}

Thanks a lot for your help, I marked answer of @Tigran just because it has more votes, but I really appreciate our discussion! — chopikadze, Aug 14 '12 at 06:48

Self-subscribe to PropertyChanged or addition method call in setter?

2 Answers2

Linked

Related