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);
}

Answer 1

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;
}

Answer 2

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):

1. Use ExecuteQuery to pull a DataTable from database.
2. Set DropDownName.DataSource = yourDataTable.
3. Set DropDownName.ValueMember = "id".
4. 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.

Answer 3

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";
            }


        }