BackgroundWorker vs TPL ProgressBar Exercise

Question

I wanted to fiddle around with the BackgroundWorker and Task classes, to see how I would implement a given task with these two techniques.

So I created a new WinForms project, and implemented a simple UI; two sections, each with a ProgressBar, and Start + Cancel buttons:

I implemented a DoSomething-type "service":

public class SomeService
{
    public void SomeMethod()
    {
        Thread.Sleep(1000);
    }
}

But that's irrelevant. The form's code-behind is where I put all the code this post is all about:

Constructor

The form's constructor is essentially the entry point here (program.cs is ignored), so I put the obvious fields in first, and initialized them in the constructor:

public partial class Form1 : Form
{
    private readonly SomeService _service;
    private readonly BackgroundWorker _worker;

    public Form1()
    {
        _service = new SomeService();
        _worker = new BackgroundWorker { WorkerReportsProgress = true, WorkerSupportsCancellation = true };

        _worker.DoWork += OnBackgroundDoWork;
        _worker.ProgressChanged += OnBackgroundProgressChanged;
        _worker.RunWorkerCompleted += OnBackgroundWorkerCompleted;

        InitializeComponent();
        
        CloseButton.Click += CloseButton_Click;
        
        StartBackgroundWorkerButton.Click += StartBackgroundWorkerButton_Click;
        CancelBackgroundWorkerButton.Click += CancelBackgroundWorkerButton_Click;

        StartTaskButton.Click += StartTaskButton_Click;
        CancelTaskButton.Click += CancelTaskButton_Click;
    }

    private void CloseButton_Click(object sender, EventArgs e)
    {
        CancelBackgroundWorkerButton_Click(null, EventArgs.Empty);
        CancelTaskButton_Click(null, EventArgs.Empty);
        Close();
    }

#region BackgroundWorker

Button.Click handlers

These are the event handlers for the Start and Cancel buttons' Click event:

    private void StartBackgroundWorkerButton_Click(object sender, EventArgs e)
    {
        StartBackgroundWorkerButton.Enabled = false;
        _worker.RunWorkerAsync();
    }

    private void CancelBackgroundWorkerButton_Click(object sender, EventArgs e)
    {
        CancelBackgroundWorkerButton.Enabled = false;
        _worker.CancelAsync();
    }

BackgroundWorker.DoWork handler

Here we are. The "work" here, will be to call our "time-consuming operation" 5 times, reporting progress as we go, and assigning the DoWorkEventArgs.Cancel as needed:

    private void OnBackgroundDoWork(object sender, DoWorkEventArgs e)
    {
        //CancelBackgroundWorkerButton.Enabled = true; // this call fails the background task (e.Error won't be null)
        Invoke((MethodInvoker)(() => { CancelBackgroundWorkerButton.Enabled = true; }));

        var iterations = 5;
        for (var i = 1; i <= iterations; i++)
        {

            if (_worker.CancellationPending)
            {
                e.Cancel = true;
                return;
            }

            _service.SomeMethod();

            if (_worker.CancellationPending)
            {
                e.Cancel = true;
                return;
            }

            _worker.ReportProgress(100 / (iterations) * i);
        }

        Thread.Sleep(500); // let the progressbar animate to display its actual value.
    }

ProgressChanged handler

This handler assigns the new value to the ProgressBar control owned by the UI thread:

    private void OnBackgroundProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        // BGW facilitates dealing with UI-owned objects by executing this handler on the main thread.
        BackgroundWorkerProgressBar.Value = e.ProgressPercentage;
        if (BackgroundWorkerProgressBar.Value == BackgroundWorkerProgressBar.Maximum)
        {
            CancelBackgroundWorkerButton.Enabled = false;
        }
    }

WorkerCompleted handler

If I get the BackgroundWorker right, this is where we can determine whether the thing has succeeded, failed with an error or was cancelled:

    private void OnBackgroundWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        if (e.Cancelled)
        {
            MessageBox.Show("BackgroundWorker was cancelled.", "Operation Cancelled", MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
        }
        else if (e.Error != null)
        {
            MessageBox.Show(string.Format("BackgroundWorker operation failed: \n{0}", e.Error), "Operation Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
        else
        {
            MessageBox.Show("BackgroundWorker completed.", "Operation Completed", MessageBoxButtons.OK, MessageBoxIcon.Information);
        }

        ResetBackgroundWorker();
    }

    private void ResetBackgroundWorker()
    {
        BackgroundWorkerProgressBar.Value = 0;
        StartBackgroundWorkerButton.Enabled = true;
        CancelBackgroundWorkerButton.Enabled = false;
    }
    #endregion

---

#region Task

I don't get to write this kind of code very often, so I'm very interested in this part. I declared a private field and handled the Click event like this:

    CancellationTokenSource _cancelTokenSource;
    private void StartTaskButton_Click(object sender, EventArgs e)
    {
        StartTaskButton.Enabled = false;

        _cancelTokenSource = new CancellationTokenSource();
        var token = _cancelTokenSource.Token;

        var task = Task.Factory.StartNew(DoWork, token);
        task.ContinueWith(t =>
            {
                switch (task.Status)
                {
                    case TaskStatus.Canceled:
                        MessageBox.Show("Async task was cancelled.", "Operation Cancelled", MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
                        break;
                    case TaskStatus.Created:
                        break;
                    case TaskStatus.Faulted:
                        MessageBox.Show(string.Format("Async task failed: \n{0}", t.Exception), "Operation Failed", MessageBoxButtons.OK, MessageBoxIcon.Error);
                        break;
                    case TaskStatus.RanToCompletion:
                        MessageBox.Show("Async task completed.", "Operation Completed", MessageBoxButtons.OK, MessageBoxIcon.Information);
                        break;
                    case TaskStatus.Running:
                        break;
                    case TaskStatus.WaitingForActivation:
                        break;
                    case TaskStatus.WaitingForChildrenToComplete:
                        break;
                    case TaskStatus.WaitingToRun:
                        break;
                    default:
                        break;
                }

                ResetTask();
                _cancelTokenSource = null;
            });
    }

DoWork Action

Instead of inlining the DoWork method, I wrote it as a private void parameterless method:

    private void DoWork()
    {
        // CancelTaskButton.Enabled = true; // fails the background thread.
        Invoke((MethodInvoker)(() => { CancelTaskButton.Enabled = true; }));

        var iterations = 5;
        for (var i = 1; i <= iterations; i++)
        {
            _cancelTokenSource.Token.ThrowIfCancellationRequested();
           _service.SomeMethod();
           _cancelTokenSource.Token.ThrowIfCancellationRequested();
           
            var progress = 100 / (iterations) * i;
            Invoke((MethodInvoker)(() => { TaskProgressBar.Value = progress; }));
            if (i == iterations)
            {
                Invoke((MethodInvoker)(() => { CancelTaskButton.Enabled = false; }));
            }
        }

        Thread.Sleep(500); // let the progressbar animate to display its actual value.
    }

Cancellation

In the Click handler for the Cancel button, I called Cancel() on the token source:

    private void CancelTaskButton_Click(object sender, EventArgs e)
    {
        CancelTaskButton.Enabled = false;

        // token source is null if Close button is clicked without task started
        if (_cancelTokenSource != null) 
        {
            _cancelTokenSource.Cancel();
        }
    }

    private void ResetTask()
    {
        Invoke((MethodInvoker)(() =>
            {
                TaskProgressBar.Value = 0;
                StartTaskButton.Enabled = true;
                CancelTaskButton.Enabled = false;
            }));
    }
    #endregion

---

Of course this is just an exercise. The question is, is it well conducted? Are there suspicious patterns in the coding style?

In the BGW code, I don't like that I'm accessing _worker.CancellationPending, but it works (the CancellationPending property must be thread-safe then). Is this correct usage?

In the TPL code, I don't like that switch (task.Status) block. That can't be the best way to go about it?!

Answer 1

It does show very good example for the progressbar in WinForms. Here are my code-review outputs for you.

• Closebutton_Click calls the other click "handlers". This is not the common way. Implement another method and call it from those handlers.
• I couldnt understand the logic why are you asking the cancellation before and after invoking the DoSomething().
• Reduce synchronization. Try to "synchron" your threads at once. Optimize method invocation delegate for progressbar.value and the canceltaskbutton.enabled.
• Your .ContinueWith approach is good. But always eliminate 'switch’s from your code. Here would be a dictionary very handy. You can define your status-action set before hand and .ContinueWith status keyed action. This would reduce the complexity and increase the readability of your code.

• On the OnBackgroundWorkerCompleted method, the Else-if should be separated from the first if-expression. (Again complexity and readability) Please see following code.

  if (e.Error != null)
  {
      // Handle Error and call reset.
      return; 
  }
  
  if (e.Cancelled)
  {
      // Handle cancel and call reset.
      return;
  }
  
  
  if (e.Result == null)
  {
      // No Result
  }
  else
  {
      // Result
  }
  
  // reset ...
  

Answer 2

1. I think the OnXxxxxx naming for event handlers is more common, so StartBackgroundWorkerButton_Click should probably be OnStartClicked or something. Either way, you should choose single naming notation and stick to it.

2. private void CloseButton_Click(object sender, EventArgs e)
   {
       CancelBackgroundWorkerButton_Click(null, EventArgs.Empty);
       CancelTaskButton_Click(null, EventArgs.Empty);
       Close();
   }
   

   should be:

   private void CloseButton_Click(object sender, EventArgs e)
   {
       CancelBackgroundWorker();
       CancelTask();
       Close();
   }
   
   private void CancelBackgroundWorkerButton_Click(object sender, EventArgs e)
   {
       CancelBackgroundWorker();
   }
   

3. if (BackgroundWorkerProgressBar.Value == BackgroundWorkerProgressBar.Maximum)
   {
       CancelBackgroundWorkerButton.Enabled = false;
   }
   

   I think this is a bad idea. If you want do disable a cancel button when bgw finished working - then you should do so in appropriate event handler (and you already do!). Doing so depending on progress bar state is a lousy solution.

4. I don't get this switch hate i see everywhere. Yes, you can replace it with Dictionary, yes, it will look more fabulous. But frankly, there is no difference what so ever in such simple case. I would even prefer a switch (or if-else) solution, because it does not force me to go look for actual mapping somewhere else. switch is there for a reason and it is certainly not something you should "always eliminate". What you can do is remove empty cases, the default: operator should deal with those.

5.     if (i == iterations)
       {
           Invoke((MethodInvoker)(() => { CancelTaskButton.Enabled = false; }));
       }
   

   You can simply call Invoke outside the loop.