Is it necessary to move OpenFileDialog code for opening the files and put it into a separate method?

Question

I want to separate the code inside my Add button click event from the code in order to make my code looks more arranged. this is my code before these changes (i am using BackgroundWorker in order to avoid my GUI to freeze because every file that I am choosing need to open process and check if this is file OK before add this file into my Listbox):

private void btnAddfiles_Click(object sender, EventArgs e)
{
    System.IO.Stream stream;

    if (openFileDialog1.ShowDialog() == DialogResult.OK)
    {
        fileSelected();
        if (openFileDialog1.FileNames.Length > 0)
            lastPath = Path.GetDirectoryName(openFileDialog1.FileNames[0]);

        ListboxFile lbf = new ListboxFile();
        lbf.OnFileAddEvent += lbf_OnFileAddEvent;

        BackgroundWorker backgroundWorker = new BackgroundWorker();
        backgroundWorker.WorkerReportsProgress = true;
        backgroundWorker.DoWork +=
        (s3, e3) =>
        {
            foreach (String file in openFileDialog1.FileNames)
            {
                try
                {
                    if ((stream = openFileDialog1.OpenFile()) != null)
                    {
                        int numberOfFiles = openFileDialog1.SafeFileNames.Length;
                        using (stream)
                        {
                            lbf.checkFile(file);
                            lastPath = Path.GetDirectoryName(openFileDialog1.FileNames[0]);
                        }
                    }
                }

                catch (Exception ex)
                {
                    MessageBox.Show("Error: Could not read file from disk. Original error: " + ex.Message);
                }
            }
        };

        backgroundWorker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(
            (s3, e3) =>
            {
                //update my gui
            });

        backgroundWorker.RunWorkerAsync();
    }
}

and this is after the changes:

private void btnAddfiles_Click(object sender, EventArgs e)
{
    if (openFileDialog1.ShowDialog() == DialogResult.OK)
    {
        fileSelected();
    }
}

private void fileSelected()
{
    if (openFileDialog1.FileNames.Length > 0)
        lastPath = Path.GetDirectoryName(openFileDialog1.FileNames[0]);

    ListboxFile lbf = new ListboxFile();
    lbf.OnFileAddEvent += lbf_OnFileAddEvent;

    BackgroundWorker backgroundWorker = new BackgroundWorker();
    backgroundWorker.WorkerReportsProgress = true;
    backgroundWorker.DoWork +=
    (s3, e3) =>
    {
        foreach (String file in openFileDialog1.FileNames)
        {
            System.IO.Stream stream;
            try
            {
                if ((stream = openFileDialog1.OpenFile()) != null)
                {
                    int numberOfFiles = openFileDialog1.SafeFileNames.Length;
                    using (stream)
                    {
                        lbf.checkFile(file);
                        lastPath = Path.GetDirectoryName(openFileDialog1.FileNames[0]);
                    }
                }
            }

            catch (Exception ex)
            {
                MessageBox.Show("Error: Could not read file from disk. Original error: " + ex.Message);
            }
        }
    };

    backgroundWorker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(
        (s3, e3) =>
        {
            //update my gui
        });

    backgroundWorker.RunWorkerAsync();
}

So, what I did is necessary ? Maybe something else was better ?

public class ListboxFile
{
    public delegate void OnFileAdd(string file);
    public event OnFileAdd OnFileAddEvent;
    private static List<string> _files;

    public ListboxFile()
    {
        _files = new List<string>();
    }

    private void add(string file)
    {
        _files.Add(file);
        OnFileAddEvent(file);
    }

    public void remove(string file)
    {
        if (_files.Contains(file))
        {
            _files.Remove(file);
        }
    }

    public void clear()
    {
        _files.Clear();
    }

    public void checkFile(string file)
    {
        if ((new Editcap()).isLibpcapFormat(file))
        {
            add(file);
        }
    }

    public List<string> list
    {
        get { return _files; }
    }
}

Answer 1

well in part it helps, but you need to keep taking it a bit further. Think of it as a building blocks. Button_Click is the top most level and should be readable like a book. have the method state its intent. maybe this might have been a better idea

private void btnaddfiles_click(object sender, EventArgs e)
{
    GetFolderToAddFilesFrom();
}

Now when someone comes in and wants to know exactly what the button does they don't have to read the source code and figure out what it does. The method states very clearly what it is supposed to do. The further you go down the list the more details you add.

so what should the next level look like? Maybe something like this.

    private void GetFolderToAddFilesFrom()
    {
        if (openFileDialog1.ShowDialog() == DialogResult.OK)
        {
            CheckFilesInFolder();
        }
        if (folderBrowserDialog1.ShowDialog() == DialogResult.OK)
        {
            CheckFilesInFolder();
        }
    }

as you see I have 2 dialogs now. One was yours, the other is simply a folder browser. Judging by the code you provided I would say the FolderBrowser is the actual control you want and not the other. Either way you only need one. But they do the same thing. Once you've selected a Folder you then go to check the files in the folder. fileSelected() tells us the state your openFileDialog is in, not what the method does. We want all our methods to tell us what they do. Hence, CheckFilesInFolder() Lets gander at that method now. Notice how each level gets more and more detailed, but we still aren't doing any nitty gritty work. It's just easy to read right now.

    private void CheckFilesInFolder()
    {
        if (openFileDialog1.FileNames.Length > 0)
        {
            lastPath = Path.GetDirectoryName(openFileDialog1.FileNames[0]);
        }
        lastPath = folderBrowserDialog1.SelectedPath;

        ListboxFile lbf = new ListboxFile();
        lbf.OnFileAddEvent += lbf_OnFileAddEvent;

        StartBackgroundFileChecker();
    }

I still have both options in use for the folder selector. I believe the second one is the one you are after. Plus it is only 1 line long. The previous call told us we are going to be checking the Files in the Folder. Although we don't do that we are getting ready to do that. So maybe we could have chose another name such as PrepareToCheckFilesInFolder() but the last line does start the checker, therefore it isn't the worst name in the world. And you can guess what the last method looks like i'm sure.

To answer your question then it is not necessary to move it out because obviously it still works either way. But you have to ask yourself if you'll know what that method does a month from now. Will you have to re-read your code and figure out what it does or will you be able to read the method name and know EXACTLY what it does. The second is the preferred way. Now you can reuse the code elsewhere if you need. If someone else comes in and reads your code they won't be temped to repeat the code you have, they'll just use your code. This is preferred.

Last little tip is that you don't use your stream. Even if you did end up using your stream you are using using incorrectly.

consider something like this

        foreach (string file in Directory.GetFiles(lastPath))
        {
            try
            {
                using (var stream = new System.IO.FileStream(file, FileMode.Open, FileAccess.Read))
                {
                    lbf.checkFile(file);
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show("Error");
            }
        }

but since you don't use stream it would appear that this works just as well

        foreach (string file in Directory.GetFiles(lastPath))
        {
            try
            {
                //stream was not used, and is not included here.
                lbf.checkFile(file);
            }
            catch (Exception ex)
            {
                MessageBox.Show("Error: Could not read file from disk. Original error: " + ex.Message);
            }
        }

You may notice that i took out the check for more than 0 files in the directory. Foreach does that for me because if the list is empty it just skips the foreach statement.