1
\$\begingroup\$

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:

  1. Have I implemented MVVM using Prism properly?
  2. Are the performance issues solvable?
  3. Is the major bug solvable?
\$\endgroup\$
2
\$\begingroup\$
<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 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.

\$\endgroup\$
3
  • \$\begingroup\$ BindableBase is a class from Prism, a Microsoft-generated NuGet package that abstracts away all that OnPropertyChanged and handler boilerplate code for WPF binding. Also, it's a UWP project, and from what I've developed since the question, I seem to remember not coming across the BooleanToVisibilityConverter class. \$\endgroup\$ – Nzall Feb 16 '16 at 19:40
  • \$\begingroup\$ At any rate, I've had my brother go over my MVVM implementation and apparently, the way I implemented it is not even close to true MVVM. what I currently have is basically the classic 3 layers model. I'm going to rewrite the entire project a 3rd time for TRUE MVVM implementation. For example, Model would be a file, and View Model would be the class that selects the file. \$\endgroup\$ – Nzall Feb 16 '16 at 19:42
  • \$\begingroup\$ The converter is merely an implementation of IValueConverter - make your own if it's not available! Also, design patterns are a mean of communication, not a coding standard. What matters is whether your code is robust, readable, maintainable and flexible, not whether it adheres to such or such blog's definition of what MVVM should be. \$\endgroup\$ – Mathieu Guindon Feb 16 '16 at 19:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.