3
\$\begingroup\$

I recently asked for some advice on the best way to structure my code for a program I was writing. (see this question on Programmers.SE). The solution I was given was quite elegant, however I am less happy with the code that I've written so that the UI remains responsive.

Here is my code, I will outline the reasons I am not happy with it below:

    private void buttonUpdateContacts_Click(object sender, EventArgs e)
    {
        BackgroundWorker worker = new BackgroundWorker();
        worker.WorkerReportsProgress = true;
        worker.DoWork += new DoWorkEventHandler(worker_DoWork);
        worker.ProgressChanged += new ProgressChangedEventHandler(worker_ProgressChanged);
        worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(worker_RunWorkerCompleted);
        worker.RunWorkerAsync();
    }

    void worker_DoWork(object sender, DoWorkEventArgs e)
    {
        (sender as BackgroundWorker).ReportProgress(0, "Parsing word document...");

        WordDocumentParser wordDocumentParser;
        try
        {
            wordDocumentParser = new WordDocumentParser(Settings.Default.OohSheetLocation);
        }
        catch (FileNotFoundException exception)
        {
            e.Result = new FileNotFoundException("Could not find the word document. File path is: " + exception.FileName, exception);
            return;
        }

        List<TppContact> oohSheetContacts = wordDocumentParser.GetContacts();

        try
        {
            _contactManager = GoogleContactsManager.CreateContactsManager();
        }
        catch (InvalidCredentialsException argumentException)
        {
            e.Result = argumentException;
            return;
        }
        catch (ArgumentNullException nullArg)
        {
            e.Result = nullArg;
            return;
        }

        (sender as BackgroundWorker).ReportProgress(0, "Comparing Google contacts to the word document...");
        var comparer = new ContactComparer(oohSheetContacts, _contactManager);
        _actions = comparer.Compare();
    }

    void worker_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        labelCurrentStatus.Text = e.UserState.ToString();
    }

    void worker_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        if (e.Result != null)
        {
            MessageBox.Show((e.Result as Exception).Message);
        }
        else
        {
            if (_actions.Count == 0)
            {
                textBoxChanges.Text = "There are no changes to be made.";
                buttonApplyChanges.Enabled = false;
            }
            else
            {
                textBoxChanges.Text = string.Empty;
                _actions.ForEach(action => textBoxChanges.Text += action.ChangeDescription + Environment.NewLine);
                buttonApplyChanges.Enabled = true;
            }
        }
    }

    private void button2_Click(object sender, EventArgs e)
    {
        labelCurrentStatus.Text = "Updating Google...";
        _contactManager.UpdateContacts(_actions);

        labelCurrentStatus.Text = "Done.";
    }
  1. I don't like how tied up my code is to the background worker. For example, to handle the exceptions that the various methods can throw, I have to set the result of the background worker to be the exception then handle it in the worker_RunWorkerCompleted method. This also means that I lose the ability to give the user proper error messages such as "invalid username and password supplied". Maybe the answer is to set that as the result rather than the exception?

  2. It goes without saying that I probably shouldn't have all this logic being called from the button clicked method but I am unsure of a better way to do it. This is written in WinForms at the minute but there's very little reason not to write it in WPF.

  3. Am I using the best structure here? I tried to use Tasks but didn't get very far and I've used background workers in the past so I know how they work.

Can you offer any pointers how to make this code "better" or am I doing it the best I can?

\$\endgroup\$
1
\$\begingroup\$

1) I would use the ProgressChanged event with a custom object as my e.UserState which is updated within the code.

// sample worker state class definition
internal class WorkerState
{
    public Exception ExceptionThrown { get; set; }
    public string ProgressMessage { get; set; }
    public int MaximumProgressValue { get; set; }
    public int ProgressValue { get; set; }
    public int Actions { get; set; }
} 

Sample Usage

// this would likely be initialized / reinitiatlized before the RunWorkerAsync is called
private WorkerState m_WorkerState; 

private void buttonUpdateContacts_Click(object sender, EventArgs e)
{
    // for question 2) this could be convered into some form of
    // factory where providing delegates for the ProgressChange and WorkCompleted
    BackgroundWorker worker = new BackgroundWorker();       

    worker.UserState = new WorkerState();

    worker.WorkerReportsProgress = true;      
    worker.DoWork += new DoWorkEventHandler(worker_DoWork);     
    worker.ProgressChanged += new ProgressChangedEventHandler(worker_ProgressChanged);   
    worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(worker_RunWorkerCompleted);   
    worker.RunWorkerAsync();      
}                

void worker_DoWork(object sender, DoWorkEventArgs e)         
{ 
    Worker worker = (sender as BackgroundWorker);
    WorkerState state = worker.UserState;

    state.UserState.ProgressMessage = "Parsing word document...";
    state.UserState.MaximumProgressValue = 100;
    state.UserState.ProgressValue = 0;

    worker.ReportProgress(0, state);          

    // ...

    try
    {
        // do something
    }
    catch (InvalidCredentialsException argumentException)
    {
        e.ExceptionThrown = argumentException;
        return;
    }
    catch (ArgumentNullException nullArg)
    {
        e.ExceptionThrown = nullArg;
        return;
    }

    // ..
} 

2) There is not too much wrong with putting the code in the button click event but you could make better use of OO concepts like factories for creating the BackgroundWorker component etc. I personally would split the bulk of the code in the DoWork method into seperate classes following SOLID principles (especially the Single Responsibility... this class is responsible for blah, blah blah).

3) Tasks are likely to be unsuitable in this case as this is exactly what the BackgroundWorker component was designed for.

\$\endgroup\$
  • \$\begingroup\$ I like the idea of using a custom class for the UserState on the background worker, it's much more obvious what's happening and what I'm trying to report. I'm not so sure how to accomplish what you suggested for part 2 though. The work that I'm actually doing is already split into separate classes (eg: GoogleContactManager). Could you maybe elaborate on how you would write this code, possibly with an example? Also, I don't understand how a factory method for the background worker would help? Do you mean create a separate class for this specific piece of functionality? \$\endgroup\$ – Stuart Leyland-Cole Jan 3 '12 at 10:34

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.