C# Populate a ComboBox from table in database

Question

I have a combobox which is called combobox1, which I want to populate it with id as value and Name as display name. I searched and read some tutorial and found this code to use in Form load event, but it doesn't populate the list. I see an empty dropdown. Any ideas of where I am wrong?

In my database class I have this function.

public static void FillDropDownList(string Query, System.Windows.Forms.ComboBox DropDownName)
{
   SqlDataReader dr;

   SqlConnection myConnection = new SqlConnection(CONNECTION_STRING);
   try
   {
      myConnection.Open();
   }
   catch (Exception e)
   {
      Console.WriteLine(e.ToString());
   }

   // Check whether the Drop Down has existing items. If YES, empty it.
   if (DropDownName.Items.Count > 0)
      DropDownName.Items.Clear();

   SqlCommand cmd = new SqlCommand(Query, myConnection);
   dr = cmd.ExecuteReader();

   while (dr.Read())
      DropDownName.Items.Add(dr[0].ToString());

   Console.Write(DropDownName.Items.Add(dr[0].ToString()));
   dr.Close();
}

In my form i call it as

private void sales_record_Load(object sender, EventArgs e)
{
    SqlConnection con = new SqlConnection(DBUtils.CONNECTION_STRING);
    DBUtils.FillDropDownList("select id,Name from Farms", comboBox1);
}

Is the result set from the DB non-empty? – XORcist Jan 01 '13 at 20:40 — XORcist, Jan 01 '13 at 20:40

XIVSolutions · Answer 1 · 2013-01-02T00:02:32.780

This should do what you are seeking. The issue I see, from a design standpoint, is that anyone coding a query for this will need to be aware that the first two columns returned need to reflect the ID and the display item, respectively. Other than that, the actual column names don;t matter, because the ValueMember and DisplayMember properties (the string name of each respective column in the DataSource) are obtained by ordinal reference to Column[0] and Column[1].

Note that I have wrapped each DataAccess object (the SQLConnection instance and the SQLCommand Instance in using blocks. This is a recommended practice for data access objects, which tend to consume non-managed resources and need to be disposed. The using block handles disposal of each object for you. Note that each using block contains its own scope.

Hope that helps!

UPDATE: @Neolisk posted his answer while I was composing mine. While they are not duplicates, they cover a lot of the same ground. Between his answer and this, you should have what you need!

public void FillDropDownList(string Query, ComboBox DropDownName)
{
    // If you use a DataTable (or any object which implmenets IEnumerable)
    // you can bind the results of your query directly as the 
    // datasource for the ComboBox. 
    DataTable dt = new DataTable();

    // Where possible, use the using block for data access. The 
    // using block handles disposal of resources and connection 
    // cleanup for you:
    using (var cn = new SqlConnection(CONNECTION_STRING))
    {
        using(var cmd = new SqlCommand(Query, cn))
        {
            cn.Open();

            try
            {
                dt.Load(cmd.ExecuteReader());
            }
            catch (SqlException e)
            {
                // Do some logging or something. 
                MessageBox.Show("There was an error accessing your data. DETAIL: " + e.ToString());
            }
        }
    }

    DropDownName.DataSource = dt;
    DropDownName.ValueMember = dt.Columns[0].ColumnName;
    DropDownName.DisplayMember = dt.Columns[1].ColumnName;
}

+1. I forgot you can do `DataTable.Load` and actually keep `DataReader` object. Interesting concept with ordinal references. I think your `cn.Close()` should be `dr.Close()` or perhaps removed altogether, because when connection is closed, it should close related readers (but don't quote me on this). — Victor Zakharov, Jan 01 '13 at 23:23
Actually, I recall reading somewhere that the using block also takes care of closing open connections too (don't quote ME on that!) but I threw cn.close into the finally block just in case. I SHOULD have closed the DataReader, but it goes out of scope within the SQLCommand using block, so I don't think that's a problem . . . — XIVSolutions, Jan 01 '13 at 23:48
Yes, it [does close a connection automatically](http://stackoverflow.com/questions/376068/does-end-using-close-an-open-sql-connection). — Victor Zakharov, Jan 01 '13 at 23:49
Oooh. I had sudden (and rather pedantic, at this point!) recollection . . . instead of using `dr = cmd.ExecuteReader()`, you can be a tad bit more concise, and use `dt.Load(cmd.ExceuteReader())`. See edit in my post . . . — XIVSolutions, Jan 02 '13 at 00:01
Thanks a lot, +1 for helping. I used a similar approach to achieve my objective. — Amna Ahmed, Jan 02 '13 at 07:36

score 8 · Accepted Answer · edited May 23 '17 at 12:31

My advice - use .NET built-in functionality as much as possible, and don't handle data binding manually (which is what you are trying to do in your code):

Use ExecuteQuery to pull a DataTable from database.
Set DropDownName.DataSource = yourDataTable.
Set DropDownName.ValueMember = "id".
Set DropDownName.DisplayMember = "Name".

So your code would look similar to this:

public static void FillDropDownList(string Query, System.Windows.Forms.ComboBox DropDownName)
{
  DataTable dt;

  using (var cn = new SqlConnection(CONNECTION_STRING))
  {
    cn.Open();

    try
    {
      SqlCommand cmd = new SqlCommand(Query, cn);
      dt = cmd.ExecuteQuery();
    }
    catch (SqlException e)
    {
      Console.WriteLine(e.ToString());
      return;
    }
  }

  DropDownName.DataSource = dt;
  DropDownName.ValueMember = "id";
  DropDownName.DisplayMember = "Name";
}

Notice how I changed exception type to SqlException, so we are only looking for database errors. Everything else will blow up. I don't remember any situation when myConnection.Open(); would throw an exception, so your try block is not very useful. Notice in my try clause - it has ExecuteQuery inside it, which is likely to fail.

EDIT: There is no need to close connection in the finally block when using the using construct. So it can be removed - and your code becomes more compact as a result.

Nice answer - You must have posted while I was writing mine! Nice job explaining your code +1 — XIVSolutions, Jan 01 '13 at 23:02
I would recommend, though, that the OP examine the using block, and the use of ordinal column reference (in this unique case) instead of aliasing the id and name column in any SQL statement passed in. — XIVSolutions, Jan 01 '13 at 23:06

score 3 · Answer 3 · answered Jan 02 '13 at 07:38

I was able to make it work using @Neolisk 's code. Only made some minor changes to code which are as follows.

public static void FillDropDownList(string Query, System.Windows.Forms.ComboBox DropDownName)
        {
            using (var cn = new SqlConnection(CONNECTION_STRING))
            {
                cn.Open();
                DataTable dt = new DataTable();
                try
                {
                    SqlCommand cmd = new SqlCommand(Query, cn);
                    SqlDataReader myReader = cmd.ExecuteReader();
                    dt.Load(myReader); 
                }
                catch (SqlException e)
                {
                    Console.WriteLine(e.ToString());
                    return;
                }
                DropDownName.DataSource = dt;
                DropDownName.ValueMember = "id";
                DropDownName.DisplayMember = "Name";
            }


        }

C# Populate a ComboBox from table in database

3 Answers3

Linked