6
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I didn't see anything in your "why you should replace events with delegates" link that suggested replacing events with delegates at all. The way I like to think of a delegate is as a template for a bit of functionality that can be changed out or added to without needing to subclass. An event I see as a trigger to that can be subscribed to or ignored. In essence, an event in my eyes can be left null with no consequences, a delegate should be fulfilled as the code using it requires it to complete. Your mileage may vary. \$\endgroup\$ – Nick Udell Jan 8 '15 at 13:22
  • \$\begingroup\$ What's the point of using FileManager.Open(title, fileType, doWithFileHandler) instead of doWithFileHandler(FileManager.Open(title, fileType))? Is it just to simplify handling nulls? \$\endgroup\$ – svick Jan 10 '15 at 20:25
  • \$\begingroup\$ @svick, to be frank I wasn't thinking of putting it that way. That's all. Would the second form give me any advantage? Maybe readability or simplicity I guess. \$\endgroup\$ – Mehrad Jan 10 '15 at 22:38
  • 1
    \$\begingroup\$ OpenFileDialog implements IDisposable and thus its lifetime should be wrapped in a using construct. \$\endgroup\$ – Jesse C. Slicer Jan 12 '15 at 16:29
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Rob, first of all thanks you for the handful of tips and comments. This is what I'm after all the time. Great learning. I'll dig into your answer and shoot you couple of questions. However, I'm really curious to get your recommendation about couple of my variables and methods names. I have been criticised about my naming habits in the past and nobody actually was up for some suggestions so if you do me a favour I'd really learn from that. Cheers \$\endgroup\$ – Mehrad Jan 9 '15 at 12:36
  • 1
    \$\begingroup\$ Rob, great answer. Can't tell how much I have learned from it. In this case I have change my class name from FileManager to DialogProvider and since I use this in other places in the application and with other file types other than xls, I kept it more general. Not the best name I could go for but after reading the articles you provided it make whole lot of more sense in comparison to the (apparently) not deeply loved Manager post-fix. Cheers. \$\endgroup\$ – Mehrad Jan 14 '15 at 2:04
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ So you belive I should be using an event here rather? \$\endgroup\$ – Mehrad Jan 9 '15 at 12:28
  • \$\begingroup\$ @Mehrad Actually, your delegate are calling right before the function is exit. in the flow that you shows currently, there is no reasons for delegate or event. You can pass another helper class(in parameters of function open), that do all work. \$\endgroup\$ – Avram Jan 10 '15 at 9:16

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.