MVVM, Navigation, and More - Part 3

Question

This is actually a second follow-up to Correct MVVM format, but I have made different changes.

This is my ViewModel.cs class for my MainPage.xaml:

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

    private ObservableCollection<string> _back1 = new ObservableCollection<string>();
    public ObservableCollection<string> Back1
    {
        get { return _back1; }
        set { _back1 = value; }
    }

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

    private string _currentTitle = "Menu 1";
    public string CurrentTitle
    {
        get { return _currentTitle; }
        set
        {
            _currentTitle = value;
            OnPropertyChanged();
        }
    }

    private MenuItem _currentItem = new MenuItem("Menu 1", typeof(Menus.Menu1), AvailWSMenu.Menu1);
    public MenuItem CurrentItem
    {
        get { return _currentItem; }
        set
        {
            _currentItem = value;
        }
    }

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

    public Windows.ApplicationModel.Resources.ResourceLoader resourceFile = new Windows.ApplicationModel.Resources.ResourceLoader();

    public ViewModel()
    {
        ItemList.Add(new MenuItem(resourceFile.GetString("Menu 1"), typeof(Menus.Menu1), AvailWSMenu.Menu1));
        ItemList.Add(new MenuItem(resourceFile.GetString("Menu 2"), typeof(Menus.Menu2), AvailWSMenu.Menu2));
        ItemList.Add(new MenuItem(resourceFile.GetString("Menu 3"), typeof(Menus.Menu3), AvailWSMenu.Menu3));
        ItemList.Add(new MenuItem(resourceFile.GetString("Menu 4"), typeof(Menus.Menu4), AvailWSMenu.Menu4));

        ItemTitles = new ObservableCollection<string>(ItemList.Select(x => x.Title));
    }

    public void SelectionChanged(string newSelection)
    {
        Back.Insert(0, new MenuItem(newSelection, ItemList[ItemTitles.IndexOf(newSelection)].Page, ItemList[ItemTitles.IndexOf(newSelection)].Menu));

        if (newSelection.StartsWith(" ")) return;

        ItemList.RemoveAll(_ => _.Title.StartsWith(" "));
        ItemTitles.RemoveAll(_ => _.StartsWith(" "));

        switch (newSelection)
        {
            case "Menu 1":
                ItemList.Insert(1, (new MenuItem(resourceFile.GetString("Submenu 2"), typeof(Menus.MenuItems.Submenu2), AvailWSMenu.Menu1)));
                ItemList.Insert(1, (new MenuItem(resourceFile.GetString("Submenu 1"), typeof(Menus.MenuItems.Submenu1), AvailWSMenu.Menu1)));

                ItemTitles.Insert(1, resourceFile.GetString("Submenu 2"));
                ItemTitles.Insert(1, resourceFile.GetString("Submenu 1"));
                break;

            case "Menu 2":
                ItemList.Insert(2, (new MenuItem(resourceFile.GetString("Submenu 4"), typeof(Menus.MenuItems.Submenu4), AvailWSMenu.Menu2)));
                ItemList.Insert(2, (new MenuItem(resourceFile.GetString("Submenu 3"), typeof(Menus.MenuItems.Submenu3), AvailWSMenu.Menu2)));

                ItemTitles.Insert(2, resourceFile.GetString("Submenu 4"));
                ItemTitles.Insert(2, resourceFile.GetString("Submenu 3"));
                break;

            case "Menu 3":
                ItemList.Insert(3, (new MenuItem(resourceFile.GetString("Submenu 5"), typeof(Menus.MenuItems.Submenu5), AvailWSMenu.Menu3)));

                ItemTitles.Insert(3, resourceFile.GetString("Submenu 5"));
                break;

            case "Menu 4":
                ItemList.Insert(4, (new MenuItem(resourceFile.GetString("Submenu 10"), typeof(Menus.MenuItems.Submenu10), AvailWSMenu.Menu4)));
                ItemList.Insert(4, (new MenuItem(resourceFile.GetString("Submenu 9"), typeof(Menus.MenuItems.Submenu9), AvailWSMenu.Menu4)));
                ItemList.Insert(4, (new MenuItem(resourceFile.GetString("Submenu 8"), typeof(Menus.MenuItems.Submenu8), AvailWSMenu.Menu4)));
                ItemList.Insert(4, (new MenuItem(resourceFile.GetString("Submenu 7"), typeof(Menus.MenuItems.Submenu7), AvailWSMenu.Menu4)));
                ItemList.Insert(4, (new MenuItem(resourceFile.GetString("Submenu 6"), typeof(Menus.MenuItems.Submenu6), AvailWSMenu.Menu4)));

                ItemTitles.Insert(4, resourceFile.GetString("Submenu 10"));
                ItemTitles.Insert(4, resourceFile.GetString("Submenu 9"));
                ItemTitles.Insert(4, resourceFile.GetString("Submenu 8"));
                ItemTitles.Insert(4, resourceFile.GetString("Submenu 7"));
                ItemTitles.Insert(4, resourceFile.GetString("Submenu 6"));
                break;
        }
    }

    public void GoBack()
    {
        if (Back.Count == 1) return;

        if (Back[1].Title.StartsWith(" ") && Back[0].Menu != Back[1].Menu)
        {
            Back.RemoveAt(0);

            switch (Back[1].Menu)
            {
                case AvailWSMenu.Menu1:
                    CurrentTitle = resourceFile.GetString("Menu 1");
                    break;

                case AvailWSMenu.Menu2:
                    CurrentTitle = resourceFile.GetString("Menu 2");
                    break;

                case AvailWSMenu.Menu3:
                    CurrentTitle = resourceFile.GetString("Menu 3");
                    break;

                case AvailWSMenu.Menu4:
                    CurrentTitle = resourceFile.GetString("Menu 4");
                    break;
            }

        }

        Back.RemoveAt(0);

        CurrentTitle = Back[0].Title;

        Back.RemoveAt(0);
    }
}

My MenuItem.cs contains two things, an enum AvailMenus and a class MenuItem:

public enum AvailMenus
{
    Menu1, Menu2, Menu3, Menu4, MenuItem
}

public class MenuItem
{
    public MenuItem(string title, Type page, AvailMenus menu)
    {
        Title = title;
        Page = page;
        Menu = menu;
    }

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

    private AvailMenus _menu = new AvailMenus();
    public AvailMenus Menu
    {
        get { return _menu; }
        set { _menu = value; }
    }
}

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" SelectedValue="{Binding CurrentTitle, Mode=TwoWay}" />

    <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>
<Page.BottomAppBar>
    <CommandBar>
        <AppBarButton Label="Back" Icon="Back" Click="AppBarButton_Click"/>
    </CommandBar>
</Page.BottomAppBar>

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

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

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

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

    private void AppBarButton_Click(object sender, RoutedEventArgs e)
    {
        Data.GoBack();
    }
}

This is the RemoveAll extension for ObservableCollection:

public static class ExtensionMethods
{
    public static int RemoveAll<T>(this ObservableCollection<T> coll, Func<T, bool> condition)
    {
        var itemsToRemove = coll.Where(condition).ToList();

        foreach (var itemToRemove in itemsToRemove)
        {
            coll.Remove(itemToRemove);
        }

        return itemsToRemove.Count;
    }
}

The one thing I really dislike is maintaining the two ObservableCollections ItemList and ItemTitles, but I haven't been able to get it to work any other way. I would like my use of MVVM reviewed in particular, as well as my method of handling navigation and everything else.

apieceoffruit · Accepted Answer · 2015-01-07 12:45:53Z

You are using this a lot:

 ItemList.Add(new MenuItem(resourceFile.GetString("Menu 1"), typeof(Menus.Menu1), AvailWSMenu.Menu1));

and also:

ItemList.Insert(2, (new MenuItem(resourceFile.GetString("Submenu 4"), typeof(Menus.MenuItems.Submenu4), AvailWSMenu.Menu2)));

It Might Make more sense to create a:

MenuItem CreateMenuItem(string resourceName,Type type, AvailMenus item){}

then in your add and insert can both use it. Why bother? Well say you want to add a setting to each and every one of them later, or you change the way you construct one e.g change out the resource file call. it should be abstracted.

In the interest of saving you having a lot of messy constructors also using the Property initializer syntax might be cleaner.

   new MenuItem
   {
     Title = resourceFile.GetString(Submenu4String),
     Menu = typeof(Menus.MenuItems.Submenu4String),
     AvailMenus = AvailWSMenu.Menu2
   };

Of course if any of those properties are required then the constructor is the way to go, but the lack of null validation makes me think they are not

Also there is a lot of magic strings going on. At minimum, you should make those constants. I would personally just do an enum but that is a design choice:

const string Menu1String = "Menu1";
const string Menu2String = "Menu2";

May seem superfluous but when it comes to debugging it makes things alot easier than finding that one place you wrote "Memu1"

Also have a think about your viemodels goals. It is effectively providing data to the view and allowing a select, right?

In the interest of only exposing the abilities I want i prefer to make an interface for my ViewModel

interface IMainViewModel
{

  ObservableCollection<MenuItem> Back {get; }
  ObservableCollection<MenuItem> Back1 {get; }
  ObservableCollection<MenuItem> ItemList {get; }

  string CurrentTitle{ get; set; }
  MenuItem CurrentItem { get; set; }

  string SelectedValue{ set; }
  ICommand UpdateSelection{ get; }
  ICommand GoBack{ get; }
}

having a public set exposed the possibility of the View completely changing the Observable collection or setting it null. should that ever happen?

Also using ICommand instead of calling methods directly is the MVVM way, you can then bind it directly to things like buttons

 Command="{Binding GoBack}"

which has some nice features like auto deactivating the button when the CanExecute fails. and having the ability to have a canexecute is nice. Good separation of concerns.

Also if you decide down the line to maybe go fill ViewModelLocator using the interface as a binding in your view allows some hot swapping of views/models.

So what i mean by that is

  IMainViewModel Vm { get { return DataContext as IMainViewModel } }

in your view means if you want to swap out logic/models you dont have to change references tied directly to the model instance.

   private void OnSelectionChanged(object sender, SelectionChangedEventArgs e)
   {
        if(Vm != null)
        {
           Vm.UpdateSelection.Execute();
        }

      DataFrame.Navigate(Data.ItemList[Items.SelectedIndex].Page);
   }

In fact that SelectedChanged, I would recommend moving that to your model too, the logic i mean. Your ViewModel Should, if possible handle your navigation not the view.

As for your notification changes Have a look at fody weavers. When doing a WPF ui personally I prefer the cleanliness of adding a single

[ImplementNotifyPropertyChanged]
public class ViewModel
{
}

and have compile time addition of the notifyProperties. Although if you do want more manual control that is fine too.

Also, Xaml grows fast. Try to remove any extraneous properties. Do you NEED names for each row/column?

P.s Also a pet peeve of mine, HUGE METHODS, especially switch case. I try to avoid them as much as possible. If i have to use them as a golden rule I only ever have one line after the actual Case so:

    public void SelectionChanged(string newSelection)
{
    const string BLANK = " ";
    Back.Insert(0, CreateMenuItem(newSelection));

    if (newSelection.StartsWith(BLANK)) return;

    ItemList.RemoveAll(_ => _.Title.StartsWith(BLANK));
    ItemTitles.RemoveAll(_ => _.StartsWith(BLANK));

    switch (newSelection)
    {
        case Menu1String: UpdateMenu1(); break;
        case Menu2String: UpdateMenu2(); break;
        case Menu3String: UpdateMenu3(); break;
        case Menu4String: UpdateMenu4(); break;
    }
}

I find that much cleaner IMO.

Wow.YAY! I forgot how much I liked this site since I changed jobs. :) — apieceoffruit, Commented Jan 20, 2015 at 4:03

Community · Accepted Answer · 2020-06-10 13:24:26Z

You've got a lot of properties with explicit backing values when that isn't needed. I assume you do this because you want to instantiate the collections?

When you don't have any additional logic you can just use the automatically implemented properties and instantiate them in the constructor:

public ObservableCollection<MenuItem> Back { get; set; }

public ViewModel()
{
    Back = new ObservableCollection<MenuItem>();
}

Add a using statement for Windows.ApplicationModel.Resources so you don't have to explicitly name the namespace.

 Back.Insert(0, new MenuItem(newSelection, ItemList[ItemTitles.IndexOf(newSelection)].Page, ItemList[ItemTitles.IndexOf(newSelection)].Menu));

In this line of code you're essentially recreating a new MenuItem object that already exists inside ItemList. I would just retrieve the existing MenuItem from ItemList instead.

if (newSelection.StartsWith(" ")) return;

Add brackets so it is clear where the statement begins and ends.

ItemList.RemoveAll(_ => _.Title.StartsWith(" "));
ItemTitles.RemoveAll(_ => _.StartsWith(" "));

I don't know why this is being done so this would be a good place to put a comment detailing why this is done.

Usually an undercore is used to indicate that the variable will not be used. Since you do use it, use something more descriptive like item annd itemTitle.

case "Menu 1":

You're again linking the presentation with the codebehind logic! Either use the presentation string from the resourcebundle or, better yet, use the enum that corresponds with Menu 1. This does require you to change something because right now you're passing a string to SelectionChanged instead of your MenuItem.

Conclusion

It looks better already in terms of decoupling presentation from logic but you're still causing redundancies: ItemTitles should never be a separate collection. Instead you should bind your listbox that displays the titles to your ItemList's Title property.

That way you only have a single collection to deal with.

I would like to add that you should have your collection properties be read only, msdn.microsoft.com/en-us/library/ms182327.aspx — Nick Udell, Commented Jan 7, 2015 at 14:08

Stack Exchange Network

MVVM, Navigation, and More - Part 3

2 Answers 2

Conclusion

Your Answer

Linked

Hot Network Questions

MVVM, Navigation, and More - Part 3

2 Answers 2

Conclusion

Your Answer

Sign up or log in

Post as a guest

Linked

Related

Hot Network Questions