Using property change notifications and inheritance

Question

I have a base class which implements property changed notifications. I've omitted the implementation technical details for clarity.

public class PersonDTO : INotifyPropertyChanged {

  // in real these all have the backing field + equality check in setter + OnChanged call implementations
  public string Name { get; set; }
  public int Age { get; set; }
  public Gender Gender { get; set; }

  public PersonDTO() {
    // initialize default values
    // this invoke OnChanged so the object state can be maintained
    Name = "New person";
    Age = 30;
    Gender = Gender.Female;
  }

  protected virtual void OnChanged(string propertyName) {
    // raise PropertyChanged
  }
}

I have another class which inherits from PersonDTO and adds some properties.

public class PersonEditorModel : PersonDTO {

  public BindingList<string> Titles { get; private set; }

  private readonly IRepository _repository;
  public PersonEditorModel(IRepository repository) {
    _repository = repository;
  }

  protected override void OnChanged(string propertyname) {
    if (propertyName == "Gender") {
       // Here is a NullReferenceException
       Titles.Clear();
       if (Gender == Gender.Female) {
         Titles.AddRange(new[] {"Ms", "Mrs"});
       else
         Titles.AddRange(new[] {"Mr", "Sir"});
    }
    // do some other things perhaps using the _repository (which would raise a NullReferenceException again)
  }
}

The problem with this model is that in the base constructor, setting a property invokes the change notification, and the OnChanged method in the descendant class executes when the descendant is not yet constructed (Titles list is null).

Several ways I've been thinking of.

1. Use the backing fields in the base constructor. This would fix the exception, but the object state was not refreshed accordingly. I need a consistent state all the time, which means that the Gender and the Titles should be in sync.

2. Include a flag which means that the object is constructed, and check that flag in the OnChanged. This would be valid for simple cases like this, but what if I have a 3 level hierarchy. I would need to ensure that the flag is set when the bottom most constructor had finished running, which is not trivial.

3. Use a factory method pattern in the base class where after construction I would call something like StartChangeTracking(). This is problematic because the descendant classes can have different constructor arguments, like in this case an IRepository service. Also, the factory method pattern would make for example Json serialization/deserialization fairly harder (I mean for those classes without constructor arguments of course).

Answer 1

You got couple of choices here. First, you could directly assign the Titles, like below, assuming you won't ever reassign the BindingList<T> (normally in MVVM we use ObservableCollection<T> though) I've made it readonly. This assures that Titles will never be null and you won't have to do null-checks

// C# 6.0, read only property
public BindingList<string> Titles { get; } = new BindingList<string>();

// C# 5.0 and older
private readonly BindingList<string> titles = new BindingList<string>();
public BindingList<string> Titles { get { return titles; } }

Other (less optimal) option of course includes to do the null-check inside the OnChange Method.

        protected override void OnChanged(string propertyName)
        {
            if (propertyName == "Gender")
            {
                if(Titles==null) 
                {
                    Titles = new BindingList<string>();
                }
                Titles.Clear();
                if (Gender == Gender.Female)
                {
                    //Titles.AddRange(new[] { "Ms", "Mrs" });
                    Titles.Add("Ms");
                    Titles.Add("Mrs");
                }
                else
                {
                    Titles.Add("Mr");
                    Titles.Add("Sir");
                }
            }

            base.OnChanged(propertyName);
        }

Less optimal, because depending on the execution order you may end up in another branch where Titles could be null and you have to add additional null-checks and assignments. Further more, when your parent constructor is done executing it will execute your child's constructor and there Titles will be already assigned. If you reassign it here again, you override the assignments done inside OnChanged.

Last but not least, you can do lazy instantiation.

private BindingList<string> titles;
public BindingList<string> Titles { 
    get 
    {
        if(titles == null)
        {
            titles = new BindingList<string>();
        }

        return titles;
    }
}

This way, Title will always return an instance if you access the property and you don't have to instantiate it in the constructor.

You should use the first option I posted above, as it's the best one that follows best practices and for lists and other objects it works well. If you need a runtime parameter to instantiate it you only have constructor or lazy instantiation as option.

You should avoid Initialize() type of methods in your public api (interfaces, classes, etc.) as this is considered a code smell, as it's not obvious that calling this method is required for the correct functionality of the class.

P.S. Depending on the implementation of your OnChange call, you may or may not receive the "Gender" Event inside your overriden OnChange class. I assume Gender is a enum and if it's defined as following

public enum Gender 
{
    Female,
    Male
}

and you call Gender = Gender.Female inside your constructor, it may not trigger OnChange method, if your code looks like

public Gender 
{
    get { return gender; }
    set 
    {
        if(gender!=value) 
        {
            gender = value;
            OnChange("Gender");
        }
    }
}

Answer 2

Provided solutions work, but I think the architecture and even view models themselves can be improved as shown below:

1) data models vs. service models/view models separations - already mentioned in a comment, but here illustrated with an example. View models (your "editor" class) should be as loosely coupled as possible to the data models, so inheritance is a no-no. Also, DI advice is to prefer composition over inheritance, although some tricks might be employed.

2) prefer statically-typed over magic strings. E.g. e.PropertyName == "Gender" is vulnerable to property names changes that are usually done via refactoring (automatically), but fails to change these strings.

public enum Gender
{
    Male,
    Female
};

// this should be a simple class (POCO), persistence agnostic
public class PersonDTO
{
    public string Name { get; set; }
    public int Age { get; set; }
    public Gender Gender { get; set; }
}

// repository interface
public interface IRepository<T>
{
    IQueryable<T> GetAll();
    T GetById(int id);
}

// this is responsible for delivering person related information without exposing fetching details
public class PersonService
{
    private IRepository<PersonDTO> _Repository;

    public PersonService(IRepository<PersonDTO> repository)
    {
        _Repository = repository;
    }

    // normally, service should return service models that are view agnostic, but this requires extra mapping
    // so, for convenience service returns the view model
    public PersonEditorModel GetPerson(int id)
    {
        var ret = AutoMapper.Mapper.Map<PersonEditorModel>(_Repository.GetById(id));
        return ret;
    }
}

// base editor model (or view model)
public class BaseEditorModel : INotifyPropertyChanged
{
    /// <summary>
    ///  Occurs when a property value changes.
    /// </summary>
    public event PropertyChangedEventHandler PropertyChanged;

    /// <summary>
    /// Raises the PropertyChanged event
    /// </summary>
    /// <param name="propertyName">Name of the property</param>
    protected void OnPropertyChanged(string propertyName)
    {
        var ev = PropertyChanged;
        if (ev != null)
            PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
    }
}

// base editor model (or view model) that allow statically-typed property changed notifications
public abstract class BaseEditorModel<TVm> : BaseEditorModel
                              where TVm : BaseEditorModel<TVm>
{
    /// <summary>
    /// Raises the PropertyChanged event
    /// </summary>
    /// <param name="expr">Lambda expression that identifies the updated property</param>
    protected void OnPropertyChanged<TProp>(Expression<Func<TVm, TProp>> expr)
    {
        var prop = (MemberExpression)expr.Body;
        OnPropertyChanged(prop.Member.Name);
    }
}

// the actual editor
// notice that property changed is done directly on setters and without magic strings
public class PersonEditorModel : BaseEditorModel<PersonEditorModel>
{
    public BindingList<string> Titles { get; private set; }

    public PersonEditorModel()
    {
        Titles = new BindingList<string>();

        UpdateTitles();
    }

    private Gender _Gender;
    public Gender Gender
    {
        get { return _Gender; }
        set
        {
            _Gender = value;
            UpdateTitles();
            OnPropertyChanged(m => m.Gender);
        }
    }

    private void UpdateTitles()
    {
        Titles = Gender == Gender.Female ?
            new BindingList<string>(new[] { "Ms", "Mrs" }) :
            new BindingList<string>(new[] { "Mr", "Sir" });
        OnPropertyChanged(m => m.Titles);
    }
}

// just an example
class Program
{
    static void Main(string[] args)
    {
        // this should be performed one per application run
        // obsolete in a newer version
        AutoMapper.Mapper.CreateMap<PersonDTO, PersonEditorModel>();

        var service = new PersonService(null);     // get service using DI

        // work on a dummy/mock person
        var somePerson = service.GetPerson(30);

        // bind and do stuff with person view model
    }
}

Answer 3

I've finally realized that to solve this issue I have to solve the root issues which indirectly caused this behavior. The root issue is virtual member call in constructor (in this case in an indirect way).

So I decided to not use the virtual OnChanged method at all, and instead, self-subscribe to the object's own PropertyChanged event. The self-subscription should be always safe, without the need of unsubscription. The solution looks like this.

public class PersonEditorModel : PersonDTO {

  public BindingList<string> Titles { get; private set; }

  private readonly IRepository _repository;
  public PersonEditorModel(IRepository repository) {
    _repository = repository;
    Titles = new BindingList<string>();

    UpdateTitles();
    PropertyChanged += OnPropertyChanged;
  }

  private void OnPropertyChanged(object sender, PropertyChangedEventArgs e) {
    if (e.PropertyName == "Gender") {
       UpdateTitles();
    }
  }

  private void UpdateTitles() {
    Titles.Clear();
    if (Gender == Gender.Female) {
      Titles.AddRange(new[] {"Ms", "Mrs"});
    else
      Titles.AddRange(new[] {"Mr", "Sir"});
  }
}