HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewevent-handlingwpfcsharpstackoverflowc#delegates
theprettyneedbutterflywaytransformdelegatedoesandevent

Problem

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);

///
/// Brings the Open File Dialog and asks the user to choose
/// the file. Also handles the exceptions.
///
///
///
///
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;
}

///
/// Checks to see if the file is ready to be used
/// meaning it's not locked since being open etc.
//

Solution

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 `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 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`.

Code Snippets

var dialog = new OpenFileDialog();
SomeObject a = new SomeObject();
a.Foo = "123";
a.Bar = "456";
a.Baz = "789";
SomeObject a = new SomeObject
    { 
        Foo = "123",
        Bar = "456",
        Baz = "789"
    };
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;
}
public static string Open(string title, string fileType = "xlsx")
{
    return Open(title, fileType, s => s);
}

Context

StackExchange Code Review Q#76942, answer score: 5

Revisions (0)

No revisions yet.