Class design question I have Winform application and I want to separate the button click event from the main form.

My application has Add button which adds files into my Listbox and until now all my code was inside this button click event and caused my code to look complicated.
So I created this class:

public class ListboxFile
{
    public delegate void OnFileAdd(string file);
    public event OnFileAdd OnFileAddEvent;
    private static List&lt;string&gt; _files;

    public ListboxFile()
    {
        _files = new List&lt;string&gt;();
    }

    public 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 List&lt;string&gt; list
    {
        get { return _files; }
    }
}


in the constructor I am initiate my List and each file that comes and add to my List raise an event and from the main form this file also add to my Listbox

this is the event from the main form that responsible to add the file into the Listbox:

private void lbf_OnFileAddEvent(string file)
{
    if (InvokeRequired)
    {
        this.Invoke((MethodInvoker)delegate
        {                    
            listBoxFiles.Items.Add(file);
        });
    }
    else
    {
        listBoxFiles.Items.Add(file);
    }
}


and this is the Add button click that takes the files that the user chooses to add:

    private void btnAddfiles_Click(object sender, EventArgs e)
    {
        string fileToAdd = string.Empty;
        System.IO.Stream stream;
        OpenFileDialog thisDialog = new OpenFileDialog();
        thisDialog.InitialDirectory = (lastPath.Length &gt; 0 ? lastPath : "c:\\");
        thisDialog.Filter = "(*.snoop, *.pcap, *.cap, *.net, *.pcapng, *.5vw, *.bfr, *.erf, *.tr1)" +
            "|*.snoop; *.pcap; *.cap; *.net; *.pcapng; *.5vw; *.bfr; *.erf; *.tr1|" + "All files (*.*)|*.*";
        thisDialog.FilterIndex = 1;
        thisDialog.RestoreDirectory = false;
        thisDialog.Multiselect = true;
        thisDialog.Title = "Please Select Source File";

        if (thisDialog.ShowDialog() == DialogResult.OK)
        {
            if (thisDialog.FileNames.Length &gt; 0)
            {
                lastPath = Path.GetDirectoryName(thisDialog.FileNames[0]);
            }

            ListboxFile lbf = new ListboxFile();
            lbf.OnFileAddEvent += lbf_OnFileAddEvent;
            BackgroundWorker backgroundWorker = new BackgroundWorker();
            backgroundWorker.WorkerReportsProgress = true;
            backgroundWorker.DoWork +=
            (s3, e3) =&gt;
            {
                foreach (String file in thisDialog.FileNames)
                {
                    try
                    {
                        if ((stream = thisDialog.OpenFile()) != null)
                        {
                            int numberOfFiles = thisDialog.SafeFileNames.Length;
                            using (stream)
                            {
                                lbf.add(file); //add the file
                                lastPath = Path.GetDirectoryName(thisDialog.FileNames[0]);
                            }
                        }
                    }

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

            backgroundWorker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(
                (s3, e3) =&gt;
                {
                    updateUi("bla bla"); //update the gui
                });

            backgroundWorker.RunWorkerAsync();
        }
    }


Would love to hear comments about this design since I am new developer.
My first reaction is that your click handler is doing way too much work on its own.  You should break up the code into smaller methods to make it easier to maintain.


Move all the OpenFileDialog setup to the designer file.  This will make it a field rather than local variable, preventing unnecessary object creation when a user clicks Add multiple times.  Additionally, it will wire-up the proper Dispose call for you.
Take the code for opening the files and put it into a separate method.
Split off your worker event handlers into class methods rather than anonymous methods, better separating out method responsibilities.


Other things I would do:


Pass the filenames to the BackgroundWorker in its RunWorkerAsync(Object) overload.  This prevents synchronization issues if a user clicks add while the worker is still running.
Folowing the Single Responsibility Principle, you should move the file testing into the ListBoxFile class or some other helper class.  UI views should not typically handle I/O.
Make ListBoxFiles._files non-static.  If you want singleton behavior for some reason, you can keep a static reference to a ListBoxFiles object.  Providing instance-based methods but backing them with a static collection could be a bit jarring to callers, failing the Principle of Least Surprise.
Instead of using Stream, it may be more clear to use File.OpenRead(string).
With either Stream or File.OpenRead, you need to dispose the streams
Instead of raising an add event on ListBoxFile, you should instead data bind your list control to a BindingList&lt;T&gt;.  That will handle automatically updating your control as items are added/removed.