Explore our sites

Stack Exchange
tour help
Take the tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite

I use this code to Load and Insert data to a table using a DataGridView in a C# windows application.

        SqlCommand sCommand;
        SqlDataAdapter sAdapter;
        SqlCommandBuilder sBuilder;
        DataSet sDs;
        DataTable sTable;          
private void form1_Load(object sender, EventArgs e)
            {    
                string connectionString = "Data Source=.\\SQLEXPRESS;AttachDbFilename=|DataDirectory|\\Database1.mdf;Integrated Security=True;User Instance=True";
                string sql = "SELECT * FROM mytable";
                SqlConnection connection = new SqlConnection(connectionString);
                connection.Open();
                sCommand = new SqlCommand(sql, connection);
                sAdapter = new SqlDataAdapter(sCommand);
                sBuilder = new SqlCommandBuilder(sAdapter);
                sDs = new DataSet();
                sAdapter.Fill(sDs, "mytable");
                sTable = sDs.Tables["mytable"];
                connection.Close();
                dataGridView1.DataSource = sDs.Tables["mytable"];
                dataGridView1.ReadOnly = true;
                save_btn.Enabled = false;
                dataGridView1.SelectionMode = DataGridViewSelectionMode.FullRowSelect;
            }

            private void new_btn_Click(object sender, EventArgs e)
            {
                dataGridView1.ReadOnly = false;
                save_btn.Enabled = true;
                new_btn.Enabled = false;
                delete_btn.Enabled = false;
            }

            private void delete_btn_Click(object sender, EventArgs e)
            {
                if (MessageBox.Show("Are you sure?", "Delete", MessageBoxButtons.YesNo) == DialogResult.Yes)
                {
                    dataGridView1.Rows.RemoveAt(dataGridView1.SelectedRows[0].Index);
                    sAdapter.Update(sTable);
                }
            }

            private void save_btn_Click(object sender, EventArgs e)
            {
                sAdapter.Update(sTable);
                dataGridView1.ReadOnly = true;
                save_btn.Enabled = false;
                new_btn.Enabled = true;
                delete_btn.Enabled = true;
            }
        }

It's ok and works, but when I try to work with a query that has a condition then no rows are added to the DataGrid and MyTable anymore

sql = "SELECT * FROM mytable where col2 = 1";
share|improve this question
add comment

closed as off topic by svick, Jeff Vanzella, seand, Brian Reichle, mnhg Apr 11 at 8:13

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

up vote 2 down vote

It looks like your code should work and output rows if your database contains records corresponding to your condition.

Since we are on a code review site I would like to highlight several code issues not related to your question:

  • You should dispose all objects implementing IDisposable interface. In your code these objects are SqlConnection, SqlCommand and SqlDataAdapter. Easiest way to dispose objects properly is via using statement (note that you don't need to close connection if you dispose it).
  • You don't need to use DataSet to populate a DataTable, you can populate DataTable directly from DataAdapter.
  • Initialization of controls (dataGridView1.ReadOnly, dataGridView1.SelectionMode and save_btn.Enabled in form1_Load) can be done at design time, so that your code is not cluttered with it.

As a result of described transformations your code may look like this:

private void form1_Load(object sender, EventArgs e)
{
    string connectionString = "Data Source=.\\SQLEXPRESS;AttachDbFilename=|DataDirectory|\\Database1.mdf;Integrated Security=True;User Instance=True";
    string sql = "SELECT * FROM mytable";

    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(sql, connection))
    using (var adapter = new SqlDataAdapter(command))
    {
        connection.Open();
        var myTable = new DataTable();
        adapter.Fill(myTable);
        dataGridView1.DataSource = myTable;
    }
}

This code is good enough for simple project, but it won't allow you to unit-test your project. If you start looking in this direction prepare to spend quite some time learning ORM frameworks and Inversion of Control.

share|improve this answer
add comment
up vote 0 down vote

You have multiple problems in your code, to name a few:

  • you don't close the connection nor dispose any of the problematic objects (SqlConnection, SqlCommand and SqlDataAdapter). Yes, I see you're using them later in the code, but trust me, it's a BAD idea
  • as almaz already pointed out, you don't need DataSet in this situation at all

What I'm troubled with the most is of a different matter:
You don't separate layers at all! You mix and match UI logic, business logic and data access into one big ugly mess. This looks like a procedural code, completely ignoring every OOP rule. How did you come to the conclusion that your Form should be aware (and responsible!!!) for objects like SqlConnection?

What you should do in the first place is to separate your logic into layers. For instance this is a good place to start reading and learning the theory.

If you fix this, I believe you won't have problems like this anymore, because you would be able to find the problem more easily without any external help.

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.