Problem
After asking Randomly choose a game executable from a folder, I applied the recommended changes. I also redesigned the program to make use of the MVVM (Model-View-Viewmodel) paradigm, using Prism 6.0. This was such a large change that I bumped the version number to 2.0.
The project can be found at https://github.com/nzall/GameRoulette. I will copy the relevant files below:
MainPage.xaml (.cs is basically empty and is not worth sharing):
<Page
x:Class="GameRoulette.MainPage"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:local="using:GameRoulette"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:grv="using:GameRoulette.Views"
mc:Ignorable="d" Width="400" Height="400">
<Grid>
<grv:GameRouletteView></grv:GameRouletteView>
</Grid>
</Page>
GameRouletteView.xaml (again, associated .cs file is basically empty, so no point sharing):
<UserControl
x:Class="GameRoulette.Views.GameRouletteView"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:local="using:GameRoulette.Views"
xmlns:prism="using:Prism.Windows.Mvvm"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
mc:Ignorable="d"
d:DesignHeight="300"
d:DesignWidth="400"
prism:ViewModelLocator.AutoWireViewModel="True">
<Grid Background="White">
<Button x:Name="btnSelectGames" Content="Click here to select your games"
HorizontalAlignment="Left" Margin="110,50,0,0"
VerticalAlignment="Top" Height="40" Width="240"
Command="{Binding SelectCommand}"/>
<Button x:Name="btnChooseGame" Content="{Binding ChooseGameText}"
HorizontalAlignment="Left" Margin="110,150,0,0"
VerticalAlignment="Top" Width="240" Height="40"
Command="{Binding ChooseCommand}"
IsEnabled="{Binding ChooseGameEnabled}"/>
<ProgressRing HorizontalAlignment="Left" Margin="200,100,0,0"
VerticalAlignment="Top" RenderTransformOrigin="1.05,1.983"
Height="45" Width="45" IsActive="True" Visibility="{Binding ProgressRingVisibility}"/>
<Image x:Name="imgFileIcon" HorizontalAlignment="Left"
Source="{Binding FileThumbnail}"
Height="64" Margin="110,224,0,0"
VerticalAlignment="Top" Width="64" />
<TextBlock x:Name="lblFileName" HorizontalAlignment="Left"
Margin="179,224,0,0" TextWrapping="Wrap"
Text="{Binding Filename}"
VerticalAlignment="Top" Width="171" Height="64"/>
</Grid>
</UserControl>
GameRouletteViewModel.cs
using Prism.Mvvm;
using Windows.Storage;
using Windows.Storage.Pickers;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Media;
using System;
using System.Collections.Generic;
using System.Linq;
using Windows.UI.Xaml.Media.Imaging;
using GameRoulette.Models;
using Prism.Commands;
using System.Threading.Tasks;
namespace GameRoulette.ViewModels
{
public class GameRouletteViewModel : BindableBase
{
private ImageSource fileThumbnail;
private Visibility progressRingVisibility;
private bool chooseGameEnabled;
private string filename;
private string chooseGameText;
private GameRouletteModel Model;
public DelegateCommand SelectCommand { get; private set; }
public DelegateCommand ChooseCommand { get; private set; }
public GameRouletteViewModel()
{
progressRingVisibility = Visibility.Collapsed;
chooseGameEnabled = false;
ChooseGameText = "First select a folder";
filename = "";
FileThumbnail = new BitmapImage();
Model = new GameRouletteModel();
SelectCommand = DelegateCommand.FromAsyncHandler(SelectFolderAsync);
ChooseCommand = DelegateCommand.FromAsyncHandler(OnChooseGameClick);
}
public async Task SelectFolderAsync()
{
FolderPicker folderPicker = new FolderPicker();
folderPicker.FileTypeFilter.Add(".exe");
ProgressRingVisibility = Visibility.Visible;
StorageFolder selectedFolder = await folderPicker.PickSingleFolderAsync();
ChooseGameText = "retrieving files";
int fileCount = await Model.SelectGameExecutables(selectedFolder);
ProgressRingVisibility = Visibility.Collapsed;
ChooseGameText = string.Format("{0} games found. Click here to choose your game.", fileCount);
ChooseGameEnabled = true;
}
public async Task OnChooseGameClick()
{
Tuple<BitmapImage, string> fileProperties = await Model.ChooseGame();
FileThumbnail = fileProperties.Item1;
Filename = fileProperties.Item2;
}
public string ChooseGameText
{
get { return chooseGameText; }
set
{
SetProperty(ref chooseGameText, value);
}
}
public ImageSource FileThumbnail
{
get { return fileThumbnail; }
set
{
SetProperty(ref fileThumbnail, value);
}
}
public Visibility ProgressRingVisibility
{
get
{
return progressRingVisibility;
}
set
{
SetProperty(ref progressRingVisibility, value);
}
}
public bool ChooseGameEnabled
{
get { return chooseGameEnabled; }
set
{
SetProperty(ref chooseGameEnabled, value);
}
}
public string Filename
{
get { return filename; }
set
{
SetProperty(ref filename, value);
}
}
}
}
GameRouletteModel.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Windows.Storage;
using Windows.UI.Xaml.Media.Imaging;
namespace GameRoulette.Models
{
class GameRouletteModel
{
string[] blacklist = { "uninst", "system", "redist", "framework", "physx", "builder", "processor", "helper" };
IList<StorageFile> Files;
public async Task<int> SelectGameExecutables(StorageFolder folder)
{
IEnumerable<StorageFile> retrievedFiles = await folder.GetFilesAsync(Windows.Storage.Search.CommonFileQuery.OrderByName);
var fileCount = retrievedFiles.Count();
Files = retrievedFiles.Where((file, index) => ValidateFile(file, index, fileCount)).ToList();
return Files.Count;
}
private bool ValidateFile(StorageFile file, int index, int fileCount)
{
if (file.FileType != ".exe")
{
return false;
}
var lowerCaseFileName = file.Name.ToLowerInvariant();
return !blacklist.Any(e => lowerCaseFileName.Contains(e));
}
internal async Task<Tuple<BitmapImage, string>> ChooseGame()
{
if (!Files.Any())
{
return null;
}
Random generator = new Random();
int index = generator.Next(0, Files.Count - 1);
StorageFile chosenGame = Files[index];
Windows.Storage.FileProperties.StorageItemThumbnail thumbnail = await chosenGame.GetScaledImageAsThumbnailAsync(Windows.Storage.FileProperties.ThumbnailMode.SingleItem, 64);
BitmapImage image = new BitmapImage();
image.SetSource(thumbnail);
return new Tuple<BitmapImage, string> (image, chosenGame.DisplayName);
}
}
}
2 of the 3 bugs from before are still present. I removed one of the bugs since it doesn’t really
MAJOR: from what I can tell, it does not handle symbolic links well. I have several games symbolically linked from another drive, but they’re not found.
MINOR: my blacklist is incomplete, but that’s just a matter of adding the right items.
I have also noticed that it’s quite slow compared to other similar software. For example, WinDirStat takes under 2 minutes to check my entire C: drive. This program isn’t finished with that after 20 minutes AND makes the Runtime Broker use 4 GB of memory, which causes system warning for out of memory.
The first Major bug might be a limitation of GetFilesAsync. The Minor bug is just a matter of testing until a proper blacklist has been found.
the performance issues, however, are quite concerning. I am not sure if they are fixable though.
Special feedback I’m looking for on top of normal refactoring:
- Have I implemented MVVM using Prism properly?
- Are the performance issues solvable?
- Is the major bug solvable?
Solution
<grv:GameRouletteView></grv:GameRouletteView>
The tag should be self-closing if there’s nothing in it:
<grv:GameRouletteView />
You’re using x:Name attributes, but the identifiers aren’t used anywhere and can be discarded: that’s a very often very good sign you’re doing something right, when your XAML doesn’t need named controls.
What I don’t like though, is all these explicit margins: I don’t know what your UI looks like, but it seems you could use better layouts; explicit margins everywhere are a sign that you’re dragging-and-dropping your controls into the designer – that works, but you’ll have a much better UI if you let WPF handle the layout; you’re literally dumping your controls into that Grid with explicit positioning, as if drawing on a canvas.
You could start by defining rows and columns for your grid:
<Grid.RowDefinitions>
<RowDefinition Height="*" /> <!-- todo -->
</Grid.RowDefinitions>
And then use layout panels, for example StackPanel, wisely and as needed:
<StackPanel Grid.Row="1">
<!-- controls -->
</StackPanel>
Seeing a textbox’s Width=171 in the markup, begs the question: why isn’t it 170? With a structured layout, you get rid of arbitrary margins like 179,224,0,0, and let the controls take the space they’re allowed to take – you can constrain grid rows/columns’ MinHeight and/or MaxWidth to keep things under control, and the result is a view that adapts much better to resizing.
In other words, using explicit margins everywhere is the winforms way of doing layout.
Code-wise, I don’t like seeing a Tuple<T1,T2> in any API, even if it’s internal. What you’re returning from that ChooseGame method could have its own name:
internal class GameInfo
{
public string FileName { get; set; }
public BitmapImage Thumbnail { get; set; }
}
And then the signature and call sites become much more readable:
internal async Task<GameInfo> ChooseGame()
And then instead of this:
Tuple<BitmapImage, string> fileProperties = await Model.ChooseGame();
FileThumbnail = fileProperties.Item1;
Filename = fileProperties.Item2;
You could have that:
var info = await Model.ChooseGame();
FileThumbnail = info.Thumbnail;
Filename = info.FileName;
This is a problem:
Random generator = new Random();
It should be an instance-level field, not a new instance every time it’s called.
Also, blacklist could be a private readonly IReadOnlyList<string> instead of a simple array.
Visibility should be bool IsVisible:
public Visibility ProgressRingVisibility
{
get
{
return progressRingVisibility;
}
set
{
SetProperty(ref progressRingVisibility, value);
}
}
I’m not sure why you’re calling SetProperty here, why can’t you just assign the value and OnPropertyChanged? Or is that some magic going on in BindableBase? I’m not sure I like that it needs that ref parameter, and SetProperty makes it look like you’re assigning a DependencyProperty, which shouldn’t be the case here.
The ViewModel exposing a bool Visibility will obviously break your binding:
Visibility="{Binding ProgressRingVisibility}"
The trick is to add a <UserControl.Resources> section where you give an x:Key to a BooleanToVisibilityConverter, like this:
<BooleanToVisibilityConverter x:Key="BoolToVisibility"/>`
And then you update the binding with the converter:
Visibility="{Binding ProgressRingVisibility, Converter={StaticResource BoolToVisibility}}"
The concept of Visibility shouldn’t be leaking into your ViewModel; it’s really a presentation thing. Hence I’d probably expose it as some bool IsBusy or whatever fits the bill here.