Avoid type checking but preserve separation of concerns

Question

I'm working on an MVVM project and trying to preserve separation of concerns. Our current architecture has an entity framework model and MVVM light view and viewmodel projects. I'm working ViewModel first so I'm passing those around with a datatemplate in the view project to get the proper view.

Having to type check every type to get the correct viewmodel just seems wrong. Is there a better way?

To set this up suppose I have a model with classes:

namespace FoodData
{
    public class Hamburger:EntityObject
    {
            ..... //Add properties
    }

    public class Taco:EntityObject
    {
           .... //Add properties
    }

}

Now Lets say in my ViewModel namespace I have two different ViewModels for Taco and Hamburger

namespace FoodViewModel
{
  public class HamburgerViewModel : ViewModelBase
  {
  }

  public class TacoViewModel : ViewModelBase
  {
  }

  public class FoodViewModel : ViewModelBase
  {

    private ViewModelBase _displayViewModel;

    public ViewModelBase DisplayViewModel
    {
       get {return _displayViewModel;}
       set 
        {
            _displayViewModel = value;
            RaisePropertyChanged(nameof(DisplayViewModel));
        }
     }

     private EntityObject _selectedFood;

     public EntityObject SelectedFood
     {
        get{return _selectedFood;}
        set
        {
          _selectedFood = value;
          DisplayViewModel = _selectedFood?.GetViewModel();
          RaisePoropertyChanged(nameof(SelectedFood));
        }
     }

  }
}

The current solution I have is using extension methods in the FoodViewModel namespace

namespace FoodViewModel
{
   public static FoodViewModelExtensions
   {
       public static ViewModelBase GetViewModel(this EntityObject obj)
       {
            if(obj is Taco)
            {
              return new TacoViewModel();
            }
            if(obj is Hamburger)
            {
              return new HamburgerViewModel();
            }

            return new EmptyViewModel("Object not recognized");
       }
    }

}

Answer 1

What you have here between your models and viewmodels is a mapping.

No amount of code sorcery¹ is going to change the fact that you're going to have to map pairs of (model+viewmodel) in order to decide which viewmodel to use for your model. As this is a given, the only real discussion left is how we want to define this mapping.

There are myriad ways to do so; most of which can be argued pro and con. We've already seen a few in answers, which I'll include in the list. Others are my suggestions.

---

• (suggested by Lee) Letting the model implement a generically typed interface for the viewmodel (class Hamburger : IHasViewModel<HamburgerViewModel>)

This is the first thing I thought of too. However, I do agree that it feels a bit dirty for your model to have to know its related viewmodel (especially if you have several viewmodels for a given model).

---

• (suggested by Felix Castor) Setting a Dictionary<ModelType,ViewModelType defined by letting the viewmodel implement a generically typed interface for the model (class HamburgerViewModel : FoodViewModelBase<Hamburger>)

The interface implementation has the same pros and cons as the previous bullet point, but now you have an added responsibility of having to use reflection to compose a dictionary on the fly?

I'm not a fan of this added complexity. The previous bullet point seems objectively better (due to less management) with no additional drawbacks.

---

• (as a counter to Felix Castor's idea): why not simply maintain the type dictionary yourself, and do away with the interfaces and reflection?

Something like this:

public static Dictionary<Type,Type> ModelToViewModelDict = new Dictionary<Type,Type>()
{
    { typeof(Hamburger) , typeof(HamburgerViewModel) }
    { typeof(Taco) , typeof(TacoViewModel) }
}

This way, neither your model nor viewmodel class definitions are dirtied, because the mapping is maintained externally to the classes.

Having to maintain the dictionary isn't really an issue (as discussed in the beginning, the mapping needs to be maintained somewhere, in some shape anyway).

---

• Using a visitor.

Unless I'm misunderstanding the example, this would require a similar generically typed interface and, on top of that, also require you to design a visitor who needs to be updated for every newly added food.

I don't see the benefits of this approach compared to the others. You end up with the same "dirty" model/viewmodel link by implementing an interface from one one the other; while also having to maintain an extra definition.

However, I'll admit that I have little experience with the visitor pattern in general, so I may be unaware of additional pro's (or cons).

Edit
The Visitor answer has been slightly updated since I wrote this, and I understand it a bit better now.
I still don't like it. It requires an overloaded method per entity type (which is very similar to your original FoodViewModelExtensions class); yet also requires every entity type to implement an interface (granted, it doesn't contain a generic type used for mapping purposes, but you are going to be copy/pasting the same interface method implementation all over your entities).

This feel like double work. A mapping can be achieved with only doing one of the two tasks that the visitor requires.

---

• Don't overgeneralize your method parameters.

If you need to know the specific type of your passed object, then don't declare it as a shared base type. Instead, rely on overloading:

public static ViewModelBase GetViewModel(this Taco obj)
{
    return new TacoViewModel(obj);
}

public static ViewModelBase GetViewModel(this Hamburger obj)
{
    return new HamburgerViewModel(obj);
}

This comes with the added benefit that you can choose whether a default option (when the type is not known) can be given, or instead throw a compile-time error. If you want no default option to exist, then keep the code as above. However, if you want to allow the passing of types that are not explicitly mapped, you can add a default:

public static ViewModelBase GetViewModel(this EntityObject obj)
{
    return new EmptyViewModel(obj);
}

Note for the default method:
This is a bit of a pitfall. If you declare your entity as EntityObject myTaco = new Taco(); and then call GetViewModel(myTaco), you're going to end up in the default GetViewModel(this EntityObject obj) method; which is not what you'd want. This is because the type of the variable is used to decide which overloaded method to use.

To avoid this, make sure you declare your entity by its proper type: Taco myTaco = new Taco(); and then you will end up in the correct GetViewModel(this Taco obj) method.

Maintenance-wise, it's not all that different from your if chain. Originally, you needed to add a new if for every type, whereas now you need to add a new method for every type.

I like this version better, but I do concede that it at least partially relates to coding style preferences.

Note
Similar to how I advised against overgeneralizing your Hamburger and Taco objects as EntityObjects; there may be a similar argument to be made about having every method return a ViewModelBase as opposed to the specific HamburgerViewModel and TacoViewModel.

Even if you handle the resulting viewmodel the same way, you can still have the method return an object of type HamburgerViewModel but choose to store the return object in a ViewModelBase variable (thus not changing anything). So I would suggest keeping the return type sufficiently specific, and not overgeneralized.

---

• Use a factory pattern.

This would be very similar to your current FoodViewModelExtensions class; with only a minor syntax change.

---

¹I really wanted to come up with a suggestion that wouldn't require any manual maintenance when new entities get added to the codebase.

However, the best I could come up with is:

• Using standardized naming across your model/viewmodel names, e.g. a Foo entity must have a viewmodel that is named FooViewModel.

You'll have to rely on reflection to find an object's corresponding viewmodel.Not having to manually maintain this is nice; but this is essentially a stringly typed approach; which I'm generally opposed to.

I'm less offended by the stringly typed approach than for other use cases; but it still feels a bit wrong to me. However, if you're really wanting to automate your mapping, this would effectively prevent you from having to manually maintain it.

---

To conclude

There are many ways to approach this. Each method has its own benefits, drawbacks, and applicability to the developer's preferred coding style (or the company's coding standards).

The most important thing to consider here is that you want your mapping to have a minimal footprint.

• At an absolute minimum, using naming conventions to auto-map models and viewmodels is viable; but comes with some drawbacks in that the structure of an application shouldn't be defined by the names that are used.
• A middle-of-the-road approach would require you to define the mapping once. There are two possibilities here:
  • Spreading the mapping over many locations (e.g. by having each model class definition list a IHasViewModel<HamburgerViewModel>)
  • Keeping the mapping contained in a single location (e.g. by using the dictionary, overloaded extension methods, or the factory). I much prefer this approach as it keeps the mapping centralized (inspired by SRP).
• What you should definitely avoid is having the mapping be defined more than once. The posted example of the visitor comes pretty close to this by having you maintain both an interface mapping and a visitor class definition, both of which need to be updated when a new entity is added into the mix.

In the end, it's really just dealer's choice. Company coding standards (or those of your team lead) will likely take precedence. If not available, then it's up to you to decide which approach's drawbacks you're most willing to live with.

Answer 2

One approach to separating the declaration of a type hierarchy from the traversal is to use a visitor e.g.

public interface IModelVisitor<TResult>
{
    TResult Visit(Hamburger h);
    TResult Visit(Taco t);
}

public interface IModel
{
    TResult Accept<TResult>(IModelVisitor<TResult> visitor);
}

public class Taco : IModel
{
    TResult Accept<TResult>(IModelVisitor<TResult> visitor) 
    {
        return visitor.Visit(this);
    }
}

then you can create a visitor for creating viewmodels:

public class ViewModelVisitor : IModelVisitor<ViewModelBase>
{
    public ViewModelBase Visit(Taco t) { return new TacoViewModel(); }
    public ViewModelBase Visit(Hamburger h) { return new HamburgerViewModel(); }
}

This approach requires to enumerate every model type in the visitor interface and add the Accept method to every model class. If you only have one visitor type this overhead may be unacceptable, and might prefer your existing approach, which you might be able to simplify with the pattern matching features in C#7.

Answer 3

The solution I came up with was in response to JimmyJames's comment. I made a Dictionary<Type, Type> that mapped the two and used reflection to build the map at runtime.

I had to make an interface to get the types using reflection:

public interface IFoodViewModel
{
}

Then I included an abstract class that implemented the interface:

public abstract class FoodViewModelBase<T> : ViewModelBase, IFoodViewModel
   where T : EntityObject
{
}

Then the concrete ViewModel classes become

public HamburgerViewModel : FoodViewModelBase<Hamburger>
{
    // Add properties to bind to
}

public TacoViewModel : FoodViewModelBase<Taco>
{
    // Add properties to bind to
}

In my above example the FoodViewModel needs two more methods and a field added.

First add a dictionary to map them:

Dictionary<Type, Type> _viewModelMapping;

Second a method to build the map called from the constructor in FoodViewModel.

       private void BuildEditViewModelMap()
        {
            _viewModelMapping = new Dictionary<Type, Type>();
            var types = AppDomain.CurrentDomain.GetAssemblies().SelectMany(s => s.GetTypes()).Where(p => typeof(IfoodViewModel).IsAssignableFrom(p) && !p.IsAbstract && !p.IsInterface);
            foreach (var x in types)
            {
                if (x.BaseType.GenericTypeArguments.Count() > 0)
                {
                    _viewModelMapping.Add(x.BaseType.GenericTypeArguments[0], x);
                }
            }
        }

And lastly a method to use the map to build ViewModels

private ViewModelBase GetViewModel(IDeviceModel device)
{
    // Note to add default arguments to create an instance add them to the object[] argument.
   var t = Activator.CreateInstance(_viewModelMapping[device.GetType()], new object[] {}) as ViewModelBase;
   return t;
}

So in conclusion, what does this buy me?

Now if I add a new food type say Salad to my model and need a new ViewModel to go with it, I just have to implement the abstract class FoodViewModelBase and FoodViewModel will handle the rest. I'm happy with this solution. Thanks for the help.

Answer 4

I might be wrong, but why not try a strcture of type dynamic and get rid of these hardcoded case switch variants..