views:

273

answers:

5

I've got a WPF application where PageItems are model objects.

My main ViewModel has an ObservableCollection of PageItemViewModels, each one building itself from its matching PageItem model object.

Each PageItemViewModel inherits from the abstract class BaseViewModel in order to get the INotifyPropertyChanged functionality.

Each PageItemViewModel also implements the IPageItemViewModel in order to make sure it has the needed properties.

I will eventually have around 50 pages so I want to eliminate any unnecessary code:

  • SOLVED (SEE BELOW): is there a way I can get PageItemViewModel classes to inherit IdCode and Title so I don't have to implement them in each class? I can't put them in BaseViewModel since other ViewModels inherit it which don't need these properties, and I can't put them in IPageItemViewModel since it is only an interface. I understand I need multiple inheritance for this which C# doesn't support
  • SOLVED (SEE BELOW): is there a way I can get rid of the switch statement, e.g. somehow use reflection instead?

Below is a stand-alone Console application which demonstrates the code I have in my WPF application:

using System.Collections.Generic;

namespace TestInstantiate838
{
    public class Program
    {
        static void Main(string[] args)
        {
            List<PageItem> pageItems = PageItems.GetAll();
            List<ViewModelBase> pageItemViewModels = new List<ViewModelBase>();

            foreach (PageItem pageItem in pageItems)
            {
                switch (pageItem.IdCode)
                {
                    case "manageCustomers":
                        pageItemViewModels.Add(new PageItemManageCustomersViewModel(pageItem));
                        break;
                    case "manageEmployees":
                        pageItemViewModels.Add(new PageItemManageEmployeesViewModel(pageItem));
                        break;
                    default:
                        break;
                }
            }
        }
    }

    public class PageItemManageCustomersViewModel : ViewModelBase, IPageItemViewModel
    {
        public string IdCode { get; set; }
        public string Title { get; set; }

        public PageItemManageCustomersViewModel(PageItem pageItem)
        {

        }
    }

    public class PageItemManageEmployeesViewModel : ViewModelBase, IPageItemViewModel
    {
        public string IdCode { get; set; }
        public string Title { get; set; }

        public PageItemManageEmployeesViewModel(PageItem pageItem)
        {

        }
    }

    public interface IPageItemViewModel
    {
        //these are the properties which every PageItemViewModel needs
        string IdCode { get; set; }
        string Title { get; set; }
    }

    public abstract class ViewModelBase
    {
        protected void OnPropertyChanged(string propertyName)
        {
            //this is the INotifyPropertyChanged method which all ViewModels need
        }
    }

    public class PageItem
    {
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    public class PageItems
    {
        public static List<PageItem> GetAll()
        {
            List<PageItem> pageItems = new List<PageItem>();
            pageItems.Add(new PageItem { IdCode = "manageCustomers", Title = "ManageCustomers"});
            pageItems.Add(new PageItem { IdCode = "manageEmployees", Title = "ManageEmployees"});
            return pageItems;
        }
    }

}

Refactored: interface changed to abstract class

using System;
using System.Collections.Generic;

namespace TestInstantiate838
{
    public class Program
    {
        static void Main(string[] args)
        {
            List<PageItem> pageItems = PageItems.GetAll();
            List<ViewModelPageItemBase> pageItemViewModels = new List<ViewModelPageItemBase>();

            foreach (PageItem pageItem in pageItems)
            {
                switch (pageItem.IdCode)
                {
                    case "manageCustomers":
                        pageItemViewModels.Add(new PageItemManageCustomersViewModel(pageItem));
                        break;
                    case "manageEmployees":
                        pageItemViewModels.Add(new PageItemManageEmployeesViewModel(pageItem));
                        break;
                    default:
                        break;
                }
            }

            foreach (ViewModelPageItemBase pageItemViewModel in pageItemViewModels)
            {
                System.Console.WriteLine("{0}:{1}", pageItemViewModel.IdCode, pageItemViewModel.Title);
            }
            Console.ReadLine();
        }
    }

    public class PageItemManageCustomersViewModel : ViewModelPageItemBase
    {
        public PageItemManageCustomersViewModel(PageItem pageItem)
        {
            IdCode = pageItem.IdCode;
            Title = pageItem.Title;
        }
    }

    public class PageItemManageEmployeesViewModel : ViewModelPageItemBase
    {
        public PageItemManageEmployeesViewModel(PageItem pageItem)
        {
            IdCode = pageItem.IdCode;
            Title = pageItem.Title;
        }
    }

    public abstract class ViewModelPageItemBase : ViewModelBase
    {
        //these are the properties which every PageItemViewModel needs
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    public abstract class ViewModelBase
    {
        protected void OnPropertyChanged(string propertyName)
        {
            //this is the INotifyPropertyChanged method which all ViewModels need
        }
    }

    public class PageItem
    {
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    public class PageItems
    {
        public static List<PageItem> GetAll()
        {
            List<PageItem> pageItems = new List<PageItem>();
            pageItems.Add(new PageItem { IdCode = "manageCustomers", Title = "ManageCustomers"});
            pageItems.Add(new PageItem { IdCode = "manageEmployees", Title = "ManageEmployees"});
            return pageItems;
        }
    }

}

Answer to eliminating Switch statement:

Thanks Jab:

string assemblyName = System.Reflection.Assembly.GetExecutingAssembly().GetName().Name;
string viewModelName = assemblyName + ".ViewModels.PageItem" + StringHelpers.ForcePascalNotation(pageItem.IdCode) + "ViewModel";
var type = Type.GetType(viewModelName);
var viewModel = Activator.CreateInstance(type, pageItem) as ViewModelBase;
AllPageViewModels.Add(viewModel);
+2  A: 

Can you create a class that inherits from BaseViewModel that will implement these two properties - your PageItemViewModel classes that need this could then inherit from that.

Paddy
thanks that works, I included solution above, I would still like to get rid of the switch statement, is there any way to instantiate objects dynamically which inherit a common abstract class?
Edward Tanguay
+1  A: 

As suggested by Paddy, I just created an additional abstract class, PageViewModelBase, with those auto-props defined:

using System.Collections.Generic;

namespace TestInstantiate838
{
    public class Program
    {
        static void Main(string[] args)
        {
            List<PageItem> pageItems = PageItems.GetAll();
            List<ViewModelBase> pageItemViewModels = new List<ViewModelBase>();

            foreach (PageItem pageItem in pageItems)
            {
                switch (pageItem.IdCode)
                {
                    case "manageCustomers":
                        pageItemViewModels.Add(new PageItemManageCustomersViewModel(pageItem));
                        break;
                    case "manageEmployees":
                        pageItemViewModels.Add(new PageItemManageEmployeesViewModel(pageItem));
                        break;
                    default:
                        break;
                }
            }
        }
    }

    public class PageItemManageCustomersViewModel : PageViewModelBase
    {
        public PageItemManageCustomersViewModel(PageItem pageItem)
        {

        }
    }

    public class PageItemManageEmployeesViewModel : PageViewModelBase
    {
        public PageItemManageEmployeesViewModel(PageItem pageItem)
        {


        }
    }

    public abstract class ViewModelBase
    {
        protected void OnPropertyChanged(string propertyName)
        {
            //this is the INotifyPropertyChanged method which all ViewModels need
        }
    }

    public abstract class PageViewModelBase : ViewModelBase
    {
        //these are the properties which every PageItemViewModel needs
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    public class PageItem
    {
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    public class PageItems
    {
        public static List<PageItem> GetAll()
        {
            List<PageItem> pageItems = new List<PageItem>();
            pageItems.Add(new PageItem { IdCode = "manageCustomers", Title = "ManageCustomers"});
            pageItems.Add(new PageItem { IdCode = "manageEmployees", Title = "ManageEmployees"});
            return pageItems;
        }
    }

}
Matthew Flaschen
How many things are you liable to have in the switch? How likely are they to be dynamically added? For me, removing this makes it difficult to read your way through the code for maintenance, while providing not very much in return.
Paddy
A: 

Why can't you put a GetViewModel() virtual method in your base PageItem class which returns the appropriate view model?

   foreach (PageItem pageItem in pageItems)
   {
       pageItemViewModels.Add(pageItem.GetViewModel());
   }

The thing that immediately looks like a code smell is the usage of "id" properties - this can usually be replaced with polymorphism. So you would replace the switch statement with the code above.

Edit:

If your PageItem class knows nothing about your view model, then it can not be implemented this way. Basically, you need a factory, which you already have (in a way).

I usually have a list of relations (PageItem to ViewModel), which would in your case be a Dictionary<String, Type>. Then you can fill this list during initialization and have the proper view model instantiated later.

To use reflection to build this list, you at least need to know programmatically which page item is supported by a view model. For that purpose, you could use a custom attribute to decorate you class, eg.:

public class SupportsPageItemAttribute : Attribute
{
    private readonly string _id;
    public string ID
    {
     get { return _id;}
    }

    public SupportsPageItemAttribute(string id)
    {
        _id = id;
    }
}

And then use that attribute to define which PageItem your model can accept:

[SupportsPageItemAttribute("manageCustomers")
public class PageItemManageCustomersViewModel
{
   // ...
}

And then, you use reflection to get all classes implementing IPageItemViewModel and check their attributes to get the PageItem id string.

For example (without much error-checking):

Dictionary<String, Type> modelsById = new Dictionary<String, Type>();
String viewModelInterface = typeof(IPageItemViewModel).FullName;

// get the assembly
Assembly assembly = Assembly.GetAssembly(typeof(IPageItemViewModel));

// iterate through all types
foreach (Type viewModel in assembly.GetTypes())
{
    // get classes which implement IPageItemViewModel
    if (viewModel.GetInterface(viewModelInterface) != null)
    {
        // get the attribute we're interested in
        foreach (Attribute att in Attribute.GetCustomAttributes(viewModel))
        {
            if (att is SupportsPageItemAttribute)
            {
                // get the page item id
                String id = (att as SupportsPageItemAttribute).ID;

                // add to dictionary
                modelsById.Add(id, viewModel);
            }
        }
    }
}

On the other hand, there are various Inversion-of-control frameworks you might consider instead of doing the nasty reflection work yourself.

Groo
right but the PageItem class is my model class which should know nothing about "ViewModels" up in the MVVM pattern, the model could also be used for e.g. ASP.NET MVC and shouldn't be returning a "ViewModel".
Edward Tanguay
+1  A: 

One solution that isn't very pretty, but works, would be to use convention to get rid of the switch statement. This assumes you can change the IdCodes or atleast modify the case to match the ViewModel.

    var type = Type.GetType("PageItem" + pageItem.IdCode + "ViewModel");
    var viewModel = Activator.CreateInstance(type) as ViewModelBase;
    pageItemViewModels.Add(viewModel);

Note that you should add error checking here, there are a couple points of failure here. It is, however, better than having to maintain an ever-growing switch statement.

Jab
this is just the pragmatic approach I was looking for, I had to tweek the code a bit which I posted above, thanks!
Edward Tanguay
Glad you like it. Though I would lean toward Mike Spross's solution long term. The responsibility is clearer there.
Jab
I must say I don't agree with the answer that much. It is pragmatic no doubt, but not a very common approach in production code.
Groo
+1  A: 

One possible solution is to reverse the relationship between PageItem and PageItemViewModel in your code. Right now, you are generating a PageItemViewModel based on a PageItem, but what if you created the PageItemViewModels first and then in each PageItemViewModel's constructor, you created the appropriate PageItem? This eliminates the need for the switch and makes things cleaner, because now your view-model is responsible for the model, instead of the model being responsible for the view-model.

An example based on your current code:

using System;
using System.Collections.Generic;

namespace TestInstantiate838
{
    public class Program
    {
        static void Main(string[] args)
        {
            List<ViewModelPageItemBase> pageItemViewModels = PageItemViewModels.GetAll();

            // No switch needed anymore. Each PageItem's view-model contains its PageItem
            // which is exposed as property of the view-model.
            foreach (ViewModelPageItemBase pageItemViewModel in pageItemViewModels)
            {
                System.Console.WriteLine("{0}:{1}", pageItemViewModel.PageItem.IdCode, pageItemViewModel.PageItem.Title);
            }
            Console.ReadLine();
        }
    }

    public class PageItemManageCustomersViewModel : ViewModelPageItemBase
    {
        public PageItemManageCustomersViewModel()
        {
            PageItem = new PageItem { IdCode = "manageCustomers", Title = "ManageCustomers" };
        }
    }

    public class PageItemManageEmployeesViewModel : ViewModelPageItemBase
    {
        public PageItemManageEmployeesViewModel()
        {
            PageItem = new PageItem { IdCode = "manageEmployees", Title = "ManageEmployees" };
        }
    }

    public abstract class ViewModelPageItemBase : ViewModelBase
    {
        //The PageItem associated with this view-model
        public PageItem PageItem { get; protected set; }
    }

    public abstract class ViewModelBase
    {
        protected void OnPropertyChanged(string propertyName)
        {
            //this is the INotifyPropertyChanged method which all ViewModels need
        }
    }

    public class PageItem
    {
        public string IdCode { get; set; }
        public string Title { get; set; }
    }

    // Replaces PageItems class
    public class PageItemViewModels
    {
        // Return a list of PageItemViewModel's instead of PageItem's.
        // Each PageItemViewModel knows how to build it's corresponding PageItem object.
        public static List<PageItemViewModelBase> GetAll()
        {
            List<PageItemViewModelBase> pageItemViewModels = new List<PageItemViewModelBase>();
            pageItemViewModels.Add(new PageItemManageCustomersViewModel());
            pageItemViewModels.Add(new PageItemManageEmployeesViewModel());
            return pageItemViewModels;
        }
    }
}
Mike Spross