4
\$\begingroup\$

My application needs to let the user select a folder from somewhere on their local machine. Below are the interface and model pair that I have written to let the user do so.

public interface IFolderPicker
{
    //  Methods
    //  =======

    string Show();
}
using Microsoft.WindowsAPICodePack.Dialogs;
using WinForms = System.Windows.Forms;

public class FolderPicker : IFolderPicker
{
    //  Constants
    //  =========

    private const string SelectATargetFolder = "Select a target folder";
    private const string CDrive = @"C:\";

    //  Variables
    //  =========

    private readonly bool isPlatformSupported;
    private readonly WinForms.FolderBrowserDialog xpFolderBrowserDialog;
    private readonly CommonOpenFileDialog vistaFolderBrowserDialog;

    //  Constructor
    //  ===========

    public FolderPicker()
    {
        isPlatformSupported = CommonFileDialog.IsPlatformSupported;

        xpFolderBrowserDialog = new WinForms.FolderBrowserDialog
        {
            Description = SelectATargetFolder
        };

        vistaFolderBrowserDialog = new CommonOpenFileDialog
        {
            Title = SelectATargetFolder,
            IsFolderPicker = true,
            DefaultDirectory = CDrive,
            AllowNonFileSystemItems = false,
            EnsurePathExists = true,
            Multiselect = false,
            NavigateToShortcut = true
        };
    }

    //  Deconstructor
    //  =============

    ~FolderPicker()
    {
        xpFolderBrowserDialog.Dispose();
        vistaFolderBrowserDialog.Dispose();
    }

    //  Methods
    //  =======

    public string Show()
    {
        if (isPlatformSupported)
        {
            return SelectWinVistaOrLaterFolder();
        }
        else
        {
            return SelectWinXPFolder();
        }
    }

    private string SelectWinXPFolder()
    {
        WinForms.DialogResult result = xpFolderBrowserDialog.ShowDialog();

        switch (result)
        {
            case WinForms.DialogResult.OK:
            case WinForms.DialogResult.Yes:
                return xpFolderBrowserDialog.SelectedPath;

            default:
                return null;
        }
    }

    private string SelectWinVistaOrLaterFolder()
    {
        CommonFileDialogResult result = vistaFolderBrowserDialog.ShowDialog();

        switch (result)
        {
            case CommonFileDialogResult.Ok:
                return vistaFolderBrowserDialog.FileName;

            default:
                return null;
        }
    }
}

My reasoning for this design is that now the 3 readonly variables can be mocked in unit tests via reflection, while this isn't an ideal way of doing it, it's better than nothing.

While I am looking for all feedback, my focus here is on the WinForms.FolderBrowserDialog and the CommonOpenFileDialog. They both implement the IDisposable interface and my concern is that I might not be implementing that properly?
I believe that I could make the IFolderPicker interface extend the IDisposable interface and then call the two .Dispose() methods from within the inherited Dispose method but I was hoping I could insulate the rest of my program from these two classes as much as possible - is disposing of them in the deconstructor like this also valid?

Originally I had both these classed being instantiated in their respective Select...() methods in a using block. But while also not being mock-able, it meant they were being newed up each time the method was called - is this current implementation better than that?

\$\endgroup\$
4
\$\begingroup\$

Additionally to the great answer of dfhwze, I'll provide an alternative implementation that includes some of the suggestions.

Primary, I would implement each case separatly and create / dispose the actual dialog each time the method Show is called:

public interface IFolderPicker
{
    string Show();
}

public class VistaFolderPicker : IFolderPicker
{
    private const string SelectATargetFolder = "Select a target folder";
    private const string CDrive = @"C:\";

    public string Show()
    {
        var vistaFolderBrowserDialog = new CommonOpenFileDialog
        {
            Title = SelectATargetFolder,
            IsFolderPicker = true,
            DefaultDirectory = CDrive,
            AllowNonFileSystemItems = false,
            EnsurePathExists = true,
            Multiselect = false,
            NavigateToShortcut = true
        };

        using (vistaFolderBrowserDialog)
        {
            return vistaFolderBrowserDialog.ShowDialog() == CommonFileDialogResult.Ok
                ? vistaFolderBrowserDialog.FileName
                : null;
        }
    }
}

public class XPFolderPicker : IFolderPicker
{
    private const string SelectATargetFolder = "Select a target folder";

    public string Show()
    {
        xpFolderBrowserDialog = new WinForms.FolderBrowserDialog
        {
            Description = SelectATargetFolder
        };

        using (xpFolderBrowserDialog)
        {
            return xpFolderBrowserDialog.ShowDialog() == WinForms.DialogResult.OK
                ? xpFolderBrowserDialog.SelectedPath
                : null;
        }
    }
}

var folderPicker = CommonFileDialog.IsPlatformSupported
   ? new VistaFolderPicker()
   : new XPFolderPicker()
\$\endgroup\$
  • \$\begingroup\$ clean and simple (+1) \$\endgroup\$ – dfhwze Aug 6 at 19:50
  • \$\begingroup\$ Thank you for your answer. I like how this separates the two implementations (now that I say it like that it seems obvious that they should be separate). Do you have any suggestions for how I could make this approach more testable? I appreciate that at this level it's essentially a wrapper around Windows APIs but I would still like to test them if possible. \$\endgroup\$ – Toby Smith Aug 7 at 9:25
  • \$\begingroup\$ IMHO there is nothing to test in the IFolderPicker implementations because they just open a dialog. However, dependent object can be tested by providing a mock implementation of the IFolderPicker interface. \$\endgroup\$ – JanDotNet Aug 7 at 16:12
5
\$\begingroup\$

Review

  • You always instantiate inner dialogs for both platforms, even though one will never be called. Your platform will not change at runtime. Use the Lazy pattern to only instantiate the required dialog at first access.
  • Rather than returning the magic path null, I would opt to use a TryShow method returning a boolean whether a path got selected an an out parameter for the path.

Object Destruction

From Programming Guide: Destructors:

The programmer has no control over when the finalizer is called because this is determined by the garbage collector. The garbage collector checks for objects that are no longer being used by the application. If it considers an object eligible for finalization, it calls the finalizer (if any) and reclaims the memory used to store the object.

Since your class is not sealed, derived classes can be made. These classes will not be able to control the order of finalisation code. The order of destructors is fixed as this snipped from the link shows:

protected override void Finalize()  
{  
    try  
    {  
        // Cleanup statements...  
    }  
    finally  
    {  
        base.Finalize();  
    }  
}

That's why you should implement the dispose pattern instead. It allows for more flexibility in flow and distinguishes between disposing managed vs unmanaged code.


Unit Tests

My reasoning for this design is that now the 3 readonly variables can be mocked in unit tests via reflection, while this isn't an ideal way of doing it, it's better than nothing.

When writing code with testability in mind, you don't want to resort to reflection to mock private state of your classes. There are a couple of options available:

  • Make the private methods protected virtual. Mocking frameworks and derived classes are able to override the methods. Partial mocks could be used in this case.
  • Wrap both internal dialogs behind a custom interface, injected into the outer class. The class no longer has dependencies on native win and vista components and interfaces are easy to mock.
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. "These classes will not be able to control the order of finalisation code." - Why is this an issue? \$\endgroup\$ – Toby Smith Aug 6 at 13:57
  • \$\begingroup\$ This may or may not be an issue depending on your design. The point is the dispose pattern allows for more flow control. \$\endgroup\$ – dfhwze Aug 6 at 14:32
  • \$\begingroup\$ Thanks again for your answer, I didn't know about the lazy pattern. I am however going to mark the other answer as accepted because it is the implementation that I finally went with. \$\endgroup\$ – Toby Smith Aug 8 at 10:44
  • \$\begingroup\$ That was also the suggestion I made Wrap both internal dialogs behind a custom interface, injected into the outer class. but no hard feelings :) \$\endgroup\$ – dfhwze Aug 8 at 12:02

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.