Problem
This is a simple application that will eventually manage certain files for video games. For now, I’m only posting code that handles fetching, adding, and viewing games.
I’m trying to keep things incredibly simple and pure while learning WPF and MVVM, so my goal is to avoid frameworks. Please note that I’m not too concerned with the UI at this point, so I’m only interested in XAML/UI suggestions if they serve to improve my implementation of MVVM or if I’m missing easy/common shortcuts. Also, I’ll omit the service, repository, and ViewModel classes unless they are requested.
Model
[SettingsSerializeAs(SettingsSerializeAs.Xml)]
public class Game : IEntity
{
public string Name { get; set; }
public string ExecutablePath { get; set; } // e.g. Minecraft.exe
public Game() { } // Required to be serialized as a setting.
public Game(string name, string executablePath)
{
Name = name;
ExecutablePath = executablePath;
}
}
MainView and MainViewModel – I won’t share the code for these. MainView simply has a GamesView and a button to open the ManageGamesView.
GamesView – A simple ComboBox (appears on the MainView above). Used to select the current game.
<UserControl x:Class="ENBOrganizer.UI.Views.GamesView"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008" mc:Ignorable="d"
xmlns:ViewModels="clr-namespace:ENBOrganizer.UI.ViewModels" xmlns:UI="clr-namespace:ENBOrganizer.UI">
<UserControl.Resources>
<UI:PathToIconConverter x:Key="PathToIconConverter" />
</UserControl.Resources>
<UserControl.DataContext>
<ViewModels:GamesViewModel />
</UserControl.DataContext>
<Grid>
<ComboBox ItemsSource="{Binding Games}" SelectedItem="{Binding SelectedGame}"
VerticalAlignment="Top" HorizontalAlignment="Left" MaxHeight="25">
<ComboBox.ItemTemplate>
<DataTemplate>
<StackPanel Orientation="Horizontal">
<Image Source="{Binding ExecutablePath, Converter={StaticResource PathToIconConverter}, Mode=OneWay}" />
<TextBlock Text="{Binding Name}" VerticalAlignment="Center" Padding="5,0,0,0" />
</StackPanel>
</DataTemplate>
</ComboBox.ItemTemplate>
</ComboBox>
</Grid>
</UserControl>
GamesViewModel
public class GamesViewModel : ViewModelBase
{
private readonly GameService _gameService;
private ObservableCollection<Game> _games;
public ObservableCollection<Game> Games
{
get { return _games; }
set { _games = value; RaisePropertyChanged("Games"); }
}
public Game SelectedGame
{
get { return Settings.Default.SelectedGame; }
set
{
if ((value != null) && (Settings.Default.SelectedGame != value))
{
Settings.Default.SelectedGame = value;
Settings.Default.Save();
RaisePropertyChanged("SelectedGame");
}
}
}
public GamesViewModel()
{
_gameService = ServiceSingletons.GameService;
_gameService.GamesChanged += OnGamesChanged;
Games = new ObservableCollection<Game>(_gameService.GetAll());
}
private void OnGamesChanged(object sender, EventArgs e)
{
Games = new ObservableCollection<Game>(_gameService.GetAll());
}
}
ManageGamesView – A screen used to add and remove games
<Window x:Class="ENBOrganizer.UI.Views.ManageGamesView"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:ViewModels="clr-namespace:ENBOrganizer.UI.ViewModels" xmlns:UI="clr-namespace:ENBOrganizer.UI"
Title="Manage Games" MinWidth="250" SizeToContent="WidthAndHeight" >
<Window.Resources>
<UI:PathToIconConverter x:Key="PathToIconConverter" />
</Window.Resources>
<Window.DataContext>
<ViewModels:ManageGamesViewModel />
</Window.DataContext>
<Grid>
<Grid.RowDefinitions>
<RowDefinition />
<RowDefinition />
</Grid.RowDefinitions>
<Grid Grid.Row ="0">
<Grid.RowDefinitions>
<RowDefinition />
<RowDefinition />
<RowDefinition />
</Grid.RowDefinitions>
<Grid.ColumnDefinitions>
<ColumnDefinition />
<ColumnDefinition />
<ColumnDefinition />
</Grid.ColumnDefinitions>
<Label Content="Name:" Grid.Row="0" Grid.Column="0" VerticalAlignment="Center" />
<TextBox Text="{Binding Name}" Grid.Row="0" Grid.Column="1" MinWidth="100" Margin="5" />
<Label Content="Path:" Grid.Row="1" Grid.Column="0" VerticalAlignment="Center" />
<TextBox Text="{Binding ExePath}" Grid.Row="1" Grid.Column="1" MaxWidth="300" TextWrapping="Wrap" Margin="5" />
<Button Content="Browse..." Command="{Binding BrowseCommand}" Grid.Row="1" Grid.Column="2" VerticalAlignment="Center" Margin="0,0,5,0" />
<StackPanel Grid.Row="2" Grid.ColumnSpan="2" Orientation="Horizontal" HorizontalAlignment="Right" >
<Button Content="Remove" Command="{Binding RemoveGameCommand}" HorizontalAlignment="Right" Margin="0,0,5,0" />
<Button Content="Add" Command="{Binding AddGameCommand}" HorizontalAlignment="Right" />
</StackPanel>
</Grid>
<StackPanel Grid.Row="1" Orientation="Vertical">
<TextBlock>Games</TextBlock>
<ListView Name="GamesListView" ItemsSource="{Binding Games}" SelectedItem="{Binding SelectedGame}" >
<ListView.ItemTemplate>
<DataTemplate>
<StackPanel Orientation="Horizontal">
<Image Source="{Binding ExecutablePath, Converter={StaticResource PathToIconConverter}, Mode=OneWay}" />
<TextBlock Text="{Binding Name}" VerticalAlignment="Center" Padding="5,0,0,0" />
</StackPanel>
</DataTemplate>
</ListView.ItemTemplate>
</ListView>
</StackPanel>
</Grid>
</Window>
It ends up looking like this:
ManageGamesViewModel
public class ManageGamesViewModel : ViewModelBase, IDataErrorInfo
{
private readonly GameService _gameService;
private ObservableCollection<Game> _games;
private Game _selectedGame;
private string _name, _exePath;
public ICommand RemoveGameCommand { get; set; }
public ICommand AddGameCommand { get; set; }
public ICommand BrowseCommand { get; set; }
public ObservableCollection<Game> Games
{
get { return _games; }
set { _games = value; RaisePropertyChanged("Games"); }
}
public Game SelectedGame
{
get { return _selectedGame; }
set
{
_selectedGame = value;
if (value == null)
return;
Name = value.Name;
ExePath = value.ExecutablePath;
}
}
public string Name
{
get { return _name; }
set { _name = value; RaisePropertyChanged("Name"); }
}
public string ExePath
{
get { return _exePath; }
set { exePath = value; RaisePropertyChanged("ExePath"); }
}
public ManageGamesViewModel()
{
_gameService = ServiceSingletons.GameService;
_gameService.GamesChanged += OnGamesChanged;
Games = new ObservableCollection<Game>(_gameService.GetAll());
RemoveGameCommand = new ActionCommand(RemoveGame, CanRemove);
AddGameCommand = new ActionCommand(AddGame, CanAdd);
BrowseCommand = new ActionCommand(BrowseForGameFile, () => true);
}
private void AddGame()
{
Game game = new Game(Name, ExePath);
_gameService.Add(game);
}
private bool CanAdd()
{
Game game = new Game(Name, ExePath);
return _gameService.ValidateGame(game);
}
private void BrowseForGameFile()
{
OpenFileDialog openFileDialog = new OpenFileDialog
{
Filter = "EXE Files (*.exe)|*.exe",
Title = "Select the game's .exe file"
};
if (openFileDialog.ShowDialog() == true)
{
Name = Path.GetFileNameWithoutExtension(openFileDialog.FileName);
ExePath = openFileDialog.FileName;
}
}
private void RemoveGame()
{
_gameService.Delete(SelectedGame);
}
private bool CanRemove()
{
return SelectedGame != null;
}
private void OnGamesChanged(object sender, EventArgs e)
{
Games = new ObservableCollection<Game>(_gameService.GetAll());
}
}
Please note that the SelectedGame property here is simply what is selected in the ListView. SelectedGame in GamesViewModel is an application setting.
Key Concerns
My concerns are pretty much all related to one another. I could be worrying about nothing, but I can’t fight this feeling that I’m missing a pattern or solution that would address all of these issues at once:
- I’m using an event,
GamesChanged, in the GameService class to alert my ViewModel to CRUD operations. This feels wrong and slightly WinForms-ish. I thought of implementingINotifyPropertyChangedin my service class, but I’m not quite sure that would work since I would have to invent a new property to watch. I also considered not listening to the service at all and manually updating the UI, i.e. adding/removing from theObservableCollectionafter a service call. However, issue 2 explains my dilemma there. - Because of issue 1, I needed the same instance of
GameServicein bothGamesViewModelandManageGamesViewModelso that they could each react to changes to the data simultaneously, e.g. adding a new game in the Manage Games screen refreshes the ComboBox on the main screen. Assuming I’ve taken the right approach in using an event from the service, is my use of a singleton property acceptable? In WinForms, I would have passed an instance ofGameServicein the constructor of the form, but I don’t believe that’s possible for my ViewModel because it is instantiated in XAML without arguments. Since I’m trying to stay very simple, I’d like to avoid an IoC container right now. - This also hinges on issue 1, but I’m skeptical of how I refresh the
ObservableCollection<Game>property in theGamesViewModelandManageGamesViewModel. I do like how simple it is, but it seems I don’t even needObservableCollectionhere. That could just as well be a plain ol’List<Game>.
I think that does it for now. Feel free to offer any suggestions. Although I’m trying to avoid frameworks, I’m open to using them if they simplify my design and/or easily resolve my issues.
Solution
- You should only use
OneTimebindings when binding to classes which do not implementINotifyPropertyChangedinterface, otherwise those bindings will leak memory. If you need aTwoWaybinding to yourGame, then you should wrap it intoGameViewModel. Same goes for collections andINotifyCollectionChanged. Using plain ol’List<Game>asItemsSourcewill leak memory too. - Singletons… just don’t. Simply pass the service into constructors manually or using some
DIcontainer. - You should avoid re-creating
ObservableCollection. CallClearand repopulate it instead, when you need to reload it. - I think the simplest way to address your concerns is to share single
ObservableCollectionof your games between the viewmodels that need it. This way you won’t have to bother with synchronization. There are multiple ways to achieve this. For example, you could create a child view model to represent the list of games, and inject it everywhere its needed. Or you could re-organize your UI in a way, that there is only one place where list of games is displayed. Or you could even returnObservableCollectionfrom your service class, and expose it from your viewmodels (keep in mind the point #1 aboutOneTimebindings though). - I think it is generally a bad idea to
Savesettings in property setter. Writing to disk can be expensive and it can lag. You should perform I/O operations either in background or when user closes the window.
Disclaimer: I don’t review Xaml, I focus on the C# part only.
[SettingsSerializeAs(SettingsSerializeAs.Xml)]
public class Game : IEntity
{
public string Name { get; set; }
public string ExecutablePath { get; set; } // e.g. Minecraft.exe
public Game() { } // Required to be serialized as a setting.
public Game(string name, string executablePath)
{
Name = name;
ExecutablePath = executablePath;
}
}
The first comment // e.g. Minecraft.exe is only adding noise to the code. Its perfectly clear that the ExecutablePath property holds exactly what the name implies. Adding the comment doesn’t make this more clear.
The second comment is a good comment, because it describes why the constructor is public. One could say that the constructor could be omitted because it is added by default, but IMO it makes the indent more clear.
GamesViewModel
public ObservableCollection<Game> Games
{
get { return _games; }
set { _games = value; RaisePropertyChanged("Games"); }
}
Vertical space aka new lines doesn’t cost anything, you should use them to make your code more readable. At first glance (especially with old eyes) it is hard to see that the setter consist of two instructions. Do yourself and Sam the maintainer something good and use some more lines here like
public ObservableCollection<Game> Games
{
get { return _games; }
set
{
_games = value;
RaisePropertyChanged("Games");
}
}
If you by any chance are using C# 6.0 (implemented in VS 2015) you should use the nameof operator like so
public ObservableCollection<Game> Games
{
get { return _games; }
set
{
_games = value;
RaisePropertyChanged(nameof(Games));
}
}
which has the advantage, if you choose to rename the property to something else using the default refactoring tools ( Rename F2 ) this will be changed too.
In the SelectGame property setter the doubled brackets if ((value != null) && (Settings.Default.SelectedGame != value)) are not needed. You can change this to
if (value != null && Settings.Default.SelectedGame != value)
I don’t really like the saving of the Setting.Default in this setter. I have the feeling that this should belong to somewhere else, but not in the ViewModel which should be responsible for the interaction between the View and the Model and should contain primary the logic regarding UI.
public GamesViewModel()
{
_gameService = ServiceSingletons.GameService;
_gameService.GamesChanged += OnGamesChanged;
Games = new ObservableCollection<Game>(_gameService.GetAll());
}
private void OnGamesChanged(object sender, EventArgs e)
{
Games = new ObservableCollection<Game>(_gameService.GetAll());
}
The OnSomeEvent pattern is widely used by the eventsource, the object which is raising the event. The eventhandler usually is in the form of EventSource_Event or Event. You should rename OnGamesChanged either to GamesChanged (which I prefer) or to GameService_GamesChanged.
I will come back to the events part later.
private string _name, _exePath;
Multiple declarations on the same line makes your code less readable.
public Game SelectedGame
{
get { return _selectedGame; }
set
{
_selectedGame = value;
if (value == null)
return;
Name = value.Name;
ExePath = value.ExecutablePath;
}
}
Omitting braces {}, although they are optional for single instruction if, can lead to error prone code.
AddGame() vs RemoveGame()
In AddGame() you make a call to _gameService.Add() which is pretty straightforward but in RemoveGame() you make a call to _gameService.Delete(SelectedGame) which seems odd to me. I would like to suggest that you rename the Delete() method of the GameService to Remove().
Key concerns
I’m using an event, GamesChanged, in the GameService class to alert my
ViewModel to CRUD operations. This feels wrong and slightly
WinForms-ish.
Well, the GameService shouldn’t have anything to to with any UI related stuff, so it can’t be neither “WinForms-ish” nor “Wpf-ish”.
Having and raising events is just the way how children should talk to their parents. If the ViewModel should react on any CRUD operation of the GameService then using events is the way to go.
Using the INotifyPropertyChanged event for such a case wouldn’t be good, because typically this event is used for binding clients and like you mentioned yourself
I thought of implementing INotifyPropertyChanged in my service class, but I’m not quite sure that would work since I would have to invent a new property to watch.
which leads now to
Because of issue 1, I needed the same instance of GameService in both GamesViewModel and ManageGamesViewModel so that they could each react to changes to the data simultaneously, e.g. adding a new game in the Manage Games screen refreshes the ComboBox on the main screen. Assuming I’ve taken the right approach in using an event from the service, is my use of a singleton property acceptable? In WinForms, I would have passed an instance of GameService in the constructor of the form, but I don’t believe that’s possible for my ViewModel because it is instantiated in XAML without arguments. Since I’m trying to stay very simple, I’d like to avoid an IoC container right now.
You don’t need to use constructor injection you could simply use property injection. There would be no need to have a Singleton then.
This also hinges on issue 1, but I’m skeptical of how I refresh the ObservableCollection property in the GamesViewModel and ManageGamesViewModel. I do like how simple it is, but it seems I don’t even need ObservableCollection here. That could just as well be a plain ol’ List.
For this part you just need to ask yourself: Do I need to react on changes made to this collection ? If the answer is Yes you should keep the ObservableCollection<T> otherwise skip it.
Events
I would like to suggest that you don’t use the event GamesChanged in the way you do it now. It is adding a big overhead if you need each time to call _gameService.GetAll() although maybe only one Game is added or removed.
I would like to suggest that you add your own event arguments which contains for instance the action which took place (by using an enum) and the Game object in question, which would make it easier to update your objects.
You are right. You don’t need the GamesChanged event.
GameService seems to be something like a ViewModel, but the ManageGamesViewModel holds the games.
I would move the ObservableCollection<Game> Games to the GameService class, because it seems that your GameService class has a List or something similar to this. Your code will look e.g. like this:
public GameService
{
public ObservableCollection<Game> Games {get;set;}
public Add(Game game)
{
Games.Add(game);
}
}
In the ManageGamesViewModel you wrap _gameService in a public property with the RaisePropertyChanged Method in the setter:
public class ManageGamesViewModel : ViewModelBase, IDataErrorInfo
{
public GameService GameService
{
get
{
return this._gameService;
}
set
{
this._gameService = value;
RaisePropertyChanged("GameService");
}
}
}
And now you bind to the Games Collection like this:
<ListView Name="GamesListView" ItemsSource="{Binding GameService.Games}"... />
instead of
<ListView Name="GamesListView" ItemsSource="{Binding Games}" ... />