Correct MVVM format

I have a Windows-Runtime app (very similar to WPF), and I am using (at least I think I am!) the MVVM style. I want to make sure I am doing this the proper way, and not just a working way.

This is my MainPage.xaml:











In my MainPage.xaml.cs, I have this:

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

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

private void NewSelect(object sender, SelectionChangedEventArgs e)
{
// IMPORTANT:
// This method is being reconstructed, and will be posted later.
// Instead of using two independent lists for Pages and Items, I am making a class or struct to keep them linked.
// If you attempt to run this code, leave this method empty.

switch(Data.OneNoteVersion)
{
case 0:
Data.NewSelectionWS(Items.SelectedValue.ToString());
break;
case 1:
Data.NewSelectionP(Items.SelectedValue.ToString());
break;
case 2:
Data.NewSelection2013(Items.SelectedValue.ToString());
break;
}

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


This is my ViewModel class:

`public class ViewModel : INotifyPropertyChanged
{
public ViewModel()
{
foreach(string s in GlobalVars.Menus())
{
ItemList.Add(s);
}
}

private ObservableCollection _itemList = new ObservableCollection();
public ObservableCollection ItemList
{
get { return _itemList; }
set
{
_itemList = value;
OnPropertyChanged();
}
}

public event PropertyChangedEventHandler PropertyChanged;
protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
{
PropertyChangedEventHandler hand

You're declaring your viewmodel as public static. This indicates encapsulation abuse because a single viewmodel instance should be tied to a single page. In fact, you often want a specific viewmodel for every page (where appropriate) so there isn't that much re-use of the viewmodels in the first place.

This should become private ViewModel _data unless I'm missing a critical piece of information.

ViewModel doesn't tell me anything about what it does. This will cause confusion when more viewmodels are added, things start to become interweaved, etc. Something like ItemSelectionPageViewmodel would be more appropriate (notice how I name the viewmodel according to the page where it will be used instead of referring to an actual model class).

OnSelectionChanged seems like a more proper name for a selection changed event handler. Event handlers are typically in the form of On[event].

Personally I like to use Fody.PropertyChanged to automatically inject everything about OnPropertyChanged() in my code. It saves me time, it's easy (you just put the [ImplementPropertyChanged] annotation above your viewmodel) and your code isn't littered by the backing variables and stuff. It looks like it is appropriate in your scenario as well.

Your naming sometimes indicates items (content) but other times menus (navigation). Considering everything is navigation-related I'd suggest renaming where necessary to something like "Menu", "MenuList", "Navigation", etc.

Right now you're hardcoding the menus. This should be saved in a resource file so you can centrally access your content and easily add multiple languages to your application. More keywords to google: localization and globalization.

I'm not a fan of simple types in an ObservableCollection. Even if it would be just one field, I would suggest wrapping it in a complex type. This has the benefit that you can always add more fields for extra data without having to change the entire pipeline to suddenly fit a new type through it.

Following the previous suggestion: right now you're using some methods like this:

Data.NewSelectionWS(Items.SelectedValue.ToString());
Data.NewSelectionP(Items.SelectedValue.ToString());

I don't know what this does exactly but I do see a problem: right now you're directly linking the presentation of your application to the codebehind. It's obvious why this is a problem when you're thinking about multiple languages: suddenly the same menu but in a different language might not work as you expect it to (I don't know the actual code that uses it).

What I would suggest is the following: since you have to work with the limitations of the lack of TreeView and your content is displayed on the same page instead of spread over multiple pages, you need some centralized way to display what is shown when the selection in the menu listbox changes.

I agree with your current approach but right now you limit yourself because you can't change the presentation without having to change internal workings.

I propose the following solution:

public class MenuItem
{
    public string DisplayText { get; set; }
    public Menu Menu { get; set; }
}

public enum Menu
{
    Homepage,
    Notebooks,
    SomeSubmenu,
    Sections, 
    Pages, 
    Notes,
    Otherfunstuff
}

public class ViewModel
{
    private Dictionary _menus { get; set; }

    public ViewModel()
    {
        CreateMenus();
    }

    private void CreateMenus()
    {
        _menus = new Dictionary();
        _menus.Add(Menu.Homepage, new MenuItem { Menu = Menu.Homepage, DisplayText = string.Empty /* Use the resource bundle */ });
        // Add all other menus
    }

    IEnumerable GetMenuItems(Menu menu)
    {
        Menu[] allowedMenus = null; 
        if(menu == Menu.Homepage)
        {
            allowedMenus = new[] { Menu.Notebooks, Menu.Notes, Menu.Pages };    
        }

        if(menu == Menu.Notebooks)
        {
            allowedMenus = new[] { Menu.SomeSubmenu };
        }

        return _menus.Where(x => allowedMenus.Contains(x.Key));
    }
}

You can now happily change the display of your itemslist and easily add more menus. If a new page has to be added you can create a new enum value, initialize it to point to the correct displaytext and add it to each page that should have it.

This solution also removes the need for GlobalVars.


One specific question I have is whether Menus() should be like this:
public static string[] Menus { get { return _menus; } }
This would prevent _menus from being modified (I think)

No, it won't. You're returning a reference to the array so any changes you make to an element inside that reference, will be propagated to the underlying _menus array. You'd have to break the connection by constructing a new object with the same data as the _menus array and passing that if you'd want to avoid mutations. All you do now is make sure nobody overwrites the value of _menus with a new one (e.g. `_menus = new s

StackExchange Code Review Q#75455, answer score: 3