Problem
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:
<Grid Background="White">
<Grid.ColumnDefinitions>
<ColumnDefinition Width="300" x:Name="ItemsColumn"/>
<ColumnDefinition Width="*" x:Name="DataColumn"/>
</Grid.ColumnDefinitions>
<ListBox Background="WhiteSmoke" Name="Items" Grid.Column="0" ItemsSource="{Binding ItemList}" SelectionChanged="NewSelect"
Margin="-2,-2,0,-2" Padding="0,10" />
</Grid>
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<string> _itemList = new ObservableCollection<string>();
public ObservableCollection<string> ItemList
{
get { return _itemList; }
set
{
_itemList = value;
OnPropertyChanged();
}
}
public event PropertyChangedEventHandler PropertyChanged;
protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
{
PropertyChangedEventHandler handler = PropertyChanged;
if (handler != null)
{
handler(this, new PropertyChangedEventArgs(propertyName));
}
}
}
This is GlobalVars.cs:
class GlobalVars
{
private static string[] _menus = { "Menu 1", "Menu 2", "Menu 3", "Menu 4",
"Menu 5", "Menu 6", "Menu 7", "Menu 5",
"Menu 9", "Menu 10", "Menu 11", "Menu 12",
"Menu 13", "Menu 14" };
public static string[] Menus() { return _menus; }
}
Of course, this is only a subset of the full scope of my app, but I couldn’t possibly post the full thing here, and I am working on some very major changes suggested by other reviews and want to get the whole thing right this time. Be sure to tell me everything I should change.
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), and this is important, but again, I can just be careful to not do Menus = ...;.
Just for reference, if you try to run it this way, you need to change all GlobalVars.Menus() calls to GlobalVars.Menus.
For more information on how the menus work, see this question: Display submenus
Solution
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<Menu, MenuItem> _menus { get; set; }
public ViewModel()
{
CreateMenus();
}
private void CreateMenus()
{
_menus = new Dictionary<Menu, MenuItem>();
_menus.Add(Menu.Homepage, new MenuItem { Menu = Menu.Homepage, DisplayText = string.Empty /* Use the resource bundle */ });
// Add all other menus
}
IEnumerable<MenuItem> 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 string[];) but you’re not protecting against mutating the individual elements.