Is my Delegate defined the right way and does it need to transform to a pretty Event butterfly?

Question

I have read so many bed time stories and ins and outs of how delegates work and why events are to be replaced with delegates (surely not all the time) and just couldn't get my head around it.

I am trying to use delegates in a more functional way in my code rather than practice runs in the LinqPad with dummy examples and this is my first go.

In this case I have a FileManager helper class which I can reach out anytime and use it to bring up the Open File Dialog with the retry functionality if the file is still locked and under use.

In one stage I was doing different operations with the file received through the OpenFileDialog so I decided to pass the functionality through a delegate to my FileManager.Open() method.

First of all, let me know if I am implementing this using delegates the right way. In the next step, I am eager to learn what would I earn from transforming this delegate to an event in this particular case and how would I do that?

FileManager.cs

public class FileManager
{
    public delegate string DoWithFile(string file);

    /// <summary>
    /// Brings the Open File Dialog and asks the user to choose 
    /// the file. Also handles the exceptions.
    /// </summary>
    /// <param name="title"></param>
    /// <param name="fileType"></param>
    /// <returns></returns>
    public static string Open(string title, string fileType, DoWithFile doWithFile)
    {
        OpenFileDialog openFileDialog = new OpenFileDialog();
        openFileDialog.DefaultExt = "." + fileType;
        openFileDialog.Filter = string.Format("Excel file (*.{0})|*.{0}", fileType);
        openFileDialog.RestoreDirectory = false;
        openFileDialog.Title = title;

        if (openFileDialog.ShowDialog() == DialogResult.OK)
        {
            if (FileIsReady(openFileDialog.FileName))
                return doWithFile(openFileDialog.FileName);
            //return openFileDialog.FileName;
        }
        return null;
    }

    /// <summary>
    /// Checks to see if the file is ready to be used
    /// meaning it's not locked since being open etc.
    /// </summary>
    /// <param name="fileName"></param>
    /// <returns></returns>
    private static bool FileIsReady(string fileName)
    {
        try
        {
            if (fileName != null)
            {
                // This is to make sure if the file isn't open atm.
                using (Stream stream = new FileStream(fileName, FileMode.Open))
                {
                    return true;
                }
            }
        }
        catch (IOException)
        {
            // Retry mechanism 
            if (System.Windows.Forms.MessageBox.Show(
                                                "Error: Could not read file from disk.\r\n" +
                                                 "Try closing the file if it is still open and try again.",
                                                 "Cannot open the file",
                                                 MessageBoxButtons.RetryCancel,
                                                 MessageBoxIcon.Exclamation,
                                                 MessageBoxDefaultButton.Button1
                                                 ) == DialogResult.Retry)
            {
                return FileIsReady(fileName);
            }
        }
        return false;
    }
}

MainWindowViewModel.cs

/// <summary>
/// Opens the FileOpenDialog and asks the user to choose the pricing 
/// spreadsheet.
/// </summary>
/// <returns></returns>
private string ChoosePricingSpreadsheet()
{
    // Parameters are OpenFileDialog header, file type filtered and Delegate
    return FileManager.Open("Select the pricing spreadsheet", "xlsx", doWithFileHandler);
}

// Instantiating the delegate with the method we want to be passed to FileManager.Open
FileManager.DoWithFile doWithFileHandler = ReturnFileIfReady;

/// <summary>
/// The action need to be done with the file opened using FileManager.Open
///     In this case we want the file which user have chosen in the 
///     OpenFileDialog to be passed to the program.
/// </summary>
/// <param name="file"></param>
/// <returns></returns>
private static string ReturnFileIfReady(string file)
{
    return file;
}

Answer 1

I don't think it makes sense for this to be an event. What you want to be able to do is pass in an additional processing step. An event is something that may happen in the future for you to react to.

General code notes:

Use of var

If the right hand side of an assignment makes the type obvious, use var to declare the variable.

var dialog = new OpenFileDialog(); 

Object initializers

Anytime you do:

SomeObject a = new SomeObject();
a.Foo = "123";
a.Bar = "456";
a.Baz = "789";

It can be written as (which is generally more readable):

SomeObject a = new SomeObject
    { 
        Foo = "123",
        Bar = "456",
        Baz = "789"
    };

Comments

It's great to see Xml documentation comments but don't include tags unless you intend on filling them in! E.g. you have lots of empty <param />s

Comments should explain the what/why not the how. "Also handles the exceptions." isn't helpful.

Delegates

It's very rare to need to define your own delegates as there are a couple of general purpose ones built in: Action and Func.

Naming

Your names could be better. I loath classes called XxxxxManager - so do a lot of people.

Your Open method is also badly named as it is adds "Excel" to the filter - it's not as general as the name sounds.

Application

Let's see how your code could look. I haven't changed the names to make things easier to compare.

public static string Open(string title, string fileType, Func<string, string> processor)
{
    if (processor == null)
    {
        throw new ArgumentNullException("processor");
    }
   // Add similar guards for title and fileType.

    var openFileDialog = new OpenFileDialog
        {
            DefaultExt = "." + fileType,
            Filter = string.Format("Excel file (*.{0})|*.{0}", fileType),
            RestoreDirectory = false,
            Title = title
        };

    return openFileDialog.ShowDialog() == DialogResult.OK && FileIsReady(openFileDialog.FileName) 
        ? processor(openFileDialog.FileName) 
        : null;
}

private static bool FileIsReady(string fileName)
{
    try
    {
        if (fileName != null)
        {
            // This is to make sure if the file isn't open atm.
            using (new FileStream(fileName, FileMode.Open))
            {
                return true;
            }
        }
    }
    catch (IOException)
    {
        // Retry mechanism 
        if (MessageBox.Show(
                "Error: Could not read file from disk." + Environment.NewLine +
                    "Try closing the file if it is still open and try again.",
                    "Cannot open the file",
                    MessageBoxButtons.RetryCancel,
                    MessageBoxIcon.Exclamation,
                    MessageBoxDefaultButton.Button1
                    ) == DialogResult.Retry)
        {
            return FileIsReady(fileName);
        }
    }
    return false;
}

You could also add in an overload which handles the case where you don't want to do anything to the filename and add a default for the extension:

public static string Open(string title, string fileType = "xlsx")
{
    return Open(title, fileType, s => s);
}

This will just return the filename. This overload could be used by your MainWindowViewModel (bad name btw):

private string ChoosePricingSpreadsheet()
{
    return FileManager.Open("Select the pricing spreadsheet");
}

Having said all of that, why do you need this at all?! If you're returning the filename anyway, there is no reason I can think of to add a hook into the filename called at the time of the method.

Why not just do:

public static string Open(string title, string fileType)
{
    if (title== null)
    {
        throw new ArgumentNullException("title");
    }
    if (fileType== null)
    {
        throw new ArgumentNullException("fileType");
    }
    var openFileDialog = new OpenFileDialog
        {
            DefaultExt = "." + fileType,
            Filter = string.Format("Excel file (*.{0})|*.{0}", fileType),
            RestoreDirectory = false,
            Title = title
        };

    return openFileDialog.ShowDialog() == DialogResult.OK && FileIsReady(openFileDialog.FileName) 
        ? openFileDialog.FileName
        : null;
}

Edit

Further to my comment about your naming. Consider your delegate:

delegate string DoWithFile(string file);

It's actually a template for a function on a file name. The parameter is the filename too. It's worth checking MS guidelines

delegate string OpenFilenameCallback(string filename);

But, delegates are hard to name when they're not used in an event handler...

FileManager.Open sounds like I can open anything. However, you add "Excel file" to the filter. I think you should remove the fileType parameter and rename either the class or the method. e.g. SpreadsheetFileManager.Select or FileManager.SelectExcelFile.

FileIsReady would sound better to me as CheckFileReady.

Answer 2

You don't have any problem. :)
The usage must be events, not delegates.

I read carefully your link, he saying use events (much safer) not delegates.

Actually, in most cases you don't need delegates, there is prepared standard delegates from Microsoft (like Action), that good enough for most cases.