Skip to main content
added 795 characters in body
Source Link
RobH
  • 17k
  • 6
  • 37
  • 72

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.

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.

Source Link
RobH
  • 17k
  • 6
  • 37
  • 72

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