4
\$\begingroup\$

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:

<Grid Background="White">
    <Grid.RowDefinitions>
        <RowDefinition Height="100" x:Name="TitleRow"/>
        <RowDefinition Height="*" x:Name="DataRow"/>
    </Grid.RowDefinitions>
    <Grid.ColumnDefinitions>
        <ColumnDefinition Width="300" x:Name="ItemsColumn"/>
        <ColumnDefinition Width="*" x:Name="DataColumn"/>
    </Grid.ColumnDefinitions>

    <ListBox Background="WhiteSmoke" Name="Items" Grid.Column="0" Grid.RowSpan="2" ItemsSource="{Binding ItemTitles}" SelectionChanged="OnSelectionChanged"
             Margin="-2,-2,0,-2" Padding="0,10" />

    <Viewbox Name="TitleView" Margin="10" VerticalAlignment="Center" HorizontalAlignment="Left" Grid.Column="1">
        <TextBlock Name="TitleText" Foreground="Black" Margin="5,10,5,5"
                   Text="{Binding SelectedValue, ElementName=Items}" />
    </Viewbox>

    <Border Grid.Column="1" BorderBrush="Black" BorderThickness="0,1,0,0" VerticalAlignment="Bottom" Height="1"/>

    <Frame Grid.Column="1" Grid.Row="1" Foreground="Black" FontSize="20" Margin="20,20,0,20" Name="DataFrame" VerticalAlignment="Top" />
</Grid>

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; }
                    ItemList.Insert(ItemTitles.IndexOf("Menu 2") + 1, m);
                    ItemTitles.Insert(ItemTitles.IndexOf("Menu 2") + 1, m.Title);
                }
                break;

            case "Menu 3":

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

            case "Menu 4":

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

And VMProperties.cs (a continuation of ViewModel.cs):

public partial class ViewModel : INotifyPropertyChanged
{
    private ObservableCollection<MenuItem> _itemList = new ObservableCollection<MenuItem>();
    public ObservableCollection<MenuItem> ItemList
    {
        get { return _itemList; }
        set { _itemList = value; }
    }

    private ObservableCollection<string> _itemTitles = new ObservableCollection<string>();
    public ObservableCollection<string> ItemTitles
    {
        get { return _itemTitles; }
        set
        {
            if (_itemTitles == value) return;
            _itemTitles = value;
            OnPropertyChanged();
        }
    }

    public event PropertyChangedEventHandler PropertyChanged;
    protected virtual void OnPropertyChanged(string propertyName = null)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null)
        {
            handler(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}

This is Menus.cs:

public class MenuItem
{
    public MenuItem(string Title, Type Page)
    {
        _title = Title;
        _page = Page;
    }

    private string _title = "";
    public string Title
    {
        get { return _title; }
        set { Title = value; }
    }

    private Type _page = null;
    public Type Page
    {
        get { return _page; }
        set { _page = value; }
    }
}

public class Menu
{
    public Menu() { }

    public void Add(MenuItem m)
    {
        Menus.Add(m);
    }

    private ObservableCollection<MenuItem> _menus = new ObservableCollection<MenuItem>();
    public ObservableCollection<MenuItem> Menus
    {
        get { return _menus; }
        set { _menus = value; }
    }
}

And this is GlobalVars.cs:

class GlobalVars
{
    public static void InitMenus()
    {
        var resourceFile = new Windows.ApplicationModel.Resources.ResourceLoader();

        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)));

        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)));

        Menu3.Add(new MenuItem(resourceFile.GetString("Menu 3"), typeof(Menus.Menu3)));
        Menu3.Add(new MenuItem(resourceFile.GetString("Submenu 5"), typeof(Menus.MenuItems.Submenu5)));

        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.GetString("Submenu 8"), typeof(Menus.MenuItems.Submenu8)));
        Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 7"), typeof(Menus.MenuItems.Submenu7)));
        Menu4.Add(new MenuItem(resourceFile.GetString("Submenu 6"), typeof(Menus.MenuItems.Submenu6)));
    }

    private static Menu _menu1 = new Menu();
    public static Menu Menu1
    {
        get { return _menu1; }
    }

    private static Menu _menu2 = new Menu();
    public static Menu Menu2
    {
        get { return _menu2; }
    }

    private static Menu _menu3 = new Menu();
    public static Menu Menu3
    {
        get { return _menu3; }
    }

    private static Menu _menu4 = new Menu();
    public static Menu Menu4
    {
        get { return _menu4; }
    }
}

Extension.cs contains the code for ObservableCollection.RemoveAll(), taken from SO.

These are the values in Resources.resw:

Menu 1         Menu 1
Menu 10        Menu 10
Menu 11        Menu 11
Menu 12        Menu 12
Menu 13        Menu 13
Menu 14        Menu 14
Menu 2         Menu 2
Menu 3         Menu 3
Menu 4         Menu 4
Menu 5         Menu 5
Menu 6         Menu 6
Menu 7         Menu 7
Menu 8         Menu 8
Menu 9         Menu 9
Submenu 1      Submenu 1
Submenu 10     Submenu 10
Submenu 2      Submenu 2
Submenu 3      Submenu 3
Submenu 4      Submenu 4
Submenu 5      Submenu 5
Submenu 6      Submenu 6
Submenu 7      Submenu 7
Submenu 8      Submenu 8
Submenu 9      Submenu 9

I would like my navigation, use of MVVM, and everything else reviewed. I hate the way I have to have ItemsTitle separate from ItemList, but my ListBox would not update in response to my clicks any other way. I am not at all sure I am doing this right, and that is why I want it reviewed. I do want to keep one ObservableCollection or some other collection type per menu so I don't have to iterate over hundreds of values each time I need to insert a new list. I know I can't have menus inside menus, but I don't want that feature now anyway.

\$\endgroup\$
4
\$\begingroup\$

A few things I noticed:

  1. 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.

  2. 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<MenuItem> 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;
        ...
    }
    
  3. 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<MenuItem> GetMainMenus();
    IEnumerable<MenuItem> 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<string, Menu> _Menus = new Dictionary<string, Menu>();

    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.GetString("Submenu 8"), typeof(Menus.MenuItems.Submenu8)));
        menu4.Add(new MenuItem(resourceFile.GetString("Submenu 7"), typeof(Menus.MenuItems.Submenu7)));
        menu4.Add(new MenuItem(resourceFile.GetString("Submenu 6"), typeof(Menus.MenuItems.Submenu6)));
        _Menus["Menu 4"] = menu4;
    }

    IEnumerable<MenuItem> GetMainMenus()
    {
        return _Menus.Select(kvp => kvp.Value.Menus.First());
    }

    IEnumerable<MenuItem> GetSubMenus(string menuName)
    {
        return _Menus[menuName].Menus.Skip(1); // skip main menu
    }
}
\$\endgroup\$
  • \$\begingroup\$ @Hosch250: Well, I'd re-implement GlobalVars as a ResourceMenuProvider - see added example implementation. \$\endgroup\$ – ChrisWue Jan 4 '15 at 1:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.