1
\$\begingroup\$

Currently, I have been digging up my old codes. One of the applications I worked on was an Image editing phone app that displays a collection of random pictures from a user's picture, which I limited to less than 80 pictures because of performance. These random pictures are chosen from different folders in the pictures Library and read as a bitmapImage to give the user a general flip view of their pictures.

Although, I have initialised the pictures to display in the constructor of my ViewModel which is bad because it takes a bit of time( almost 1 min) to load up the data. I implemented INotifyPropertyChanged because I allow the user to create an album of pictures from the display or subset and save it as a gif image but I will not be reviewing that at the moment

public class PictureVM : INotifyPropertyChanged
{

   public PictureVM()
    {
        Task.Run(() => loadData());   
    }
ObservableCollection<BitmapImage> pictures = new ObservableCollection<BitmapImage>();

private ObservableCollection<BitmapImage> _pictureGallery;
public ObservableCollection<BitmapImage> PictureGallery
{
    get { return _pictureGallery; }
    set
    {
        if (_pictureGallery != value)
        {
            _pictureGallery = value;
            onPropertyChanged("PictureGallery");
        }
    }
}



public async void loadData()
    {
       StorageFolder recentPictures = KnownFolders.PicturesLibrary;
       IReadOnlyList<StorageFile> recentImagesInLibrary = await recentPictures.GetFilesAsync();
       IReadOnlyList<StorageFolder> subfolders = await recentPictures.GetFoldersAsync();
       IReadOnlyList<StorageFile> subfolderImages;
       await CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, async () =>
                     {
                         // tranversing the pictures library current folder
                         foreach (var item in recentImagesInLibrary)
                         {
                             BitmapImage BitImage = new BitmapImage();
                             var stream = await item.OpenAsync(Windows.Storage.FileAccessMode.Read);
                             BitImage.SetSource(stream);
                             pictures.Add(BitImage);
                         }

                         foreach (var subfolderItem in subfolders)
                         {
                             subfolderImages = await subfolderItem.GetFilesAsync();
                             foreach (var subfolderItemImages in subfolderImages)
                             {
                                 BitmapImage BitImage2 = new BitmapImage();
                                 var stream2 = await subfolderItemImages.OpenAsync(Windows.Storage.FileAccessMode.Read);
                                 BitImage2.SetSource(stream2);

                                 if (pictures.Count < 80)
                                 {
                                     pictures.Add(BitImage2);
                                 }
                                 else
                                 {
                                     break;
                                 }

                             }

                         }
                         PictureGallery = pictures;
                     } ); 
  }}

The flipview in xaml looks like this at the moment

<FlipView x:Name="flipView" Height="500" ItemsSource="{Binding PictureGallery}" Visibility="Visible"  Margin="0,0">
                            <FlipView.ItemTemplate>
                                <DataTemplate>
                                    <Image Height="680"  Width="680"  Source="{Binding}"   />
                                </DataTemplate>
                            </FlipView.ItemTemplate>
                        </FlipView>

How can I improve the way the data is loaded (performance wise)? Is there a way to load more images without performance trade off? How do I improve my randomised image selection from the user's library? Any suggestions will be gladly appreciated.

\$\endgroup\$
3
\$\begingroup\$

which I limited to less than 80 pictures because of performance

This is not correct. If in the recentImagesInLibrary are e.g 200 pictures you are loading all of them. The limiting is only happening in the second loop (the inner one).


  • A thing which bothers me as well is that you are requesting the subfolders although you might not need them.

  • By extracting the picture loading to a separate method you can get rid of some duplicated code.

  • you aren't consistent with using var. E.g I would expect using var here: BitmapImage BitImage2 = new BitmapImage();.

  • local variables should be named using camelCase casing, so e.g BitImage2 => bitImage2

  • methods should be named PascalCase casing. Please take a look at the C# naming guideline

  • async methods should be named xxxAsync see: https://stackoverflow.com/a/14652922/2655508

  • by having the pictures.Count < 80 condition only in the inner loop you are doing more work than is needed. For each subFolderItem you are loading at least one image although the condition might be reached already.

  • the stream you are opening for each image should be properly disposed/closed after it isn't needed anymore. This can be done by enclosing it inside of a using statement.

  • 80 is a magic number. By using a meaningful named variable/const you would always know what it is about. You should consider to pass it as a constructor ment so you could control it from the outside of the class.

  • if you are using C# 6 (VS 2015) you could use the nameof operator.
    Instead of onPropertyChanged("PictureGallery"); you would then write onPropertyChanged(nameof(PictureGallery)); which is a little bit longer, but if you later on decide to rename the property you wouldn't need to worry about changeing the string "PictureGallery" as well.


Disclaimer: I usually don't use async, so if anything is wrong...

Implementing the mentioned points leads to

private const int MaximumPicturesToLoad = 80;
private int maximumPicturesToLoad;

public PictureVM() : this(MaximumPicturesToLoad)
{}

public PictureVM(int maximumPicturesToLoad)
{
    this.maximumPicturesToLoad = maximumPicturesToLoad;
    Task.Run(() => loadData());   
}

private async void LoadImagesAsync(StorageFolder folder)
{
    if (AreMaximumPicturesLoaded())
    {
        return;
    }

    IReadOnlyList<StorageFile> imageFiles = await folder.GetFilesAsync();
    foreach (var file in imageFiles)
    {
        pictures.Add(await LoadImageAsync(file));
        if (AreMaximumPicturesLoaded())
        {
            return;
        }
    }
    IReadOnlyList<StorageFolder> subFolders = await folder.GetFoldersAsync();
    foreach (var subFolder in subFolders)
    {
        LoadImagesAsync(subFolder);
    }
}
private async Task<BitmapImage> LoadImageAsync(StorageFile file)
{
    var image = new BitmapImage();

    using (var stream = await file.OpenAsync(Windows.Storage.FileAccessMode.Read))
    {
        image.SetSource(stream);
    }

    return image;
}
private bool AreMaximumPicturesLoaded()
{
    pictures.Count == maximumPicturesToLoad
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ maximumPicturesToLoad should be a constant and renamed to use PascalCase. \$\endgroup\$ – Xiaoy312 Dec 19 '16 at 17:49
  • 1
    \$\begingroup\$ @Xiaoy312 fixed answer \$\endgroup\$ – Heslacher Dec 20 '16 at 8:28

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.