MVVM - UI related code in View model - true separation of concerns

Question

I started implementing a MVVM design pattern in an existing WPF c# application. I am completely new and have never used design patterns or dependency injection before. I was looking at the frameworks already available and have adopted MVVM light. I moved the logic from the view to the viewmodel. I have lot of code in the PopulateTestMenu which is related to UI in the view model. It also has calls to the event handlers. How do I take care of this?

In the XAML I have:

<Window DataContext="{Binding Main, Source={StaticResource Locator}}">
    <Menu>
       <MenuItem Header="Load All History..." Command="{Binding LoadAllHistory}">

In the MainViewModel class I have:

public ICommand LoadAllHistory { get; private set; }

public MainViewModel()
{            
    LoadAllHistory = new RelayCommand(() => LoadHistoryExecute(), () => true);
}

The code that I moved from my view to the viewmodel:

private void LoadHistoryExecute()
{
    try
    {
        OpenFileDialog ofd = new OpenFileDialog();
        ofd.Filter = "Test History File (*.xml)|*.xml";
        ofd.Title = "Open Test History";
        ofd.Multiselect = true;

        if (ofd.ShowDialog() == true)
        {
            ThreadPool.QueueUserWorkItem(LoadTestHistoryCallback, ofd.FileNames);
        }
    }
    catch
    {
       //some code
    }
}

private void LoadTestHistoryCallback(object state)
{
    try
    {
        string[] fileNames = (string[])state;

        foreach (string fileName in fileNames)
        {
            bool success = MyApp.Instance.ParseTestHistory(fileName);
            string status = success
                          ? String.Format("'{0}' loaded successfully.",      
               System.IO.Path.GetFileName(fileName))
                          : String.Format("Failed to load history from '{0}'.",    
               System.IO.Path.GetFileName(fileName));
            Dispatcher.CurrentDispatcher.DynamicInvoke(delegate()
            {
                Status = status;
            });
            PopulateTestMenu(new SortedList<int, int>());
        }
    }
    catch
    {
        //some code
    }
}

private void PopulateTestMenu(SortedList<int, int> indexes)
{
    try
    {
        _testMenuMutex.WaitOne();

        //Populate the Tests menu with the list of tests.
        Dispatcher.CurrentDispatcher.DynamicInvoke(delegate()
        {
            menuTests.Items.Clear();
            var checkEventHandler = new RoutedEventHandler(testMenuItem_Checked);
            bool added = false;

            if (MyApp.Instance.TestHistory != null && 
               MyApp.Instance.TestHistory.Count > 0)
            {
                List<ushort> subIds = new 
                   List<ushort>MyApp.Instance.TestHistory.Keys);

                foreach (ushort subId in subIds)
                {
                   MenuItem menuItem = null;
                    menuItem = new MenuItem();
                    menuItem.Header = subId.ToString().PadLeft(5, '0');**

                    MenuItem none = new MenuItem();
                    none.Header = "None";
                    none.IsCheckable = true;
                    none.IsChecked = true;
                    none.Checked += checkEventHandler;
                    none.Unchecked += checkEventHandler;

                    menuItem.Items.Add(none);

                    if (MyApp.Instance.TestHistory != null && 
                       MyApp.Instance.TestHistory.ContainsKey(subId))
                    {
                        var tests = MyApp.Instance.TestHistory[subId];

                        if (tests != null)
                        {
                            foreach (Test t in tests)
                            {
                                MenuItem item = new MenuItem();
                                item.IsCheckable = true;

                                string description = t.Description.Replace("\n", 
                                "\n".PadRight(34, ' '));
                                string header = abc;
                                item.Header = header;
                                item.DataContext = t;
                                item.Checked += checkEventHandler;
                                item.Unchecked += checkEventHandler;
                                menuItem.Items.Add(item);
                            }
                            if (tests.Count > 0)
                            {
                                menuTests.Items.Add(menuItem);
                                added = true;
                            }
                        }
                    }

                    // Carry over the previous selection.
                    if (indexes.ContainsKey(subId) && indexes[subId] > -1)
                    {                            ((MenuItem)menuItem.Items[indexes[subId]]).IsChecked = 
                         true;
                    }
                }
            }

look into Item binding, you could just bind to a list of items instead of populating menu in one by one — Steve
No. A VM would not be creating MenuItems because it breaks the abstraction principle and deals with concrete objects on the UI surface. The VM instead can create the collection of items which is bound to the menu. Create a menu in Xaml and bind its ItemsSource property to a collection! — Gayot Fow

silverfighter silverfighter · Accepted Answer · 2014-04-16T15:31:57

I am still trying to figure out what you are asking =)...

But you are mixing up some things... Remember one of the core concepts of MVVM is to make the viewmodel testable and remove all view related code off from the viewmodel. So no dependencies to WPF at all. So MenuItem looks like a WPF MenuItem and should not be in your ViewModel.

Instead you could consider to make a MenuItemViewModel which binds to the MenuItem in the View. And it I could see an ObservableCollection<MenuItemViewModel> TestMenu instead of your sorted list.

In your method LoadTestHistoryCallback you would instanciate (could be done via DI) the MenuItemViewModel and add it to the TestMenu Collection. The MenuItemViewModel could have status property which could be assigned from outside or internaly. (It can also have some additional logic, hey its a viewmodel).

In the View you could then bind it to a list with a template representing the MenuItem via DataBinding.

<Menu DockPanel.Dock="Top" ItemsSource="{Binding Path=MenuItems}" />

So remember ViewModel can also contain ViewModels or collections of viewmodel. Use the rich databinding api from WPF. Work with bindable Properties like ObservebaleCollections or Properties that are extended with PropertyChanged notification.

HTH

P.S: You can then have a click ICommand in the MenuItemViewModel and execute actions or better use the EventAggregator or Messenger to notify other ViewModels ...(but that's a story for another question =)... )

MVVM - UI related code in View model - true separation of concerns

2 Answers