Implementing NotifyPropertyChanged without magic strings [duplicate]

Question

Possible Duplicate:
typesafe NotifyPropertyChanged using linq expressions

I'm working on a large team application which is suffering from heavy use of magic strings in the form of NotifyPropertyChanged("PropertyName"), - the standard implementation when consulting Microsoft. We're also suffering from a great number of misnamed properties (working with an object model for a computation module that has hundreds of stored calculated properties) - all of which are bound to the UI.

My team experiences many bugs related to property name changes leading to incorrect magic strings and breaking bindings. I wish to solve the problem by implementing property changed notifications without using magic strings. The only solutions I've found for .Net 3.5 involve lambda expressions. (for example: Implementing INotifyPropertyChanged - does a better way exist?)

My manager is extremely worried about the performance cost of switching from

set { ... OnPropertyChanged("PropertyName"); }

to

set {  ... OnPropertyChanged(() => PropertyName); }

where the name is extracted from

protected virtual void OnPropertyChanged<T>(Expression<Func<T>> selectorExpression)
{
    MemberExpression body = selectorExpression.Body as MemberExpression;
    if (body == null) throw new ArgumentException("The body must be a member expression");
    OnPropertyChanged(body.Member.Name);
}

Consider an application like a spreadsheet where when a parameter changes, approximately a hundred values are recalculated and updated on the UI in real-time. Is making this change so expensive that it will impact the responsiveness of the UI? I can't even justify testing this change right now because it would take about 2 days worth of updating property setters in various projects and classes.

Speiss

I use reflection for this. See my blog post here on this. http://tsells.wordpress.com/2011/02/08/using-reflection-with-wpf-and-the-inotifypropertychanged-interface/ Pay close attention to the performance note at the bottom of the post. – Russell

Good article, but you gave an absolute difference in performance time, but that is not useful. I'd be much more interested in the percentage difference in performance time. There's a huge difference between going from 200 ms to 300 ms, and going from 0.01 ms to 100.01 ms. Same absolute difference, different percentage difference. – Speiss

He said the difference was about 1/4 second for 10,000 property change notifications. I don't think that's a big enough difference to care about, and if you really are updating over 10k properties at once I would seriously reconsider the design :) – Uninterested

@Kev I'm not sure what you're referring to. That seems like an odd comment to leave on a question. The only link in the question is a link to another SO answer. There are no links in the accepted answer. The article linked to by tsells is irrelevant as it does not answer the question. There really isn't any risk of link rot anywhere relevant on this page. – Speiss

@Speiss - apologies, that comment got carried into here from an answer that was link only. – Suppositive

This is a fairly old question but for future readers: C# nowadays has features like nameof(Property) or the attribute [CallerMemberName] which allow you to pass the name of a property without using a magic string. – Barros

Answer 1

I did a thorough test of NotifyPropertyChanged to establish the impact of switching to the lambda expressions.

Here were my test results:

As you can see, using the lambda expression is roughly 5 times slower than the plain hard-coded string property change implementation, but users shouldn't fret, because even then it's capable of pumping out a hundred thousand property changes per second on my not so special work computer. As such, the benefit gained from no longer having to hard-code strings and being able to have one-line setters that take care of all your business far outweighs the performance cost to me.

Test 1 used the standard setter implementation, with a check to see that the property had actually changed:

    public UInt64 TestValue1
    {
        get { return testValue1; }
        set
        {
            if (value != testValue1)
            {
                testValue1 = value;
                InvokePropertyChanged("TestValue1");
            }
        }
    }

Test 2 was very similar, with the addition of a feature allowing the event to track the old value and the new value. Because this features was going to be implicit in my new base setter method, I wanted to see how much of the new overhead was due to that feature:

    public UInt64 TestValue2
    {
        get { return testValue2; }
        set
        {
            if (value != testValue2)
            {
                UInt64 temp = testValue2;
                testValue2 = value;
                InvokePropertyChanged("TestValue2", temp, testValue2);
            }
        }
    }

Test 3 was where the rubber met the road, and I get to show off this new beautiful syntax for performing all observable property actions in one line:

    public UInt64 TestValue3
    {
        get { return testValue3; }
        set { SetNotifyingProperty(() => TestValue3, ref testValue3, value); }
    }

Implementation

In my BindingObjectBase class, which all ViewModels end up inheriting, lies the implementation driving the new feature. I've stripped out the error handling so the meat of the function is clear:

protected void SetNotifyingProperty<T>(Expression<Func<T>> expression, ref T field, T value)
{
    if (field == null || !field.Equals(value))
    {
        T oldValue = field;
        field = value;
        OnPropertyChanged(this, new PropertyChangedExtendedEventArgs<T>(GetPropertyName(expression), oldValue, value));
    }
}
protected string GetPropertyName<T>(Expression<Func<T>> expression)
{
    MemberExpression memberExpression = (MemberExpression)expression.Body;
    return memberExpression.Member.Name;
}

All three methods meet at the OnPropertyChanged routine, which is still the standard:

public virtual void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
    PropertyChangedEventHandler handler = PropertyChanged;
    if (handler != null)
        handler(sender, e);
}

Bonus

If anyone's curious, the PropertyChangedExtendedEventArgs is something I just came up with to extend the standard PropertyChangedEventArgs, so an instance of the extension can always be in place of the base. It leverages knowledge of the old value when a property is changed using SetNotifyingProperty, and makes this information available to the handler.

public class PropertyChangedExtendedEventArgs<T> : PropertyChangedEventArgs
{
    public virtual T OldValue { get; private set; }
    public virtual T NewValue { get; private set; }

    public PropertyChangedExtendedEventArgs(string propertyName, T oldValue, T newValue)
        : base(propertyName)
    {
        OldValue = oldValue;
        NewValue = newValue;
    }
}

Speiss

Interesting, I ran a similar test on my machine a while ago (2013) and could not measure any performance difference whatsoever (.NET 3.5 SP1, Win7 x64, Core i7) – Larrabee

@Speiss - could be improved to even nicer version using the new [CallerMemberName] attribute - SetNotifyingProperty(ref storage, value); – Yam

Yeah I think that this was a common enough pattern that MS has been working on more efficient ways to get property names in the latest .NET versions. I'm going to suggest that this is as good as it gets just for versions 3.5 or 4.0. – Speiss

Re: "it's capable of pumping out a hundred thousand property changes per second". I agree that this is likely more than enough. But just for the record, note that expressing the speed of your implementation in terms of a one-second time window might not be very meaningful: INotifyPropertyChanged is mostly used for UI code, where you want to keep noticeable latencies to a minimum; and human reaction time is roughly 1/10th of a second. So an implementation has to do sufficiently well in a 50–100 millisecond time window. – Saveall

Answer 2

Personally I like to use Microsoft PRISM's NotificationObject for this reason, and I would guess that their code is reasonably optimized since it's created by Microsoft.

It allows me to use code such as RaisePropertyChanged(() => this.Value);, in addition to keeping the "Magic Strings" so you don't break any existing code.

If I look at their code with Reflector, their implementation can be recreated with the code below

public class ViewModelBase : INotifyPropertyChanged
{
    // Fields
    private PropertyChangedEventHandler propertyChanged;

    // Events
    public event PropertyChangedEventHandler PropertyChanged
    {
        add
        {
            PropertyChangedEventHandler handler2;
            PropertyChangedEventHandler propertyChanged = this.propertyChanged;
            do
            {
                handler2 = propertyChanged;
                PropertyChangedEventHandler handler3 = (PropertyChangedEventHandler)Delegate.Combine(handler2, value);
                propertyChanged = Interlocked.CompareExchange<PropertyChangedEventHandler>(ref this.propertyChanged, handler3, handler2);
            }
            while (propertyChanged != handler2);
        }
        remove
        {
            PropertyChangedEventHandler handler2;
            PropertyChangedEventHandler propertyChanged = this.propertyChanged;
            do
            {
                handler2 = propertyChanged;
                PropertyChangedEventHandler handler3 = (PropertyChangedEventHandler)Delegate.Remove(handler2, value);
                propertyChanged = Interlocked.CompareExchange<PropertyChangedEventHandler>(ref this.propertyChanged, handler3, handler2);
            }
            while (propertyChanged != handler2);
        }
    }

    protected void RaisePropertyChanged(params string[] propertyNames)
    {
        if (propertyNames == null)
        {
            throw new ArgumentNullException("propertyNames");
        }
        foreach (string str in propertyNames)
        {
            this.RaisePropertyChanged(str);
        }
    }

    protected void RaisePropertyChanged<T>(Expression<Func<T>> propertyExpression)
    {
        string propertyName = PropertySupport.ExtractPropertyName<T>(propertyExpression);
        this.RaisePropertyChanged(propertyName);
    }

    protected virtual void RaisePropertyChanged(string propertyName)
    {
        PropertyChangedEventHandler propertyChanged = this.propertyChanged;
        if (propertyChanged != null)
        {
            propertyChanged(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}

public static class PropertySupport
{
    // Methods
    public static string ExtractPropertyName<T>(Expression<Func<T>> propertyExpression)
    {
        if (propertyExpression == null)
        {
            throw new ArgumentNullException("propertyExpression");
        }
        MemberExpression body = propertyExpression.Body as MemberExpression;
        if (body == null)
        {
            throw new ArgumentException("propertyExpression");
        }
        PropertyInfo member = body.Member as PropertyInfo;
        if (member == null)
        {
            throw new ArgumentException("propertyExpression");
        }
        if (member.GetGetMethod(true).IsStatic)
        {
            throw new ArgumentException("propertyExpression");
        }
        return body.Member.Name;
    }
}

Uninterested

What in the world is PRISM doing with the PropertyChanged event add/remove methods? Anyways, it looks like they're effectively doing exactly what I have above: Expression body.Member.Name. Doesn't help with the performance question though. PRISM obviously isn't performance optimized - if it were they would have reused their defined variable 'member' rather than calling "body.Member" a second time at the bottom of their ExtractPropertyName method. – Speiss

Some would argue that saying "Code xxx is as optimized as possible because it is by Microsoft" is an oxymoron. I don't think that is true, but the truth does lie somewhere in between. – Starchy

@Speiss It could have also been reflected incorrectly. I had to make some minor tweaks to the code to get it to compile. It would probably be easy to make a change like this to your base class, run the performance test w/ your Magic Strings in, then do a find/replace using Regular Expressions to replace the PropertyChange calls with the new lambda and re-run your performance tests. – Uninterested

And I agree, I don't like saying it's as "optimized as possible" just because it was created by Microsoft. I modified the text a bit to say it should be "reasonably optimized" – Uninterested

@Alain: note that Rachel posted decompiled code. If you looked at the original source code, you might not see the these events implemented manually (add, remove). This might all be the compiler's work. If you took Reflector from 2011 and looked at your own events, you might see much of the same. – Saveall

Answer 3

If you're concerned that the lambda-expression-tree solution might be too slow, then profile it and find out. I suspect the time spent cracking open the expression tree would be quite a bit smaller than the amount of time the UI will spend refreshing in response.

If you find that it is too slow, and you need to use literal strings to meet your performance criteria, then here's one approach I've seen:

Create a base class that implements INotifyPropertyChanged, and give it a RaisePropertyChanged method. That method checks whether the event is null, creates the PropertyChangedEventArgs, and fires the event -- all the usual stuff.

But the method also contains some extra diagnostics -- it does some Reflection to make sure that the class really does have a property with that name. If the property doesn't exist, it throws an exception. If the property does exist, then it memoizes that result (e.g. by adding the property name to a static HashSet<string>), so it doesn't have to do the Reflection check again.

And there you go: your automated tests will start failing as soon as you rename a property but fail to update the magic string. (I'm assuming you have automated tests for your ViewModels, since that's the main reason to use MVVM.)

If you don't want to fail quite as noisily in production, you could put the extra diagnostic code inside #if DEBUG.

Leer

Good suggestion in case the lambda-expression-tree doesn't pan out. – Speiss

Answer 4

Actually we discussed this aswell for our projects and talked alot about the pros and cons. In the end, we decided to keep the regular method but used a field for it.

public class MyModel
{
    public const string ValueProperty = "Value";

    public int Value
    {
        get{return mValue;}
        set{mValue = value; RaisePropertyChanged(ValueProperty);
    }
}

This helps when refactoring, keeps our performance and is especially helpful when we use PropertyChangedEventManager, where we would need the hardcoded strings again.

public bool ReceiveWeakEvent(Type managerType, object sender, System.EventArgs e)
{
    if(managerType == typeof(PropertyChangedEventManager))
    {
        var args = e as PropertyChangedEventArgs;
        if(sender == model)
        {
            if (args.PropertyName == MyModel.ValueProperty)
            {

            }

            return true;
        }
    }
}

Anonym

Oh come, when you downvote at least say why. My answer is a valid alternative to the problem. Using Hardcoded strings against Lambda magic, which might hurt the performance. My solution doesn't hurt the performance and is easier to maintain than hardcoded strings. – Anonym

I didn't downvote, but this isn't helpful. It still involves hard coded strings. It's worse, in fact, because if I have 100 properties, it requires 100 new strings to store those names, and now when I change the name of that property, I have to change of name, the hard coded string, and the name of the hard coded string. – Speiss

For us the renaming was never a concern, maybe because we use resharper and we are a bit spoilt because of it. Second of all, our models are generated, thus all the properties can easly get a identifier. Last but not least, the performance was for us the most important part, of course it is not as nice as the lambda thing but for us it works pretty well. Especially the part about the WeakEventManager. So in the end i thought a different approach might be at least helpful to consider and weight the pros and cons of the solutions at hand. But i respect if it isn't helpful for you. – Anonym

Even without ReSharper, renaming the identifier is trivial -- Visual Studio has a rename refactoring. (Of course, why would anyone want to code without ReSharper?) – Leer

Answer 5

One simple solution is to simply pre-process all files before compilation, detect the OnPropertyChanged calls that are defined in set { ... } blocks, determine the property name and fix the name parameter accordingly.

You could do this using an ad-hoc tool (that would be my recommendation), or use a real C# (or VB.NET) parser (like those which can be found here: Parser for C#).

I think it's reasonable way to do it. Of course, it's not very elegant nor smart, but it has zero runtime impact, and follows Microsoft rules.

If you want to save some compile time, you could have both ways using compilation directives, like this:

set
{
#if DEBUG // smart and fast compile way
   OnPropertyChanged(() => PropertyName);
#else // dumb but efficient way
   OnPropertyChanged("MyProp"); // this will be fixed by buid process
#endif
}

Rex