1
\$\begingroup\$

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<string> _files;

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

    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<string> 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 > 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 > 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) =>
            {
                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) =>
                {
                    updateUi("bla bla"); //update the gui
                });

            backgroundWorker.RunWorkerAsync();
        }
    }

Would love to hear comments about this design since I am new developer.

\$\endgroup\$
  • 1
    \$\begingroup\$ This looks a lot more complex than it needs to be, can you explain what you are trying to achieve with the code? Obviously the user is selecting some files to be added to a listbox but your background worker and related code is a bit odd and not really obvious why you're doing what you're doing. \$\endgroup\$ – RobH May 24 '13 at 12:14
  • \$\begingroup\$ As i mention before i am new developer so i don't have much experience and i would glad for some help, can you show me batter way ? \$\endgroup\$ – user2214609 May 24 '13 at 13:02
  • 2
    \$\begingroup\$ need to know what you are trying to achieve with your code before I can suggest a better way. \$\endgroup\$ – RobH May 24 '13 at 14:29
  • \$\begingroup\$ I only use BackgroundWorker instead Thread in order to avoid my GUI stuck \$\endgroup\$ – user2214609 May 24 '13 at 15:01
1
\$\begingroup\$

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.

  1. 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.
  2. Take the code for opening the files and put it into a separate method.
  3. 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<T>. That will handle automatically updating your control as items are added/removed.
\$\endgroup\$
  • \$\begingroup\$ What do u mean by saying move all the OpenFileDialog setup to the designer file ? can you show me the relevant code ? \$\endgroup\$ – user2214609 May 24 '13 at 20:24
  • \$\begingroup\$ Thanks for your reply Dan Lyons, it will be rude to ask code example for all this points ? i am quite new and don't know how to deal with all this \$\endgroup\$ – user2214609 May 24 '13 at 20:31

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.