6
\$\begingroup\$

I have written an abstract View Model which all my other View Models inherit when I write any apps in Silverlight, WinRT, WPF etc. The class was written to attempt to address an issue I have run into frequently. That is, after refactoring code, all my property names are wrong and I often forget to rename them. Debugging by identifying views which don't reflect their view models is hard, thus this.

The idea is relatively simple. It provides an implementation of the INotifyPropertyChanged interface with the addition of some additional safety checking when debugging the project. This is done via reflection by checking if the property you are attempting to notify actually exists.

Thus:

class Foo : AbstractViewModel {
    public object Bar { get; set; }
}

Will throw an exception if I try to call NotifyPropertyChanged("Ponies") since Ponies is not a property in Foo.

The AbstractViewModel looks like this:

using System;
using System.ComponentModel;

#if DEBUG
// We need reflection in order to debug our Property Changed Notifications
using System.Reflection;
#endif

namespace WorldOfZero.Common.ViewModels
{
    public class AbstractViewModel : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;

        protected void NotifyPropertyChanged(string name)
        {
#if DEBUG
            // This will detect and throw an error if we attempt to Notify a change on a none existant property.
            // This helps eliminate stupid mistakes like spelling errors.
            // This code is only included in the debug builds of the app, this prevents us from slowing down production code.
            Type type = this.GetType();
            PropertyInfo property = type.GetRuntimeProperty(name);
            if (property == null)
            {
                throw new InvalidOperationException(String.Format("Attempting to Notify a change on a property ({0}) that does not exist.", name));
            }
#endif

            if (PropertyChanged != null)
            {
                PropertyChanged(this, new PropertyChangedEventArgs(name));
            }
        }
    }
}

I'm looking for ways to improve this. I'm also curious if there might be better ways to solve this than with Reflection.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$ Commented Jul 23, 2015 at 3:10

1 Answer 1

Reset to default
8
\$\begingroup\$ 
#if DEBUG
// We need reflection in order to debug our Property Changed Notifications
using System.Reflection;
#endif

That #if directive is overkill. A using statement isn't like a C++ include, it doesn't clutter up your assembly with code you may not need. All it does, is facilitate coding by telling the parser that types in these namespaces don't need to be fully qualified; once the code is compiled, they're "inlined" anyway.

Now, the idea is nice, but as you noticed, you can give it just about any value and it happily passes the "name" to whoever is listening for that event.

That string parameter isn't refactoring-friendly. You've mitigated the issue by throwing an InvalidOperationException - if your ViewModel classes' setters are 100% covered with unit tests, you're probably good to go.

The thing that worries me is that the exception isn't thrown in a release build, so instead of blowing up and failing promptly, you're failing silently, and worse things could result from this. Of course that should not happen.. but the room for human error is there.

The best way to implement that interface, is in my opinion with a strongly-typed overload that takes a selector expression and passes the selected member's name to the interface's stringly-typed method.

Something like this for example, can turn your setters into something like this:

private int _foo;
public int Foo
{
    get { return _foo; }
    set
    {
        if (value != _foo)
        {
            _foo = value;
            OnPropertyChanged(() => this.Foo);
        }
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ I have been trying to do this for ages. The solution I posted above was sort of my compromise. That said, I'm going to take a day to run through your code in the post you linked and see what comes of that. I like the fact that you turn my runtime errors into syntax errors. And it supports refactoring. Very cool. Also, the point about Using's is good to know. I appreciate the critique. \$\endgroup\$ Commented Jul 23, 2015 at 13:17
  • \$\begingroup\$ To be fair I grabbed the selector expression code from Stack Overflow, and adapted it a bit. You should be able to find plenty of examples by searching for "strongly-typed INotifyPropertyChanged" ;-) \$\endgroup\$ Commented Jul 23, 2015 at 13:56
  • \$\begingroup\$ I spent quite a while researching this on Google etc. Granted, at the time I wasn't looking to solve this problem. I was hopping to be able to add an Attribute to a property and have it handle everything else behind the scenes. That didn't quite work out though. \$\endgroup\$ Commented Jul 23, 2015 at 14:01
  • 1
    \$\begingroup\$ So, I have been using a slightly modified version of your base ViewModel class. Now that C# 6 has a nameof operator I'm using that and the [CallerMemberName] instead. The performance increase is not insignificant since this removes all need for reflection. \$\endgroup\$ Commented Oct 20, 2015 at 0:34

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.