Interdependent properties

Dec 22, 2010 at 6:18 PM

As I’ve been developing applications using Caliburn Micro, I’ve found myself following the same pattern on a few occasions.  Namely: I have a property X which calls NotifyOfPropertyChange(()=>X) when changed.  I have property Y which does similar.  Property Z depends on properties X and Y and hence needs to be updated in the UI whenever X or Y changes.

Initially, I added NotifyOfPropertyChange(()=>Z) to the setter of both X and Y.

Following on from http://caliburnmicro.codeplex.com/Thread/View.aspx?ThreadId=225945 whenever I end up with a few of these dependent properties for one reason or another, I use NotifyOfPropertyChange(string.Empty) instead to force a reevaluation of all bindings.

Neither of these options have seemed particularly satisfying to me.  Firstly because Z depends on X and Y, so why should X and Y need to know about Z and prompt its change.  Secondly, the NotifyOfPropertyChange(string.Empty) option concerns me for potential performance reasons.

So, this afternoon, inspired by the PreviewAttribute pointed to by cshepherd on that previous thread, I came up with a possible way around this.  I’d appreciate any and all input as to whether there are any obvious flaws or routes for improvement.

First, I defined an attribute to allow me to declare that property Z depends on the value of X and Y.

[AttributeUsage(AttributeTargets.Property)]
public class NotifyOnChangeOfAttribute : Attribute
{
    public NotifyOnChangeOfAttribute(params string[] propertyNames)
    {
        PropertyNames = propertyNames ?? new string[] { };
    }
    public string[] PropertyNames { get; private set; }
}

This is used like:

[NotifyOnChangeOf("X", "Y")]
public object Z { get; }

The other piece of the puzzle is an override to NotifyOfPropertyChange that inspects the properties for the presence of this attribute and acts upon the reference.

 

public override void NotifyOfPropertyChange(string propertyName)
{
    base.NotifyOfPropertyChange(propertyName);

    // Iterate through all the properties on this object
    foreach (var propertyInfo in GetType().GetProperties())
    {
        string objectPropertyName = propertyInfo.Name;
        // If we've already checked the attributes on this one, continue to prevent 
        // an infinite loop in the event of cyclical references
        if (_propertiesAlreadyInspected.Contains(objectPropertyName)) continue;
        _propertiesAlreadyInspected.Add(objectPropertyName);

        // Pick up the NotifyOnChangeOfAttribute attribute and loop through the
        // referenced properties if any
        foreach (Attribute attr in Attribute.GetCustomAttributes(propertyInfo))
        {
            NotifyOnChangeOfAttribute objectPropertyNotifyAttribute = attr as NotifyOnChangeOfAttribute;
            if (objectPropertyNotifyAttribute != null)
                foreach (string dependentPropertyName in objectPropertyNotifyAttribute.PropertyNames)
                    // If the property we're inspecting lists the property that has changed as one of
                    // its dependent properties, also fire the property changed event for this other prop
                    if (dependentPropertyName == propertyName) NotifyOfPropertyChange(objectPropertyName);
        }
        _propertiesAlreadyInspected.Remove(objectPropertyName);
    }
}

 

I’ve built a simple example application, the solution for which can be downloaded from http://www.darranshepherd.co.uk/wp-content/uploads/2010/12/CaliburnLinkedNotificationProperties.zip

The different options for the ShellViewModel are:

Lots of NotifyOfPropertyChange calls

 

public class ShellViewModel : ExtendedNotifyPropertyChangedBase
{
    private double _firstNumber;
    private double _secondNumber;

    public double FirstNumber 
    {
        get { return _firstNumber; }
        set
        {
            _firstNumber = value;
            NotifyOfPropertyChange(() => FirstNumber);
            NotifyOfPropertyChange(() => AdditionResult);
            NotifyOfPropertyChange(() => MultiplicationResult);
            NotifyOfPropertyChange(() => PowerResult);
        }
    }
    public double SecondNumber
    {
        get { return _secondNumber; }
        set
        {
            _secondNumber = value;
            NotifyOfPropertyChange(() => SecondNumber);
            NotifyOfPropertyChange(() => AdditionResult);
            NotifyOfPropertyChange(() => MultiplicationResult);
            NotifyOfPropertyChange(() => PowerResult);
        }
    }

    public double AdditionResult { get { return FirstNumber + SecondNumber; } }
    public double MultiplicationResult { get { return FirstNumber * SecondNumber; } }
    public double PowerResult { get { return Math.Pow(FirstNumber,SecondNumber); } }
}

 

NotifyOfPropertyChange(string.Empty)

 

public class ShellViewModel : ExtendedNotifyPropertyChangedBase
{
    private double _firstNumber;
    private double _secondNumber;

    public double FirstNumber { get { return _firstNumber; } set { _firstNumber = value; NotifyOfPropertyChange(string.Empty); } }
    public double SecondNumber { get { return _secondNumber; } set { _secondNumber = value; NotifyOfPropertyChange(string.Empty); } }

    public double AdditionResult { get { return FirstNumber + SecondNumber; } }
    public double MultiplicationResult { get { return FirstNumber * SecondNumber; } }
    public double PowerResult { get { return Math.Pow(FirstNumber,SecondNumber); } }
}

 

This attributed option

public class ShellViewModel : ExtendedNotifyPropertyChangedBase
{
    private double _firstNumber;
    private double _secondNumber;

    public double FirstNumber { get { return _firstNumber; } set { _firstNumber = value; NotifyOfPropertyChange(() => FirstNumber); } }
    public double SecondNumber { get { return _secondNumber; } set { _secondNumber = value; NotifyOfPropertyChange(() => SecondNumber); } }

    [NotifyOnChangeOf("FirstNumber", "SecondNumber")]
    public double AdditionResult { get { return FirstNumber + SecondNumber; } }
    [NotifyOnChangeOf("FirstNumber", "SecondNumber")]
    public double MultiplicationResult { get { return FirstNumber * SecondNumber; } }
    [NotifyOnChangeOf("FirstNumber", "SecondNumber")]
    public double PowerResult { get { return Math.Pow(FirstNumber,SecondNumber); } }
}

Dec 22, 2010 at 7:56 PM
Edited Dec 23, 2010 at 11:03 AM

I think your approach is quite nice, since it relieves, as you say, the independant property from the burden of notifying about a change of the dependant one.

I would really like to see a benchmark on the speed of this approach compared to the standard one, just to have an idea of the performance hit (I dubt it will be a real issue, but knowing if the approach is 10% slower or 60% slower could be useful when speed is critical).

That said, I think that you can modify the NotifyOfPropertyChanged this way

 
public override void NotifyOfPropertyChange(string propertyName)
{
    base.NotifyOfPropertyChange(propertyName);

    // Iterate through all the properties on this object
    foreach (var propertyInfo in GetType().GetProperties())
    {
        string objectPropertyName = propertyInfo.Name;
        // If we've already checked the attributes on this one, continue to prevent 
        // an infinite loop in the event of cyclical references
        if (_propertiesAlreadyInspected.Contains(objectPropertyName)) continue;
        _propertiesAlreadyInspected.Add(objectPropertyName);

        // Pick up the NotifyOnChangeOfAttribute attributes
        foreach (NotifyOnChangeOfAttribute attr in propertyInfo.GetCustomAttributes(typeof(NotifyOnChangeOfAttribute, true))
        {
               // If the property we're inspecting lists the property that has changed as one of
               // its dependent properties, also fire the property changed event for this other prop
               if (attr.PropertyNames.Contains(propertyName))
               {
                      NotifyOfPropertyChange(objectPropertyName);
                      break; //Once notified there is no need to keep on checking other attributes...
        }
        _propertiesAlreadyInspected.Remove(objectPropertyName);
    }
}

Note that the inner inner loop has been substituted with a Contains check (more readeable in my opinion), and the attributes search is stopped once the first notification occurs. Moreover the inherited attributes are checked too, just in case the property is virtual and an override defines more dependencies.

Another aspect that could be improved is the fact that the _propertiesAlreadyInspected is (correct me if I am wrong) a private member of the class. Unfortunally, this means that a property change notification could be incorrectly skipped if two notifications are issued by concurrent threads (in the worst case it could lead to an exception). To avoid this, you can create a NotifyOfPropertyChangeCore function that takes the collection as a parameter (initialized in the NotifyOfPropertyChange function) and is used for recursion.

public override void NotifyOfPropertyChange(string propertyName)
{
    NotifyOfPropertyChangeCore(propertyName, new List<string>());
}

private void NotifyOfPropertyChangeCore(string propertyName, List<string> propertiesAlreadyInspected)
{
    base.NotifyOfPropertyChange(propertyName);

    // Iterate through all the properties on this object
    foreach (var propertyInfo in GetType().GetProperties())
    {
        string objectPropertyName = propertyInfo.Name;
        // If we've already checked the attributes on this one, continue to prevent 
        // an infinite loop in the event of cyclical references
        if (propertiesAlreadyInspected.Contains(objectPropertyName)) continue;
        propertiesAlreadyInspected.Add(objectPropertyName);

        // Pick up the NotifyOnChangeOfAttribute attributes
        foreach (NotifyOnChangeOfAttribute attr in propertyInfo.GetCustomAttributes(typeof(NotifyOnChangeOfAttribute, true))
        {
               // If the property we're inspecting lists the property that has changed as one of
               // its dependent properties, also fire the property changed event for this other prop
               if (attr.PropertyNames.Contains(propertyName))
               {
                      NotifyOfPropertyChangeCore(objectPropertyName, propertiesAlreadyInspected);
                      break; //Once notified there is no need to keep on checking other attributes...
        }
        propertiesAlreadyInspected.Remove(objectPropertyName);
    }
}

This way two notifications can run without affecting each other and the class can be considered thread safe respect to property change notification.

A final consideration is about obfuscation: is there a way to pass the name of the independant properties using a labda expression, instead of a string? At the cost of a performance hit, the code would be less error prone and usable in a heavily obfuscated scenario.

A final note: I  wrote the code outside the IDE  and did not even built it... I hope it is correct and I didn't do any major mistakes... --'

I hope you found my (long) comments useful!

Dec 23, 2010 at 10:45 AM

It's a very good idea!

Coordinator
Dec 23, 2010 at 1:54 PM

I highly recommend that you check this out http://code.google.com/p/notifypropertyweaver/

Dec 23, 2010 at 2:21 PM

Thanks Marco.

Interesting ideas, BladeWise.  Many thanks; the tweaks look good.  The concurrency issue with _propertiesAlreadyInspected did cross my mind, but I thought I'd deal with that another day.  Nice way around it.

I've been having a play to see how I could get some compiler checking of property names in place.  I'd have liked to have done it similar to the way the NotifyOfPropertyChange<TProperty>(Expression<Func<TProperty>> property) overload does it, but it seems that arguments passed to an Attribute have to be compile time constant which the expression tree parameter is not.

As an alternative, something encapsulated in an if #DEBUG in the constructor of the base class could do it, as shown below.  I'll keep thinking on that one and see if I can come up with anything better.  Maybe a generic NUnit test which inspects all the types implementing INotifyPropertyChanged

Whilst musing the performance issue, I came up with another idea which I've thrown in.  By having a [HasInterdependentProperties] attribute which gets declared on the class, I can limit the performance overhead of implementing this in a base class to the single check of Attribute.IsDefined(this.GetType(), typeof(HasInterdependentPropertiesAttribute), true).

As I'm currently using SVNs Vendor Branches to track the Caliburn Micro source and build it for our apps (CruiseControl.Net is my next Christmas task...) I'll add some unit tests and build a patch to add this and stick it up somewhere in case its of interest to anyone.

The code (updated demo project at http://www.darranshepherd.co.uk/wp-content/uploads/2010/12/CaliburnLinkedNotificationPropertiesV0.2.zip) now looks like:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Caliburn.Micro;

namespace CaliburnLinkedNotificationProperties.ViewModels
{
    public class ExtendedNotifyPropertyChangedBase : PropertyChangedBase
    {

        public ExtendedNotifyPropertyChangedBase()
        {
#if DEBUG
            foreach (var propertyInfo in
                GetType().GetProperties().Where(propertyInfo => Attribute.IsDefined(propertyInfo, typeof (NotifyOnChangeOfAttribute))))
            {
                foreach (var propertyName in propertyInfo.GetAttributes<NotifyOnChangeOfAttribute>(true)
                    .SelectMany(notifyOnChangeOfAttribute => notifyOnChangeOfAttribute.PropertyNames))
                    Debug.Assert(
                        GetType().GetProperty(propertyName) != null,
                        "Property not found",
                        "Interdependent property {0} declared on property {1} was not found on type {2}.",
                        propertyName, propertyInfo.Name, GetType().Name
                    );
            }
#endif
        }

        public override void NotifyOfPropertyChange(string propertyName)
        {
            base.NotifyOfPropertyChange(propertyName);
            if ( Attribute.IsDefined(GetType(), typeof(HasInterdependentPropertiesAttribute), true) )
                NotifyOfInterdependentPropertiesChange(propertyName, new List<string>());
        }

        protected virtual void NotifyOfInterdependentPropertiesChange(string changedPropertyName, IList<string> propertiesAlreadyInspected)
        {

            // Iterate through all the properties on this object
            foreach (var propertyInfo in GetType().GetProperties())
            {
                var thisPropertyName = propertyInfo.Name;
                // If we've already checked the attributes on this one, continue to prevent 
                // an infinite loop in the event of cyclical references
                if (propertiesAlreadyInspected.Contains(thisPropertyName)) continue;
                propertiesAlreadyInspected.Add(thisPropertyName);

                // Pick up the NotifyOnChangeOfAttribute attribute and loop through the
                // referenced properties if any
                if (propertyInfo.GetCustomAttributes(typeof(NotifyOnChangeOfAttribute), true)
                        .Cast<NotifyOnChangeOfAttribute>()
                        .Any(attr => attr.PropertyNames.Contains(changedPropertyName)))
                    NotifyOfPropertyChange(thisPropertyName);

                propertiesAlreadyInspected.Remove(thisPropertyName);
            }
        }

    }

    [AttributeUsage(AttributeTargets.Class)]
    public class HasInterdependentPropertiesAttribute: Attribute { }

    [AttributeUsage(AttributeTargets.Property)]
    public class NotifyOnChangeOfAttribute : Attribute
    {
        public NotifyOnChangeOfAttribute(params string[] propertyNames) { PropertyNames = propertyNames ?? new string[] { }; }
        public string[] PropertyNames { get; private set; }
    }

}
Thanks for the pointer, Rob.  Looks interesting.  Cecil (and PostSharp which I've come across looking into a better way to handle the attribute property names) are both tools of which I was completely ignorant until a few hours ago.