6
\$\begingroup\$

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.

\$\endgroup\$
8
\$\begingroup\$

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...
}
\$\endgroup\$
  • \$\begingroup\$ Thank you but how would i update the relevant list view items with the results from the _downloader ? Thank you. \$\endgroup\$ – bro_php Dec 29 '16 at 13:16
  • \$\begingroup\$ @bro_php the TPL will take care of it and process only as many tasks as it can. \$\endgroup\$ – t3chb0t Dec 29 '16 at 13:56
  • \$\begingroup\$ I have around 200 items in list-view, so would it create up-to 200 Tasks at once and start making requests to the web server ?, what would be the elegant way to have the number of tasks running controlled ? Thank you \$\endgroup\$ – bro_php Dec 29 '16 at 13:56
  • \$\begingroup\$ @bro_php We actually don't use the BackgroundWorker anymore. The same thing can be done much easier with the newer Task Parallel Library (TPL)., TPL can do anything that a BackgroundWorker can do in a shorter (most of the time) and simpler syntax, which one to use is up to you, or rather whichever you find easier to write, your example isn't that complex so both of those concepts should do the job, without any major performance losses. \$\endgroup\$ – Denis Dec 29 '16 at 13:57
  • 2
    \$\begingroup\$ Thanks Guys, TPL was a bit scary at first but i'm testing it and it works great! \$\endgroup\$ – bro_php Dec 29 '16 at 14:31
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you. I will amend these changes and i think i got a pretty good idea of where i messed up. Is this approach a good approach ? I mean with background-workers ? \$\endgroup\$ – bro_php Dec 29 '16 at 13:26
  • 1
    \$\begingroup\$ It depends, I might use a normal BackgroundWorker for a single huge task running in the background to avoid blocking the UI, but for many short tasks at once probably TPL is the way to go as @t3chb0t pointed in his answer. In your case Task can make it look prettier and shorter. \$\endgroup\$ – Denis Dec 29 '16 at 13:38
  • 2
    \$\begingroup\$ @bro_php whether you use a background-worker or the TPL the suggestions in denis' answer still apply. \$\endgroup\$ – t3chb0t Dec 29 '16 at 13:44

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.