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:

MainForm at startup. Two progressbars, each with an enabled "Start" and a disabled "Cancel" button.

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

---

Both jobs can be cancelled.

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?!

• svick: BTW, you might be interested in Stephen Cleary's Task.Run vs BackgroundWorker series (unless you already read that and it was what prompted this).
• Mathieu Guindon: Thanks! It's actually aimed at showing a fellow reviewer how to deal with progressbar in C# / WinForms - I hope it does a not-so-bad job at it!
• Mathieu Guindon: I originally wanted the task code to .ContinueWith and show the appropriate message box, but couldn't get it to work, so I resorted to switching on task.Status.

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 ...
  

• Mathieu Guindon: +1 Nice review. The 2nd cancellation check is to avoid updating the progressbar if cancellation is pending. Without it the user has to wait for the progressbar animation to complete before they get the "Operation Cancelled" message box.

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.

• Kaan: Behavioral mappings via switch-case should be "eliminated". There are many reasons for that. Readability, Complexity, Extensibility, ... The statement 'always' might be to much though :) See also this
• Nikita B: @Kaan, well, yes and no. I do agree that Dictionary is more extendable and flexible solution. If you require those two - then by all means - go for it. I this code sample it is not the case though. I do not think that switch is less readable than dictionaries. Its the opposite, imho. And i fail to see how does complexity differs between the two approaches. In general i think that the sipliest solution is the best (YAGNI, right?). But nowadays, i guess, its all about building frameworks :)