Wednesday, 23 July 2008

Is INotifyPropertyChanged an anti-pattern?

Something is starting to bother me recently about data binding and the INotifyPropertyChanged interface. It's just that EVERY class you write ends up having to implement it (I am generally talking about writing WPF/Silverlight applications here). And that's just not sitting well with me!

It doesn't sit well because I believe it violates the principles of separations of concern. The major annoyance for me lies with calculated properties (those which are derived from another). Lets start with a simple class, Order, which contains two properties, ItemPrice and Quantity (other fields and methods omitted for sake of brevity):

public class Order : INotifyPropertyChanged
{
  public decimal ItemPrice 
  { 
    get { return this.itemPrice; }
    set 
    {
       this.itemPrice = value;
       this.RaisePropertyChanged("ItemPrice");
    }
  }

  public int Quantity 
  { 
    get { return this.quantity; }
    set 
    {
       this.quantity= value;
       this.RaisePropertyChanged("Quantity");
    }
  }
}

We need to add a property TotalPrice to this entity which will expose the total price of the order. What do we do here? This is a calculated value so should clearly be read only, and I usually implement the calculation within the property itself. What I have found myself doing many times in the past (and I have seen done in a lot of examples) is:

public class Order : INotifyPropertyChanged
{
  public decimal ItemPrice 
  { 
    get { return this.itemPrice; }
    set 
    {
       this.itemPrice = value;
       this.RaisePropertyChanged("ItemPrice");
       this.RaisePropertyChanged("TotalPrice");
    }
  }

  public int Quantity 
  { 
    get { return this.quantity; }
    set 
    {
       this.quantity= value;
       this.RaisePropertyChanged("Quantity");
       this.RaisePropertyChanged("TotalPrice");
    }
  }

  public decimal TotalPrice
  {
    get { return this.ItemPrice * this.Quantity; }    
  }
}

This is nasty. We have had to modify our other two properties just because we have added this new one. It works, but as you add more and more dependent properties your code gets messy. Those properties shouldn't need to know that the TotalPrice property exists!

That pattern breaks down completely when you start using inheritance in your domain objects. Imagine if I now require a new type of domain object, SalesOrder, where I want to express the fact that there is a commission (expressed as a fraction) which needs to be paid to some salesperson on this order:

public class SalesOrder : Order
{
  public decimal SalesCommision
  { 
    get { return this.salesCommision}
    set 
    {
       this.salesCommisionPercentage = value;
       this.RaisePropertyChanged("SalesCommision");
       this.RaisePropertyChanged("TotalCommission");
    }
  }

  public decimal TotalCommission
  { 
    get { return this.TotalPrice * this.SalesCommission; }
  }
}

Uh-oh! It's come a bit unstuck here. If the price changes in the base class, my UI is not going to redisplay the total commission! This could be great for my sales person but not so great for my accountant!

So what do we do here to get around this? We clearly can't modify our base class, so how about we listen to our own property notifications, like thus:

public class SalesOrder : Order
{
  // Properties defined as previously //

  protected override void RaisePropertyChanged(string propertyName)
  {
     base.RaisePropertyChanged(propertyName);
     if (propertyName == "TotalPrice")
     {
        this.RaisePropertyChanged("TotalCommission");
     }
  }
}

Here we override the method which raises the event, check if the relevant dependent property is being raised, and if so we raise a changed event on our commission property. This works, but YUCK! Just imagine having to unit test all this! :-(

Note that we end up with the same problem if we use composition. Imagine that instead of storing the sales commission on our entity, it was actually stored on a related SalesPerson entity and we need to calculate it from there. Now we need to know that if the sales person's commission changes, the order's total commission has changed:

public class SalesOrder : Order
{
  public SalesPerson SalesPerson
  { 
    get { return this.salesPerson; }
    set 
    {
       if (this.salesPerson != null)
       {
          this.salesPerson.PropertyChanged -= HandleSalesPersonPropertyChanged;
       }

       this.salesPerson = value;
       this.RaisePropertyChanged("SalesPerson");
       this.RaisePropertyChanged("TotalCommission");

       this.salesPerson.PropertyChanged += HandleSalesPersonPropertyChanged;
    }
  }

  public decimal TotalCommission
  { 
    get { return this.TotalPrice * this.SalesPerson.Commission; }
  }

  protected virtual void HandleSalesPersonPropertyChanged(object sender, PropertyChangedEventArgs e)
  {
     if (e.PropertyName == "Commission")
     {
        this.RaisePropertyChanged("TotalCommission");
     }
  }
}

We subscribe to the PropertyChanged event on our sales person, and when its commision is changed, we act accordingly.

I think this is all a mess, and it just gets worse and worse as you build up your domain model and add more classes and more relations. It's easy to forget one of these dependencies. If you are building a fairly complex UI on top of this (e.g. one which allows you to drill down and navigate through the relationships and make changes to objects), I'm so sure you will introduce bugs due to not raising the event, I know it's happened to me!

On my next (real) WPF project this is definitely something I will be thinking about seriously.

Anyway I would be interested to know what people think about this. Is this a problem which other people have faced? How do you architect your applications to avoid this complexity?

18 comments:

Oran Dennison said...

You might look into the INotifyPropertyChanged / IEditableObject support provided by the PostSharp samples. I believe the samples used to be online but are now only available in the zip file.

Neil Mosafi said...

Hmm, is this what you are talking about Oran?
http://code.google.com/p/postsharp-user-samples/wiki/DataBindingSupport

This is good but still suffers most of the problems I was referring to around calculated properties, as far as I can tell?

Oran Dennison said...

The example I'm thinking of used to be available at http://doc.postsharp.org/UserGuide/Samples/PostSharp.Samples.Binding.html but is now available inside http://download.postsharp.org/releases/1.0-rc4/PostSharp-1.0.10.403-Release-Binary.zip

in the following location:
samples\PostSharp.Samples.Binding\PostSharp.Samples.Binding.html

It focuses more on INotifyPropertyChanged than on IEditableObject, and the code is C#, not VB like the Google Code example.

I haven't fully thought this through and I'm not sure how far this can be taken with PostSharp (you would have to extend the sample), but it seems like it might be possible to annotate a calculated property with attributes that specify its dependencies, and then use PostSharp to post-compile the appropriate RaisePropertyChanged calls into the specified dependencies. You might even be able to use reflection to dynamically determine the dependencies.

Neil Mosafi said...

Great, thanks for the link, I'll check it out! I agree it may be possible to automate all of this with some clever dependency resolution, and if I get anywhere with that I'll update you!

Sebastien Lambla said...

I'd recommend using the implementation of DataModel and DataModelCollection that never got used would've helped a lot :)

More seriously, DP2 / PostSharp help a lot in that, especially if you keep the data edited by your application as DTOs with a PresentationModel. If it's a DTO for the screen, it's job is indeed to let one view edit data, and using INotifyPropertyChanged there is not necessarily breaking SOC.

Neil Mosafi said...

@seb - Yeah it would definitely have worked for the state tracking, just not sure if it would have worked for all circumstances such as calculated properties I alluded to in my post.

Definitely would implement that on the next version if there is ever one!

Cheers

Luke Puplett said...

Hi Neil,

Did you ever get close to a satisfactory solution for this?

Reading your posting was like a flash of déja vu for me. I've also overidden my own 'Raise' virtuals to solve it, and felt dirty about doing so.

More recently I've wondered whether only pure 'state' should be properties (like fields).

This is because WCF a) can't use readonly properties meaning that I have occasionally added meaningless private setters to make it compile and b) after serialization, the logic is lost (unless the client has a copy of the original class library to reference).

So making them methods means that they don't get serialized and the client developer can't even see them - and thus won't get caught out by coding against a property which has lost its auto-calc logic.

The only solution that makes sense to me in this situation is to hold state only, and let the calculations be done by the consumer object(s), but of course we lose out on direct binding and the headache is shifted into the UI.

Neil Mosafi said...

Hi Luke

No I never really did find one I was happy with, still hunting!

Regarding your question about WCF - I would recommend you take a look at the Data Transfer Object pattern whereby you can define your data contracts with only state, and still have internal domain model with behaviours and readonly properties and the like. You simply map between your internal model and the data contracts at the boundary of your service.

The alternative approach which you alluded to is known as the "Anemic Data Model" and I don't really like it, though I have seen it succeed in a number of projects!

All the best
Neil

Luke Puplett said...

Thanks Neil. For the project I'm working on, I seem to be using a DTO-style pattern. My basic WCF service entities (simple classes with data contracts) are declared in a core library and extended in a server-side library to incorporate ADO.NET crud logic(AdoCustomerProfile : Core.CustomerProfile). The data contracts in the extended types share the same name and namespace as the core library. The client also has the core lib and so WCF marshals by state from one type to another.

I'm planning on using Model/View/ViewModel for the Silverlight client and so only the ViewModel classes will implement INotifyPropertyChanged, thus wrapping some of the core classes and insulating them from having to know about UI observers. That's the theory anyway... Good luck!

Tomas Elison said...

Neil, I have been whacking my brain at this exact problem that you describe in the original post, lately. And I have come up with a solution that I think is quite nice.

The root of the problem with the "brute force" version of solving this, like you show in your sample code is that the dependencies are backwards from a OO design perspective. The ItemPrice and Quantity properties should not be aware of the fact that the TotalPrice property depends on them. However, the TotalPrice property knows that it depends on ItemPrice and Quantity, which is fine. The solution I came up with turns these dependencies around the right way and also lets you specify these dependencies declaratively.

Here are the updated versions of your classes that use my magic class PropertyDependencyManager:

--------------------------------------------------------
public class Order : INotifyPropertyChangedPlus
{
public Order()
{
PropertyDependencyManager.Register(this);
}

public decimal ItemPrice
{
get { return itemPrice; }
set
{
itemPrice = value;
OnPropertyChanged("ItemPrice");
}
}

public int Quantity
{
get { return quantity; }
set
{
quantity = value;
OnPropertyChanged("Quantity");
}
}

[DependsOn("ItemPrice", "Quantity")]
public decimal TotalPrice
{
get { return ItemPrice*Quantity; }
}

public void OnPropertyChanged(string propertyName)
{
if (PropertyChanged != null)
{
PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
}
}

public event PropertyChangedEventHandler PropertyChanged;
}


public class SalesOrder : Order
{
public decimal SalesCommission
{
get { return salesCommission; }
set
{
salesCommission = value;
OnPropertyChanged("SalesCommission");
}
}

[DependsOn("TotalPrice", "SalesCommission")]
public decimal TotalCommission
{
get { return TotalPrice*SalesCommission; }
}
}

--------------------------------------------------------

The DependsOn attribute contains no logic at all, it is just an attribute with a string collection property.

--------------------------------------------------------
[AttributeUsage(AttributeTargets.Property)]
public class DependsOnAttribute : Attribute
{
public DependsOnAttribute(params string[] properties)
{
Properties = properties;
}

public string[] Properties { get; private set; }
}
--------------------------------------------------------

Now for the class with the magic, the PropertyDependencyManager. This class builds a dependency graph the "right" way. It knows for example that if Quantity is changed, TotalPrice has asked to be informed about this. The PropertyDependencyManager listens to the PropertyChanged event for any class that is registered with it and uses the dependency graph to propagate the events the right way.

--------------------------------------------------------
public class PropertyDependencyManager
{
private static readonly List<PropertyDependencyManager> registeredInstances = new List<PropertyDependencyManager>();
private readonly INotifyPropertyChangedPlus notifyTarget;
private readonly Type targetType;
private Dictionary<string, List<string>> dependencyGraph;

private PropertyDependencyManager(INotifyPropertyChangedPlus target)
{
notifyTarget = target;
targetType = target.GetType();
notifyTarget.PropertyChanged += notifyTarget_PropertyChanged;
CreateDependencyGraph();
}

public static void Register(INotifyPropertyChangedPlus target)
{
registeredInstances.Add(new PropertyDependencyManager(target));
}

private void CreateDependencyGraph()
{
dependencyGraph = new Dictionary<string, List<string>>();

foreach (var property in targetType.GetProperties())
{
foreach (DependsOnAttribute attribute in property.GetCustomAttributes(typeof (DependsOnAttribute), true))
{
foreach (var propertyWithDependee in attribute.Properties)
{
if (!dependencyGraph.ContainsKey(propertyWithDependee))
{
dependencyGraph.Add(propertyWithDependee, new List<string>());
}
dependencyGraph[propertyWithDependee].Add(property.Name);
}
}
}
}

private void notifyTarget_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (dependencyGraph.ContainsKey(e.PropertyName))
{
foreach (var dependeeProperty in dependencyGraph[e.PropertyName])
{
notifyTarget.OnPropertyChanged(dependeeProperty);
}
}
}
}
--------------------------------------------------------

INotifyPropertyChangedPlus is a slightly extended version of INotifyPropertyChanged. It simply includes the OnPropertyChanged(string propertyName) as a required part of the interface. (The method we all write anwyays). The reason for this is that this enables the PropertyDependencyManager to tell the registered class to fire its PropertyChanged event for a certain property, from the "outside".

--------------------------------------------------------
public interface INotifyPropertyChangedPlus : INotifyPropertyChanged
{
void OnPropertyChanged(string propertyName);
}
--------------------------------------------------------

With these three small classes I think we have a nice working solution. This allows for properties that depends on properties that depends on properties and so on without increasing the complexity.

Tomas Elison
Special Operations Software

Neil Mosafi said...

Tomas, that is a great solution! I will definitely consider using your PropertyDependencyManager in the future. Perhaps it would also combine well with Seb's DynamicProxy solution!

Andrew Tweddle said...

Neil - I feel your pain with this one.

Earlier this year I came up with a POC for a "dependency propagation engine", which could help to address this issue. But I'm not sure that I like it much either, as it involves some strange-looking setup code (a little like WPF DependencyProperty syntax).

Here's a sample class from my unit tests:

--------------------------------

public class SampleClass
{
#region "Private member variables"
FormulaProperty<SampleClass, double> inputProperty;
FormulaProperty<SampleClass, double> outputProperty;

#endregion


#region "Constructors"

public SampleClass()
{
inputProperty = new FormulaProperty<SampleClass, double>(this);
outputProperty = new FormulaProperty<SampleClass, double>(this);
}

#endregion


#region "Public properties"

public FormulaProperty<SampleClass, double> InputProperty
{
get { return inputProperty; }
}

public FormulaProperty<SampleClass, double> OutputProperty
{
get { return outputProperty; }
}

[ValueForFormula("InputProperty")]
public double Input
{
get
{
return inputProperty.Value;
}
set
{
inputProperty.Value = value;
}
}

[ValueForFormula("OutputProperty")]
public double Output
{
get
{
return outputProperty.Value;
}
}

#endregion
}

--------------------------------

Here is one of my unit tests:

[Test]
public void ShouldCalculateCorrectlyTheFirstTime()
{
SampleClass sample = new SampleClass();
sample.OutputProperty.Formula = s => 3 * s.Input + 1;
sample.Input = 10;

Assert.AreEqual(31, sample.Output);
}

--------------------------------

The FormulaProperty class keeps track of which properties it depends on (to unregister them when the formula changes), and which properties depend on it (so that it can notify them to recalculate themselves).

FormulaProperty.Formula is a LINQ Expression Tree. When changed, this searches for all referenced properties in the formula which have a ValueForFormula attribute. It uses these to create the forwards and backwards dependency links. Note that these properties can come from other instances of other classes as well.

Incidentally the expression tree is compiled to a LambdaExpression for speed of re-calculation.

This code was just a proof of concept. But I didn't like the syntax too much... for example using an attribute with the name of the formula property to associate the value property with its FormulaProperty.

Attempt 2 would focus on streamlining the syntax. One of your previous blog entries has already given me an idea for doing this. I could replace the ValueForFormula attribute with an extra constructor argument like this:

outputProperty = new FormulaProperty<SampleClass, double>(this, s => s.Output);

Something I didn't do, but which would be a good idea (especially for the scenario you describe here), is to force the owning class to implement INotifyPropertyChanged. The FormulaProperties could then take care of raising the property changed events.

One downside... both of the above changes would place constraints on the owning class which the existing solution didn't need to do.

The hardest and most important thing I would like to change is to improve the efficiency by using a propagation manager. This would be somewhat similar to Tomas' PropertyDependencyManager.

This should allow one to temporarily disable propagation of calculations, change a number of properties, then re-enable it again. At that point it should propagate changes through the dependency tree in the "most efficient" order.

Perhaps I'll start work on this again, and post an article on CodeProject when I'm done.

Philipp said...

Great thoughts here! We had a similar problem that even spanned various objects, which resulted in an independent library that tackles the issue.
Basically, it allows me to define dependencies through longer object graphs, e.g.

var dependency = DependencyNode.Create(() => Foo.Bar.Property.AnotherProperty);

...and react to changes on that targeted dependency graph:
dependency.DependencyChanged += delegate { ... };

Here's a short sample that creates a PropertyChanged event:


//some property that is calculated
public string ComplexProperty
{
get { ... }
}

//some object we're dependent on
public Foo someObject;


//creates a dependency over a whole object graph:
//someObject - Property - SubProperty
public void CreateDependencies()
{
var dependency = DependencyNode.Create(() => someObject.Property.SubProperty);
dependency.DependencyChanged += OnDependencyChanged;
...
}


//triggers a property change event
private void OnDependencyChanged(...)
{
//bubble property change event
OnPropertyChanged(() => ComplexProperty);
}



I've written some more about it here:
Lambda Dependencies

Cheers,
Philipp

Michael L Perry said...

I agree that INotifyPropertyChanged is an anti-pattern. I've written an article in Code Magazine entitled INotifyPropertyChanged is Obsolete.

http://www.code-magazine.com/Article.aspx?quickid=0907101

This article demonstrates an alternative, Update Controls .NET. This open source library automatically discovers dependencies, even through intermediate business logic and view models.

Emiel Jongerius said...

I have posted another solution to this problem here: http://www.pochet.net/blog/2010/07/02/inotifypropertychanged-automatic-dependent-property-and-nested-object-support/

Let me know what you think about this...

Riko Eksteen said...

Hi

I too think the PropertyDependencyManager solution is a good one and have used it.

Just for anyone who's interested, I'm not using the INotifyPropertyChangedPlus interface.

Instead, the PropertyDependencyManager.Register method takes an Action delegate as input which can be used to call the RaisePropertyChanged in the original caller, like so:

-----------------------------------

public static void Register(INotifyPropertyChanged target, Action propertyChangedCallback)
{
registeredInstances.Add(new PropertyDependencyManager(target, propertyChangedCallback));
}

-----------------------------------

public class Order : INotifyPropertyChanged
{
public Order()
{
PropertyDependencyManager.Register(this, OnPropertyChanged);
}

-----------------------------------

net performance said...

Nice approach!However I feel that When implementing I Notif Property Changed, make sure that you select the correct one! The one you want to use is into Windows.UI.Xaml.Data.

Benjamin Gale said...

I have developed a sample application that uses a property system similar to that used by JavaFX.

It is still in it's early stages but if anyone is interrested you can read about it (and download sample code) on my blog: http://programmingwithpassion.wordpress.com/