Converting between data and presentation types

Question

Below is how I'm solving the problem of converting between data and presentation types, I'd like to know if that's a good way to go about it, and if not, what would be a better way to go about it.

I already had an IViewModel interface:

/// <summary>
/// An interface for a ViewModel.
/// </summary>
public interface IViewModel : INotifyPropertyChanged
{
    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    void NotifyPropertyChanged<TProperty>(Expression<Func<TProperty>> property);

    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    void NotifyPropertyChanged(string propertyName);
}

So I added an IViewModel<T> interface that extends it:

/// <summary>
/// An interface for a ViewModel that encapsulates an entity type.
/// </summary>
/// <typeparam name="T">The entity interface type.</typeparam>
public interface IViewModel<T> : IViewModel where T : class
{
    /// <summary>
    /// A method that returns the encapsulated entity interface.
    /// </summary>
    /// <returns>Returns an interface to the encapsulated entity.</returns>
    T ToEntity();
}

Then to facilitate usage, I implemented it in a base class:

/// <summary>
/// Base class to derive ViewModel implementations that encapsulate an Entity type.
/// </summary>
/// <typeparam name="T">The entity type.</typeparam>
public abstract class ViewModelBase<T> : IViewModel<T> where T : class
{
    protected readonly T EntityType;

    /// <summary>
    /// Initializes a new instance of the <see cref="ViewModelBase{T}"/> class.
    /// </summary>
    /// <param name="entityType">An instance of the entity type to encapsulate.</param>
    protected ViewModelBase(T entityType)
    {
        EntityType = entityType;
        ReflectTypeProperties();
    }

    public T ToEntity()
    {
        return EntityType;
    }

    #region INotifyPropertyChanged implementation
    /// <summary>
    /// Occurs when a property value changes.
    /// </summary>
    public event PropertyChangedEventHandler PropertyChanged;

    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    /// <param name="propertyName">The name of the property to notify about.</param>
    public void NotifyPropertyChanged(string propertyName)
    {
        Action notify;
        _propertyNotifications.TryGetValue(propertyName, out notify);
        if (notify != null) notify();
    }

    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    /// <typeparam name="TProperty">The type of the property (inferred).</typeparam>
    /// <param name="property">An expression that selects a property, like <c>() => PropertyName</c>.</param>
    public void NotifyPropertyChanged<TProperty>(Expression<Func<TProperty>> property)
    {
        NotifyPropertyChanged(PropertyName(property));
    }

    private void NotifyPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        if (PropertyChanged != null) PropertyChanged(sender, e);
    }

    private IDictionary<string, Action> _propertyNotifications;

    /// <summary>
    /// Loads the names of all properties of the most derived type into a
    /// Dictionary where each entry (property name) points to a delegate that
    /// calls <see cref="NotifyPropertyChanged"/> for the corresponding property.
    /// </summary>
    private void ReflectTypeProperties()
    {
        var viewModelProperties = GetType().GetProperties().Where(p => p.CanWrite); // uses reflection (slow)

        _propertyNotifications = viewModelProperties
                                        .Select(property => new KeyValuePair<string, Action>(property.Name,
                                                      () => NotifyPropertyChanged(this, new PropertyChangedEventArgs(property.Name))))
                                        .ToDictionary(kv => kv.Key, kv => kv.Value);
    }

    /// <summary>
    /// Returns the name of a property in a LINQ Expression such as '<code>() => Property</code>'.
    /// Used for strongly-typed INotifyPropertyChanged implementation.
    /// </summary>
    protected static string PropertyName<TProperty>(Expression<Func<TProperty>> property)
    {
        var lambda = (LambdaExpression)property;
        MemberExpression memberExpression;

        var body = lambda.Body as UnaryExpression;
        if (body == null)
            memberExpression = (MemberExpression)lambda.Body;
        else
        {
            var unaryExpression = body;
            memberExpression = (MemberExpression)unaryExpression.Operand;
        }

        return memberExpression.Member.Name;
    }
    #endregion
}

This leaves me with clean & focused ViewModel classes that only expose what's meant to be displayed, while retaining the knowledge of the precious encapsulated Id:

/// <summary>
/// Encapsulates a <see cref="ISomeEntity"/> implementation for presentation purposes.
/// </summary>
public class SomeEntityViewModel : ViewModelBase<ISomeEntity>, ISelectable, IDeletable
{
    /// <summary>
    /// Encapsulates specified entity in a presentation type.
    /// </summary>
    /// <param name="poco">The entity to be encapsulated.</param>
    public SomeEntityViewModel(ISomeEntity poco) : base(poco) { }

    /// <summary>
    /// A short description for the thing.
    /// </summary>
    public string Description 
    { 
        get { return EntityType.Description; } 
        set { EntityType.Description = value; NotifyPropertyChanged(() => Description); } 
    }

    private bool _isSelected;
    public bool IsSelected 
    { 
        get { return _isSelected; } 
        set { _isSelected = value; NotifyPropertyChanged(() => IsSelected); } 
    }

    private bool _isDeleted;
    public bool IsDeleted 
    { 
        get { return _isDeleted; } 
        set { _isDeleted = value; NotifyPropertyChanged(() => IsDeleted); } 
    }
}

Bonus question: is my implementation of INotifyPropertyChanged overkill?

Answer 1

T ToEntity();

To me, ToEntity() implies some sort of conversion action. A better option might be T GetEntity() or even a property called Entity.

protected readonly T EntityType;

EntityType is a bad name for this field, because it does not contain a type, it contains the entity. Because of that, something like Entity might be better.

Also, you might want to consider making this into a property. The reasons for not using public fields also apply to protected fields (though not as strongly).

private IDictionary<string, Action> _propertyNotifications;

This seems completely unnecessary. Unless you know that this actually makes measurable improvement in performance (which I seriously doubt), just raise the event.

var lambda = (LambdaExpression)property;
var unaryExpression = body;

These two lines are unnecessary and I think they also don't improve readability.

memberExpression = (MemberExpression)unaryExpression.Operand;

If you're expecting only some specific UnaryExpressions, then I would check that those are actually what you have. For example, I think your code would work with () => !BoolProperty, which I think it shouldn't.

Answer 2

This is a wonderful implementation! Very nice getting boilerplate viewmodel code out of the way. That being said, I have a few tiny bits I'd change in ViewModelBase<T> (and the appropriate changes to IViewModel<T> too) as such (I've commented my changes):

/// <summary>
/// Base class to derive ViewModel implementations that encapsulate an Entity type.
/// </summary>
/// <typeparam name="T">The entity type.</typeparam>
public abstract class ViewModelBase<T> : IViewModel<T> where T : class
{
    // Was protected, now private and accessed by property below.
    private readonly T entity;

    // I like my members read-only as much as possible.
    private readonly IDictionary<string, Action> propertyNotifications;

    /// <summary>
    /// Initializes a new instance of the <see cref="ViewModelBase{T}"/> class.
    /// </summary>
    /// <param name="entity">An instance of the entity type to encapsulate.</param>
    protected ViewModelBase(T entity)
    {
        this.entity = entity;

        // Removed the ReflectTypeProperties() method and consolidated here so the member can be read-only.
        // Loads the names of all properties of the most derived type into a
        // Dictionary where each entry (property name) points to a delegate that
        // calls NotifyPropertyChanged() for the corresponding property.
        this.propertyNotifications = this.GetType().GetProperties()
            .Where(property => property.CanWrite)
            .Select(property => new KeyValuePair<string, Action>(
                property.Name,
                () => this.NotifyPropertyChanged(this, new PropertyChangedEventArgs(property.Name))))
            .ToDictionary(kvp => kvp.Key, kv => kv.Value);
    }

    /// <summary>
    /// Occurs when a property value changes.
    /// </summary>
    public event PropertyChangedEventHandler PropertyChanged;

    // public property removes need for protected member and ToEntity() method.
    public T Entity
    {
        get
        {
            return this.entity;
        }
    }

    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    /// <param name="propertyName">The name of the property to notify about.</param>
    public void NotifyPropertyChanged(string propertyName)
    {
        Action notify;

        // Removed need for extra null check as TryGetValue returns a bool. If successful, it should always have a non-null value per constructor.
        if (this.propertyNotifications.TryGetValue(propertyName, out notify))
        {
            notify();
        }
    }

    /// <summary>
    /// Notifies listener that the value of the specified property has changed.
    /// </summary>
    /// <typeparam name="TProperty">The type of the property (inferred).</typeparam>
    /// <param name="property">An expression that selects a property, like <c>() => PropertyName</c>.</param>
    public void NotifyPropertyChanged<TProperty>(Expression<Func<TProperty>> property)
    {
        this.NotifyPropertyChanged(PropertyName(property));
    }

    /// <summary>
    /// Returns the name of a property in a LINQ Expression such as '<code>() => Property</code>'.
    /// Used for strongly-typed INotifyPropertyChanged implementation.
    /// </summary>
    /// <returns>The name of a property in a LINQ Expression</returns>
    protected static string PropertyName<TProperty>(Expression<Func<TProperty>> property)
    {
        // Combination and simplification of statements here.
        var body = property.Body as UnaryExpression;
        var memberExpression = (MemberExpression)(body == null ? property.Body : body.Operand);

        return memberExpression.Member.Name;
    }

    /// <summary>
    /// Notifies listeners when the property has changed.
    /// </summary>
    /// <param name="sender">The sender.</param>
    /// <param name="e">The <see cref="PropertyChangedEventArgs"/> instance containing the event data.</param>
    private void NotifyPropertyChanged(object sender, PropertyChangedEventArgs e)
    {
        // This is for thread safety, in case the event subscribers are removed between statements.
        var propertyChanged = this.PropertyChanged;

        if (propertyChanged != null)
        {
            propertyChanged(sender, e);
        }
    }
}

Answer 3

Design Review

I am not convinced wrapping business layer entity properties in presentation layer is a good pattern for the following reasons:

• It asserts the presentation view of data is strongly related to the business entities. This certainly isn't always the case. Many presentation layers introduce even further denormalized presentation classes than the business entities.
• It is a breach of separation of concerns, in that user interaction can immediately adapt business entity state.
• By keeping presentation and business entities completely separated, you can focus on client-side end-user validation without impacting the business validation and vice versa.

---

Suggested Alternative

Create custom presentation classes that have no relation to business entities, even if most properties would include 1-1 mappings. Use a factory, builder or mapper pattern to put boiler-plate mapping code between layers in.