MVVM, Navigation, and More

Continuation of Correct MVVM format. This is what I have done so far, and of course it works:

To start with, this is my MainPage.xaml:























This is the code-behind - MainPage.xaml.cs:

public sealed partial class MainPage : Page
{
private ViewModel Data = new ViewModel();

public MainPage()
{
this.InitializeComponent();
this.DataContext = Data;

Items.SelectedItem = "Menu 1";
}

private void OnSelectionChanged(object sender, SelectionChangedEventArgs e)
{
Data.SelectionChanged(Items.SelectedValue.ToString());
DataFrame.Navigate(Data.ItemList[Items.SelectedIndex].Page);
}
}


Here is ViewModel.cs:

`public partial class ViewModel : INotifyPropertyChanged
{
public ViewModel()
{
GlobalVars.InitMenus();

ItemList.Add(GlobalVars.Menu1.Menus[0]);
ItemList.Add(GlobalVars.Menu2.Menus[0]);
ItemList.Add(GlobalVars.Menu3.Menus[0]);
ItemList.Add(GlobalVars.Menu4.Menus[0]);

foreach (MenuItem m in ItemList) ItemTitles.Add(m.Title);
}

public void SelectionChanged(string newSelection)
{
if (!newSelection.StartsWith(" "))
{
ItemList.RemoveAll(x => x.Title.StartsWith(" "));
ItemTitles.RemoveAll(x => x.StartsWith(" "));
}

switch (newSelection)
{
case "Menu 1":

foreach(MenuItem m in GlobalVars.Menu1.Menus)
{
if (!m.Title.StartsWith(" ")) continue;
ItemList.Insert(ItemTitles.IndexOf("Menu 1") + 1, m);
ItemTitles.Insert(ItemTitles.IndexOf("Menu 1") + 1, m.Title);
}
break;

case "Menu 2":

foreach(MenuItem m in GlobalVars.Menu2.Menus)
{
if (!m.Title.StartsWith(" ")) { continue; }

A few things I noticed:

-
You should have a property on your ViewModel which represents the SelectedItem as well as the SelectedPage. In your MainPage you would bind then the SelectedItem property of the ListBox to the view model property and the Source of the Frame to the SelectedPage property. This way when someone selects a new item you can handle the change entirely inside the view model and getting rid of the code-behind in your MainPage. The advantage is that you can easily unit test the behaviour of menu selection.

-
Your handling code for the different menus inside the ViewModel is repeating code at it's best. This code block:

foreach(MenuItem m in GlobalVars.Menu1.Menus)
        {
            if (!m.Title.StartsWith(" ")) continue;
            ItemList.Insert(ItemTitles.IndexOf("Menu 1") + 1, m);
            ItemTitles.Insert(ItemTitles.IndexOf("Menu 1") + 1, m.Title);
        }

can be generalized into a method:

private void HandleMenuSelection(string menu, IList menuItems)
{
    foreach (var m in menuItems)
    {
        if (!m.Title.StartsWith(" ")) continue;
        ItemList.Insert(ItemTitles.IndexOf(menu) + 1, m);
        ItemTitles.Insert(ItemTitles.IndexOf(menu) + 1, m.Title);  
    }          
}

in which case your switch would start reading like:

switch (newSelection)
{
    case "Menu 1": HandleMenuSelection(newSelection, GlobalVars.Menu1.Menus); break;
    case "Menu 2": HandleMenuSelection(newSelection, GlobalVars.Menu2.Menus); break;
    ...
}

-
I strongly dislike the GlobalVars class. There is an implicit dependency from the view model to it via the Menu 1, Menu 2, etc. magic strings which is not easily visible and makes the code brittle. It would be better to create an interfacelike an IMenuSource which can be queried for the different menus. This will make unit testing the code easier as well.

Update To clarify on point 3 above.

I would consider creating an interface which provides the required information about the available menus. Something along these lines:

public interface IMenuProvider
{
    IEnumerable GetMainMenus();
    IEnumerable GetSubMenus(string menuName);
}

which you can then implement accordingly and inject into your ViewModel which would then look more like this:

public partial class ViewModel : INotifyPropertyChanged
{
    private IMenuProvider _MenuProvider;

    public ViewModel(IMenuProvider menuProvider)
    {
        ItemList.AddRange(menuProvider.GetMainMenus());

        foreach (MenuItem m in ItemList) ItemTitles.Add(m.Title);
    }

    public void SelectionChanged(string newSelection)
    {
        if (!newSelection.StartsWith(" "))
        {
            ItemList.RemoveAll(x => x.Title.StartsWith(" "));
            ItemTitles.RemoveAll(x => x.StartsWith(" "));
        }

        foreach (var m in menuProvider.GetSubMenus(newSelection))
        {
            if (!m.Title.StartsWith(" ")) continue;
            ItemList.Insert(ItemTitles.IndexOf(newSelection) + 1, m);
            ItemTitles.Insert(ItemTitles.IndexOf(newSelection) + 1, m.Title);
        }
    }
}

It has also the advantage that the application code doesn't have to know what menus exist so they could easily come from a database or other configuration file. When adding a new menu no code has to change.

Example implementation could be:

```
public class ResourceMenuProvider : IMenuProvider
{
private static Dictionary _Menus = new Dictionary();

static ResourceMenuProvider()
{
var resourceFile = new Windows.ApplicationModel.Resources.ResourceLoader();

var menu1 = new Menu();
menu1.Add(new MenuItem(resourceFile.GetString("Menu 1"), typeof(Menus.Menu1)));
menu1.Add(new MenuItem(resourceFile.GetString("Submenu 2"), typeof(Menus.MenuItems.Submenu2)));
menu1.Add(new MenuItem(resourceFile.GetString("Submenu 1"), typeof(Menus.MenuItems.Submenu1)));
_Menus["Menu 1"] = menu1;

var menu2 = new Menu();
menu2.Add(new MenuItem(resourceFile.GetString("Menu 2"), typeof(Menus.Menu2)));
menu2.Add(new MenuItem(resourceFile.GetString("Submenu 4"), typeof(Menus.MenuItems.Submenu4)));
menu2.Add(new MenuItem(resourceFile.GetString("Submenu 3"), typeof(Menus.MenuItems.Submenu3)));
_Menus["Menu 2"] = menu2;

var menu3 = new Menu();
menu3.Add(new MenuItem(resourceFile.GetString("Menu 3"), typeof(Menus.Menu3)));
menu3.Add(new MenuItem(resourceFile.GetString("Submenu 5"), typeof(Menus.MenuItems.Submenu5)));
_Menus["Menu 3"] = menu3;

var menu4 = new Menu();
menu4.Add(new MenuItem(resourceFile.GetString("Menu 4"), typeof(Menus.Menu4)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 10"), typeof(Menus.MenuItems.Submenu10)));
menu4.Add(new MenuItem(resourceFile.GetString("Submenu 9"), typeof(Menus.MenuItems.Submenu9)));
menu4.Add(new MenuItem(resourceFile.GetStri

StackExchange Code Review Q#75532, answer score: 4