1

This is my background work DoWor function, is the implementation taking into consideration the GUI operation done okay?

       private void backgroundWorker1_DoWork(object sender, System.ComponentModel.DoWorkEventArgs e)
    {
         try
        {

            string connectionString = "Data Source=LPMSW09000012JD\\SQLEXPRESS;Initial Catalog=Pharmacies;Integrated Security=True";
            SqlConnection con = new SqlConnection(connectionString);
            con.Open();
            string query = "SELECT * FROM dbo.Liguanea_Lane2";
            SqlCommand cmd = new SqlCommand(query, con);

            SqlDataReader dr = cmd.ExecuteReader();
            while (dr.Read())
            {
                string scode = dr.GetString(dr.GetOrdinal("code"));

                comboBox2.Invoke((MethodInvoker)delegate
                {
                    comboBox2.Items.Add(scode);
                });
                }
        }
        catch (Exception ex)
        {

            MessageBox.Show(ex.ToString());
        }
    }

And this is the function that calls it:

  private void comboBox4_SelectedIndexChanged(object sender, EventArgs e)
    {
      if(comboBox4.SelectedIndex == 0)
        {
            backgroundWorker1.RunWorkerAsync();

        }
        else
        {

            MessageBox.Show("This table doesn't exist within the database");
        }
    }

Currently nothing happens..the code just runs with the default form on the screen. Not loading the values, what am I doing wrong?

Javy26
  • 375
  • 1
  • 7
  • 22
  • I suppose you will make your code working by calling `backgroundWorker1.RunWorkerAsync();` directly using a button. But your backgroundworker is somehow useless, because you are doing a heavy UI task in `DoWork` event. – Reza Aghaei Nov 08 '16 at 15:46
  • But I thought using method Invoker would help – Javy26 Nov 08 '16 at 15:47
  • No it would not help. When you use `Invoke` the action which you pass to it will run in UI thread. – Reza Aghaei Nov 08 '16 at 15:48
  • All I need to understand is how to split my code in these background worker functions properly. That is the only issue in my code right now because the UI freezes when heavy operations take place. Am just looking at your explanation on my last post again and I don't completely get it. – Javy26 Nov 08 '16 at 15:50
  • 2
    BGW is obsolete. Anything it does can be done just as easily with `async/await` If you want to query the database in the background, you have to use `ExecuteReaderAsync`. All you have to do is define your event handler as `async void` and then write `var reader=await cmd.ExecuteReaderAsync();` – Panagiotis Kanavos Nov 08 '16 at 15:50
  • Also, because you want to add many items to combo box one by one, you should use `BeginUpdate` and `EndUpdate` – Reza Aghaei Nov 08 '16 at 15:53
  • @PanagiotisKanavos where a I putting `var reader = await cmd.ExecuteReaderAsync();`? – Javy26 Nov 08 '16 at 15:54
  • @RezaAghaei if I don't include that does it affect performance? – Javy26 Nov 08 '16 at 15:55
  • Yes, it's simple, just use `comboBox2.BeginUpdate();` before the loop and call `comboBox2.EndUpdate();` after loop. – Reza Aghaei Nov 08 '16 at 15:57
  • @PanagiotisKanavos thanks it works. – Javy26 Nov 08 '16 at 16:00
  • @RezaAghaei same for yours. – Javy26 Nov 08 '16 at 16:00
  • Great! You did it :) – Reza Aghaei Nov 08 '16 at 16:03
  • @RezaAghaei far better to load everything in a list, then call `AddItems` once. It uses `BeginUpdate` internally, The entire operation will also be faster because there is no delay to load from the reader – Panagiotis Kanavos Nov 08 '16 at 16:04
  • @PanagiotisKanavos Using `AddRange` and { `BeginUpdate`, loop (`AddItem`), `EndUpdate`} are equivalent. See [this post](http://stackoverflow.com/a/39338415/3110834). – Reza Aghaei Nov 08 '16 at 16:06
  • @RezaAghaei no they aren't - this loop calls a *reader*, it's not just a loop over AddItem – Panagiotis Kanavos Nov 08 '16 at 16:07
  • @PanagiotisKanavos You are using `While { Read();} AddRange();` in UI thread, it's completely equivalent to `BeginUpdate(); While { Read(); AddItem();} EndUpdate();` In fact the second one is more efficient because of just a single while. `AddRange` uses a loop internally. – Reza Aghaei Nov 08 '16 at 16:14
  • @RezaAghaei no, I'm using Dapper. The first snippet was a direct conversion from the OP's code. The rest of the post explains the improvements. I would also *separate* the data logic from the UI and use data binding to a lookup list. – Panagiotis Kanavos Nov 08 '16 at 16:19
  • @PanagiotisKanavos This technical debate seems have no added value for us :) But the OP should know the main point in using dapper this way is loading data async. Like they have been [previously advised](http://stackoverflow.com/a/40473766/3110834) to use such async approach. – Reza Aghaei Nov 08 '16 at 16:24
  • @RezaAghaei one question, since implementing async, writing to excel sheets is now one cell rather that each cell being written to, which was the case originally. Is there a way to correct this? eg. each comboBox and or textFields wrote to cell A, cell B, etc. – Javy26 Nov 08 '16 at 19:53
  • Sorry I couldn't understand what you mean. – Reza Aghaei Nov 08 '16 at 20:23
  • Okay you have cells in excel. A,B,C. Originally when i wrote data to the file it would match to the cells automatically. eg. comboBox4 is written to Cell A, textField1 is written to Cell B etc. Now since using async it just writes everything to cell A – Javy26 Nov 08 '16 at 20:43
  • Understand now? – Javy26 Nov 08 '16 at 23:37

1 Answers1

3

BGW is obsolete. The TPL, async/awaitand IProgress<T> can address all cases where BGW was used and more.

In this case, BGW isn't appropriate anyway. The real delay is caused by the database call, not loading the UI. To execute the database call asynchronously, you only have to use an asynchronous event handler:

private async void comboBox4_SelectedIndexChanged(object sender, EventArgs e)
{
        var conString=Properties.Settings.Default.MyConnectionString;
        string query = "SELECT * FROM dbo.Liguanea_Lane2";                    

        using(var con = new SqlConnection(conString))
        using(var cmd = new SqlCommand(query, con))
        {
            await con.OpenAsync();
            var reader=await cmd.ExecuteReader();
            while (dr.Read())
            {
                string scode = dr.GetString(dr.GetOrdinal("code"));
                comboBox2.Items.Add(scode);
            }
        }
}

There are better ways to load a combo though. One improvement is to load all items in a list, then use AddRange to update the combo. AddRange already calls BeginUpdate to prevent UI updates while adding items from the list.

        var items=new List<string>();
        using(var con = new SqlConnection(conString))
        using(var cmd = new SqlCommand(query, con))
        {
            await con.OpenAsync();
            var reader=await cmd.ExecuteReader();
            while (dr.Read())
            {
                string scode = dr.GetString(dr.GetOrdinal("code"));
                list.Add(scode);
            }
        }

        comboBox2.Items.AddRange(items);

Even better, use a micro-ORM like Dapper to get rid of all this code:

using(var con = new SqlConnection(conString))
{
    await con.OpenAsync();
    var items=await con.QueryAsync<string>(query);
    comboBox2.Items.AddRange(items);
}
Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • Is there a way to include a progress bar to track the loading process? Am not sure if it will load is quickly for 20k records or maybe just give the user a loading screen to look at – Javy26 Nov 08 '16 at 16:03
  • @Jevon sure, with async/await just put a bar in the loop and update it as needed. You are still on the UI thread so you can do that, just like how you add items to the combo box. The hardest part of doing a bar is efficiently getting a count of the total number of items to process. – Scott Chamberlain Nov 08 '16 at 16:05
  • A combo with 20K records is unusable by humans. You'll have to implement a searchable combo in this case. In any case, you *can* update any UI control you want, as after `await` control returns to the UI thread – Panagiotis Kanavos Nov 08 '16 at 16:06
  • @PanagiotisKanavos the 20k reading from database is necessary. It's a data entry process. – Javy26 Nov 08 '16 at 16:07
  • And humans can't really pick from 20K records. Either you already use auto-complete or you'll have to find a way around this. Eg, comboboxes allow data binding to data sources. You could load all codes in a list, bind the control to it and let it update itself as necessary. – Panagiotis Kanavos Nov 08 '16 at 16:17
  • @Jevon or use a third-party control that allows data virtualization - only the data that can be displayed is loaded. More data is loaded as needed. Some third-party controls offer far better autocomplete features that the Combobox too – Panagiotis Kanavos Nov 08 '16 at 16:18
  • @PanagiotisKanavos It's autocomplete comboBox so based on what the user types it populates the required fields. They don't have to scroll through everything – Javy26 Nov 08 '16 at 17:07