Creating multiple threads to process items in a listview

Question

The goal is to pick items from a listview, do some web related task with the information and then update the UI with the returned information from the web request.

The problem is with having multiple list view items processed at once, me being a newbie to C#, i came up with this code, I'm pretty sure this is not good, i just came up with it, is there a better way to get this done ?

Dictionary<String,BackgroundWorker> worker_list = new Dictionary<String, BackgroundWorker>();
int completed_count = 0;
int total_work;

public Form1()
{
    InitializeComponent();
}

public void worker(String woker_id)
{
    WebClient client = new WebClient();
    byte[] response =
    client.UploadValues("http://google.com", new NameValueCollection() { });

    worker_list[woker_id].ReportProgress(100);

    if(completed_count != total_work){
        worker(woker_id);
    }
}

// Adding the work to the que since a thread can't access background Ui thread
private void button2_Click(object sender, EventArgs e)
{
    // preparing task list
    txtOutput.Text += "Total stuff to run" + listView1.Items.Count + Environment.NewLine + Environment.NewLine;

    // check for valid input
    int value;
    if (!int.TryParse(textBox1.Text, out value) || textBox1.Text == "0"){
        MessageBox.Show("invalid number of threads");
        return;
    }

    // create threads
    for (int i = 0; i < value; i++)
    {
        String woker_id = "bg_" + i.ToString();

        worker_list[woker_id] = new BackgroundWorker();
        worker_list[woker_id].WorkerSupportsCancellation = true;
        worker_list[woker_id].WorkerReportsProgress = true;

        // Intiial message
        txtOutput.Text += "Worker " + woker_id + " Started" + Environment.NewLine;

        worker_list[woker_id].DoWork += delegate {
            worker(woker_id);
        };

        worker_list[woker_id].ProgressChanged += delegate {
            if(completed_count == total_work) {
                return;
            }else
            {
                txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;
            }

            listView1.Items[completed_count].BackColor = Color.LightGreen;
            completed_count++;
        };

        worker_list[woker_id].RunWorkerCompleted += delegate {
            worker_list[woker_id].CancelAsync();
            worker_list.Remove(woker_id);
            txtOutput.Text += "Worker " + woker_id + " Completed | Thread count = " + worker_list.Count() + Environment.NewLine;
        };

        worker_list[woker_id].RunWorkerAsync();
    }

    // Line spacing
    txtOutput.Text += Environment.NewLine + Environment.NewLine;
}

private void Form1_Load(object sender, EventArgs e)
{
    for (int i = 1; i < 11; i++)
    {
        Random rnd = new Random();
        ListViewItem item = new ListViewItem("Item " + i);
        // Sleep Time
        item.SubItems.Add((rnd.Next(100,500) * i).ToString());
        listView1.Items.Add(item);
    }

    // setting up the total work
    total_work = listView1.Items.Count;
}

Thank you.

Answer 1

We actually don't use the BackgroundWorker anymore. The same thing can be done much easier with the newer Task Parallel Library (TPL).

---

Briefly you need to (ok, you should) put the download logic into a new class that could have a Download method. It needs to be marked with async and return a Task to be awaitable. But you need a result so you can use it with a generic parameter Task<byte[]>. (Sidenote: remember to dispose the WebClient). For the new download to work you need to use the awaitable upload API.

If you want to be able to report progress or just be notified when a task is finished you can use the IProgress<T> interface as one of the parameters. You call it inside the method to report its progress. Here just an arbitrary task-id.

class Downloader
{
    public async Task<byte[]> Download(int taskId, NameValueCollection data, IProgress<int> progress)
    {
        using (var client = new WebClient())
        {
            var response = await client.UploadValuesTaskAsync("http://google.com", data);
            progress.Report(taskId);
            return response;
        }
    }
}

Your form code could then be reduced to a simple event handler that is also marked with async. It prepares a collection of download-tasks and then awaits them all or processes each one when it's finished (your choice).

When a task is finished, the form receives a progress report (that can be seen here as a notification) via the ProgressChanged event of an instance of the Progress<T> class.

class MyForm : Form
{
    private readonly Downloader _downloader = new Downloader();

    private async void button2_Click(object sender, EventArgs e)
    {
        // Initialize progress handling.
        var progress = new Progress<int>();
        progress.ProgressChanged += (downloader, taskId) => 
        {
            // update label...
        };

        var downloadCount = 2; //... get the count
        var downloadTasks = 
            Enumerable
            .Range(0, downloadCount)
            // Create task specific arguments.
            .Select((x, i) => _downloader.Download(i, new NameValueCollection(), progress));

        var results = await Task.WhenAll(downloadTasks);
    }
}

---

You may also use a different pattern if you want to process each task result right away by doing it as soon as any task is finished with WhenAny and a loop:

for (int i = 0; i < downloadCount; i++)
{
    var firstTask = await Task.WhenAny(downloadTasks);
    var result = await firstTask;
    // process task result...
}

Answer 2

Object intializer

You can use object intializer to shorten your BackgroundWorker instantiation :

worker_list[woker_id] = new BackgroundWorker();
worker_list[woker_id].WorkerSupportsCancellation = true;
worker_list[woker_id].WorkerReportsProgress = true;

worker_list[woker_id] = new BackgroundWorker
{
    WorkerSupportsCancellation = true,
    WorkerReportsProgress = true
};

Lambda expression vs anonymous method

You can use lambda expression here :

worker_list[woker_id].DoWork += delegate
{
    worker(woker_id);
};

worker_list[woker_id].ProgressChanged += delegate
{
    if (completed_count == total_work)
    {
        return;
    }
    else
    {
        txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;
    }

    listView1.Items[completed_count].BackColor = Color.LightGreen;
    completed_count++;
};

worker_list[woker_id].RunWorkerCompleted += delegate
{
    worker_list[woker_id].CancelAsync();
    worker_list.Remove(woker_id);
    txtOutput.Text += "Worker " + woker_id + " Completed | Thread count = " + worker_list.Count() +
                      Environment.NewLine;
};

It's specially useful in your first subscription to the event because when you are using an anonymous method you need to define a body of the method while lambda expression allows 1 liners :

worker_list[woker_id].DoWork += (o, args) => worker(woker_id);

worker_list[woker_id].ProgressChanged += (o, args) =>
{
    if (completed_count == total_work)
    {
        return;
    }
    else
    {
        txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;
    }

    listView1.Items[completed_count].BackColor = Color.LightGreen;
    completed_count++;
};

worker_list[woker_id].RunWorkerCompleted += (o, args) =>
{
    worker_list[woker_id].CancelAsync();
    worker_list.Remove(woker_id);
    txtOutput.Text += "Worker " + woker_id + " Completed | Thread count = " + worker_list.Count() +
                      Environment.NewLine;
};

Execution branch outside the current block

You have redundant else here

if (completed_count == total_work)
{
    return;
}
else <---
{
    txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;
}

If the else condition evaluates to true the function will terminate if it doesn't it wont so you already know that if your method continues that the if statement evaluated to false :

if (completed_count == total_work)
{
    return;
}
txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;

Issue

Are you sure that you allow negative values as thread count ?

int value;
if (!int.TryParse(textBox1.Text, out value) || textBox1.Text == "0")
{
    MessageBox.Show("invalid number of threads");
    return;
}

I think it should be :

int value;
if (!int.TryParse(textBox1.Text, out value) || value <= 0)
{
    MessageBox.Show("invalid number of threads");
    return;
}

worker_id doesn't sounds like a string to me rather an integer.

String concatenation vs string interpolation

You can make your strings look a little bit prettier if you use interpolated string :

txtOutput.Text += "Worker " + woker_id + " Started" + Environment.NewLine;

txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;

txtOutput.Text += "Worker " + woker_id + " Completed | Thread count = " + worker_list.Count() + Environment.NewLine;

Can become :

txtOutput.Text += $"Worker {woker_id} Started {Environment.NewLine}";

txtOutput.Text += $"Worker {woker_id} is running again {Environment.NewLine}";

txtOutput.Text += $"Worker {woker_id} Completed | Thread count = {worker_list.Count} {Environment.NewLine}";

SRP - Single Responsibility Principle

Your private void button2_Click(object sender, EventArgs e) does too much, you might want to refactor the thread creation in a separate method to make things clearer.

Naming

• Private variables should follow the Camel case typing e.g private int someVariable;

• Public members should follow the Pascal case typing e.g public int SomeVariable {get; set;}, public void Worker.

• button_2, button_1 are bad names even if that's a small experiment project, I still think you should give your variables a meaningful names.