WPF excessive PropertyChanged events

Question

Typically in the property setter of an object we may want to raise a PropertyChanged event such as,

    public event PropertyChangedEventHandler PropertyChanged; 
    protected void Notify(string property)
    {
        if (PropertyChanged != null)
        {
            PropertyChanged(this, new PropertyChangedEventArgs(property));
        }
    }

    public string UserNote
    {
        get { return _userNote; }
        set
        {
            _userNote = value;
            Notify("UserNote"); 
        }
    }

In our existing code base I see instances where PropertyChangedEventArgs is being sent null in order to indicate that all properties of the object have changed. This seems inefficient and seems to lead to far more events being triggered than is needed. It also seems to causes issues where objects update each other in a circular fashion.

Is this ever a good practice?

A comment in the code tries to justify it ...

//The purpose of this method is to wire up clients of NotificationBase that are also
//NotificationBases to *their* clients. Consider the following classes:     
         public class ClassA : NotificationBase
         {
             public int Foo
             {
                 get { return 123; }
                 set { Notify("Foo"); }
             }
         }

         public class ClassB : NotificationBase
         {
             ClassA A = new ClassA();
             public ClassB()
             {
                 A.PropertyChanged += AllChanged;
             }
             public void SetFoo()
             {
                 A.Foo = 456;
             }
         }

         public class ClassC
         {
             ClassB B = new ClassB();
             public ClassC()
             {
                 B.PropertyChanged += delegate { dosomething(); };
                 B.SetFoo(); // causes "dosomething" above to be called
             }
         }

        /// ClassB.SetFoo calls ClassA.Foo's setter, which calls ClassA.Notify("Foo").
        /// The event registration in ClassB's ctor causes ClassB.AllChanged to be called, which calls
        /// ClassB.Notify(null) - implying that ALL of ClassB's properties have changed.
        /// The event registration in ClassC's ctor causes the "dosomething" delegate to be called.
        /// So a Notify in ClassA is routed to ClassC via ClassB's PropertyChanged event.

        protected void AllChanged(Object sender, PropertyChangedEventArgs e)
        {
            Notify(null);
        }

Any thoughts much appreciated.

Regards, Fzzy

Answer 1

Ignoring the other stuff, I'd say the Notify(null) alone is a bad practice. It's not inherently clear what that means, and to a developer working the code 5 years down the line would probably assume that it meant something else unless they happened upon the comments.

Answer 2

I have come across situations wherein computed properties (without setters) need to fire PropertyChangeNotification when some other property i set via a setter.

eg

double Number { get { return num;} set { num=value; OnPropertyChanged("Number"); OnPropertyChanged("TwiceNumber"); } }

double TwiceNumber { get {return _num * 2.0;} }

As a rule I only do it with get only properties and I don't see why in this case a property firing a change notification on the other is bad. But I think if I do it for any other case I most likely don't know what I am doing!

Answer 3

This is actually a problem with the design (or its documentation) of PropertyChangedEventArgs. Setting PropertyName to null means "all properties on this object have changed." But unless the class is sealed, or you're using reflection, you can't actually know that all properties on the object have changed. The most you can say is that all of the properties in the object's base class have changed.

This is reason enough to not use this particular convention, in my book, except in the vanishingly small number of cases where I create sealed classes that implement property-change notification.

As a practical matter, what you're really trying to do is just raise one event that tells listeners "a whole bunch of properties on this object have changed, but I'm not going to bother to tell you about them one by one." When you say:

I see instances where PropertyChangedEventArgs is being sent null in order to indicate that all properties of the object have changed. This seems inefficient and seems to lead to far more events being triggered than is needed.

...the actual intent is the exact opposite. If a method changes the Foo, Bar, Baz, and Bat properties on an object, and the object has only four or five properties, raising one event is probably better than raising four. On the other hand, if the object has sixty properties, raising four events is probably better making every one of the object's listeners - even those that aren't looking at those four properties - do whatever they do when the properties that they care about change, because those properties didn't.

The problem is that the property-change notification system, as designed, isn't a fine-grained enough tool for every single job. It's designed to be completely generic, and has no knowledge of a particular application domain built into it.

And that, it seems to me, is what's missing from your design: application domain knowledge.

In your second example, if a Fixture object has (say) ten properties that depend on the value of FixtureStatus, raising ten property-change events may seem a little excessive. Maybe it is. Maybe the object should raise a FixtureStatusChanged event instead. Then classes with knowledge of your application domain can listen to this one event and ignore the PropertyChanged event. (You still raise the PropertyChanged event on the other properties, so that objects that don't know what a FixtureStatusChanged event means can stay current - that is, if it's still necessary for your class to implement INotifyPropertyChanged once you've implemented FixtureStatusChanged.)

A secondary comment: Most classes in the C# universe, if they implement a method that raises the Foo event, call that method OnFoo. This is an important convention: it makes the relationship between the method and the event explicit, and it makes the fact that the code that's calling the method is raising an event easy to recognize. Notify is a weak name for a method in general - notify who? of what? - and in this case it actually obfuscates something that should be made explicit. Property-change notification is tricky enough without your naming convention concealing the fact that it's happening.