3
\$\begingroup\$

I am a self-taught programmer, so please be gently. Please give as much feedback as possible for my code.

I want to make sure I am doing things the right way and efficient way.

What is the proper directory to save this info? What file type and what format?

How do I load these settings when the program loads and make sure I update the UI properly? I currently do this, but have to call Cache.init() in my main views initializer, then once the View that has the listView I use get's initialized I then ReadVideoFolderSettings beause I passed the View's object to the cache.

My program will let the user select folders that contain movies, and I will check to see if the directory/path is already in the settings, if not I will add it to the settings.

The CreateFolders method of the Cache seems excessive, and I think there is a better way to save this info instead of a .dat file. Thoughts? It is really hard to find Windows 10 examples.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.IO;
using Windows.Storage;

namespace Movie_Management_Windows_10.Business
{
    using Views;
    public static class Cache
    {
        private static StorageFile SettingsFile;
        private static StorageFolder VideosFolder;
        private static List<String> LocatedFolders = new List<String>();
        private static AddVideoFolder VideoView;

        public static void Init()
        {
            CreateFolders();            
        }

        public static void LoadVideoView(AddVideoFolder videoView)
        {
            VideoView = videoView;
            ReadVideoFolderSettings();
        }

        private static async void CreateFolders()
        {
            StorageFolder localFolder = ApplicationData.Current.LocalFolder;
            VideosFolder = await localFolder.CreateFolderAsync("\\Videos\\", CreationCollisionOption.OpenIfExists);            
            StorageFolder settingsFolder = await localFolder.CreateFolderAsync("Settings\\", CreationCollisionOption.OpenIfExists);
            SettingsFile = await settingsFolder.CreateFileAsync("settings.dat", CreationCollisionOption.OpenIfExists);           
        }

        private static async void ReadVideoFolderSettings()
        {
            try
            {
                if (SettingsFile != null)
                {
                    string fileContent = await FileIO.ReadTextAsync(SettingsFile);
                    string[] rows = fileContent.Split('\n');
                    foreach(string row in rows)
                    {
                        string trimmedRow = row.Trim();
                        LocatedFolders.Add(trimmedRow);
                        VideoView.AddToListView(trimmedRow);
                    }
                }
            }
            catch (FileNotFoundException)
            {
                //how?
                throw new FileNotFoundException();
            }
        }

        private static bool VideoExists(string path)
        {
            foreach(string row in LocatedFolders)
            {
                if (row == path)
                    return true;
            }
            return false;
        }

        public static async void AddVideoFolderToSettings(string path)
        {
            if (!VideoExists(path))
            {
                string settingsData = "";
                foreach(string row in LocatedFolders)
                {
                    settingsData += row;
                }
                settingsData += path + "\r\n";
                LocatedFolders.Add(path + "\r\n");
                await FileIO.WriteTextAsync(SettingsFile, settingsData);
            }
        }

    }
}

The Init method is called from my Main App after the this.InitializeComponent();

public AppShell()
        {
            this.InitializeComponent();
            Cache.Init();
            this.Loaded += (sender, args) =>
            {
                Current = this;

                this.TogglePaneButton.Focus(FocusState.Programmatic);
            };

            this.RootSplitView.RegisterPropertyChangedCallback(
                SplitView.DisplayModeProperty,
                (s, a) =>
                {
                    // Ensure that we update the reported size of the TogglePaneButton when the SplitView's
                    // DisplayMode changes.
                    this.CheckTogglePaneButtonSizeChanged();
                });

            SystemNavigationManager.GetForCurrentView().BackRequested += SystemNavigationManager_BackRequested;

            NavMenuList.ItemsSource = navlist;
        }

There is a listView that I want to add the directories to. I do this from the AddVideoFolder view of my application.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices.WindowsRuntime;
using Windows.Foundation;
using Windows.Foundation.Collections;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Controls.Primitives;
using Windows.UI.Xaml.Data;
using Windows.UI.Xaml.Input;
using Windows.UI.Xaml.Media;
using Windows.UI.Xaml.Navigation;
using Windows.Storage.Pickers;
using Windows.Storage;
using Windows.Storage.AccessCache;

// The Blank Page item template is documented at http://go.microsoft.com/fwlink/?LinkId=234238

namespace Movie_Management_Windows_10.Views
{
    using Business;
    /// <summary>
    /// An empty page that can be used on its own or navigated to within a Frame.
    /// </summary>
    public sealed partial class AddVideoFolder : Page
    {
        public AddVideoFolder()
        {
            this.InitializeComponent();
            Cache.LoadVideoView(this);
        }

        private void buttonAddVideoFolder_Click(object sender, RoutedEventArgs e)
        {
            AddBrowsedFolder();
        }

        private async void AddBrowsedFolder()
        {
            FolderPicker folderPicker = new FolderPicker();
            folderPicker.SuggestedStartLocation = PickerLocationId.VideosLibrary;
            folderPicker.FileTypeFilter.Add(".mp4");
            folderPicker.FileTypeFilter.Add(".avi");
            folderPicker.FileTypeFilter.Add(".flv");


            StorageFolder folder = await folderPicker.PickSingleFolderAsync();
            if(folder != null)
            {
                StorageApplicationPermissions.FutureAccessList.AddOrReplace("PickedFolderToken", folder);
                Cache.AddVideoFolderToSettings(folder.Path);
            }
            else
            {

            }
        }

        public void AddToListView(string Path)
        {            
            this.listViewAddedVideoFolders.Items.Add(Path);
        }
    }
}

The way I add to the user interface is I pass the AddVideoFolder object instance to the Cache's LoadVideoView method. How else am I supposed to update the UI?

\$\endgroup\$
  • 1
    \$\begingroup\$ Instead of the Init() method you should have a static constructor like stackoverflow.com/questions/6721832/… \$\endgroup\$ – Heslacher Feb 8 '16 at 6:57
  • \$\begingroup\$ Thanks Heslacher, I have modified some of my other code and tried to make sure each function / method only has one job. \$\endgroup\$ – Illuminati Feb 8 '16 at 17:47
2
\$\begingroup\$

Namespaces

Maybe it's just nitpicking, but it should be pointed out nonetheless:

namespace Movie_Management_Windows_10.Business

Namespace names should respect the same naming convention as types, and be PascalCase. Capital_Snake_Case just doesn't look right. If your app can be compiled/targeted for platforms other than Win10, and stuff in that namespace is Win10-specific, then consider this:

namespace MovieManagement.Win10.Business

Otherwise, "universal app" doesn't strike me as something that should be coded with a specific OS in mind, so this would be more like it:

namespace MovieManagement.Business

I don't like the term Business though: it doesn't convey anything at all and tells me that if I expand that folder, I'll find just about anything in there.

I'd much rather see this:

namespace MovieManagement.Storage

Now that tells me something. I expand that folder and expect to find everything there is to know about how the application deals with the concern of storage.


#using Namespace

Okay another nitpick - your using statements should be grouped together; this is rather unusual:

using Views;
public static class Cache

Rethrowing Exceptions

This is very unfortunate:

catch (FileNotFoundException)
{
    //how?
    throw new FileNotFoundException();
}

You're catching an exception that contains valuable information about what happened, but then you discard/swallow it and throw a new exception instead - the outcome is the same, the client code sees a FileNotFoundException. But the stack trace is lost, along with all the information that the original exception had. If you don't want to handle an exception, let it go - let it bubble up to the caller.

If you otherwise want to handle it, but still rethrow the exception to the caller, you should do it like this:

catch (FileNotFoundException exception)
{
    // e.g. Debug.WriteLine(exception);
    throw;
}

Async Event Handling.

The AddBrowsedFolder method is going to run synchronously:

private void buttonAddVideoFolder_Click(object sender, RoutedEventArgs e)
{
    AddBrowsedFolder();
}

private async void AddBrowsedFolder()
{
    ...
}

Also, AddBrowsedFolder doesn't respect the naming convention for async methods. This would be right:

private async void buttonAddVideoFolder_Click(object sender, RoutedEventArgs e)
{
    await AddBrowsedFolderAsync();
}

private async Task AddBrowsedFolderAsync()
{
    ...
}

The only time async void is acceptable, is for calling async code from an event handler. Otherwise, a "void" async method should return a Task; the reason you can't make an event handler return a Task is because the EventHandler<T> delegate specifies a void return type.


Redundant Code

Get rid of code that doesn't need to exist:

        else
        {

        }

An empty else scope is just clutter. Kill with fire.


FileTypes

You've hard-coded the supported file extensions:

        folderPicker.FileTypeFilter.Add(".mp4");
        folderPicker.FileTypeFilter.Add(".avi");
        folderPicker.FileTypeFilter.Add(".flv");

This would be more flexible:

private readonly IEnumerable<string> _fileExtensions;

public AddVideoFolder(IEnumerable<string> fileExtensions)
{
    _fileExtensions = fileExtensions;

    this.InitializeComponent();
    Cache.LoadVideoView(this);
}

and then you can do this:

 foreach(var extension in _fileExtensions)
 {
    folderPicker.FileTypeFilter.Add(extension);
 }

and not have to change anything in this class when you need to support format .xyz.

The name AddVideoFolder is a bad name for a class. Keep verbs for methods, use nouns for types. Yes, naming is hard. But it's worth it.

\$\endgroup\$

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.