4

I don't understand how SqlCommandBuilder does its thing. I have the following code:

public void TestCommandBuilder()
{
    var pubsDataSet = new DataSet("Pubs");
    var pubs = ConfigurationManager.ConnectionStrings["PubsConnectionString"];
    var connection = new SqlConnection(pubs.ConnectionString);
    SqlCommand cmd = connection.CreateCommand();
    cmd.CommandType = CommandType.Text;
    cmd.CommandText = "SELECT * FROM Publishers";
    var da = new SqlDataAdapter(cmd);

    da.Fill(pubsDataSet, "publishers");
    foreach (DataRow row in pubsDataSet.Tables["publishers"].Rows)
    {
        row["pub_name"] = "Updated " + DateTime.Now.Minute + DateTime.Now.Second;
    }

    // The variable builder is not used
    var builder = new SqlCommandBuilder(da);

    da.UpdateBatchSize = 3;
    da.RowUpdated += DaRowUpdated;

    da.Update(pubsDataSet, "publishers");
}

private void DaRowUpdated(object sender, SqlRowUpdatedEventArgs e)
{
    Console.WriteLine("Rows: " + e.RecordsAffected + "\r\n");
}

The variable builder is not used anywhere and I do not call the GetUpdateCommand() method, like they do on MSDN. I only create the SqlCommandBuilder, passing it a SqlDataAdapter. But the code just works fine.

If you look at the code, it seems like the line

var builder = new SqlCommandBuilder(da);

can be safely deleted. In fact, ReSharper is suggesting to remove it. But if I do, the code doesn't run anymore, as the SqlDataAdapter doesn't know how to perform an update.

In debug mode I noticed that after executing that line, the SqlDataAdapter's UpdateCommand property is still null. From the MSDN-docs I get that the SqlCommandBuilder registers itself to the RowUpdated event of the SqlDataAdapter.

But what does it do when that event is triggered? Is the SqlDataBuilder actually performing the update itself?

Something else I noticed, if I remove the SqlCommandBuilder, my DaRowUpdated method is triggered once, just before the InvalidOperationException occurs on the da.Update statement. I did not expect that. I'd think the RowUpdated event only occurs when a row has actually been updated.

So... three concrete questions:

  1. How can I prevent ReSharper from suggesting to delete this line?
  2. Is it bad practice to program a class like SqlCommandBuilder, where the code doesn't indicate in any way that creating an instance is doing something with the SqlDataAdapter passed in?
  3. Is this a design pattern?
comecme
  • 6,086
  • 10
  • 39
  • 67
  • 2
    Uninstall resharper? That did wonders for me. –  Jun 24 '12 at 19:09
  • You have a number of objects which implement `IDisposable`. Wrap these in `using` blocks to make sure the resources get released deterministically. I'll pop an answer below on how it should look. – Jesse C. Slicer Jun 25 '12 at 02:02

3 Answers3

4

If you are not consuming the variable, it is definitely not necessary. If something is going on in its constructor that is useful, you can still invoke that:

var builder = new SqlCommandBuilder(da);

Should be:

new SqlCommandBuilder(da);

This should take care of the warning without changing the behavior of the code. It looks a little weird, and if this is really necessary, I question the design of SqlCommandBuilder a bit. But in practice, a usage like this is essentially the same as an ordinary method invocation.

Kirk Woll
  • 76,112
  • 22
  • 180
  • 195
  • 1
    Maybe I should keep a reference after all, so I can dispose it in the end. – comecme Jun 24 '12 at 20:20
  • @comecme, good catch. Since it does implement `IDisposable`, you should wrap it in a `using` block. – Kirk Woll Jun 24 '12 at 20:46
  • Wrapping it in a `using` statement doesn't make the warning go away, as `builder` still isn't used anywhere else. However, I can combine your answer with using `using` by writing `using (new SqlCommandBuilder(da))`. I didn't know that was possible inside `using` but it is. – comecme Jun 25 '12 at 21:22
2

As per my comment on the question:

    public void TestCommandBuilder()
    {
        var pubs = ConfigurationManager.ConnectionStrings["PubsConnectionString"];

        using (var pubsDataSet = new DataSet("Pubs"))
        using (var connection = new SqlConnection(pubs.ConnectionString))
        using (SqlCommand cmd = connection.CreateCommand())
        {
            cmd.CommandType = CommandType.Text;
            cmd.CommandText = "SELECT * FROM Publishers";

            using (var da = new SqlDataAdapter(cmd))
            using (new SqlCommandBuilder(da))
            {
                da.UpdateBatchSize = 3;
                da.RowUpdated += DaRowUpdated;
                da.Fill(pubsDataSet, "publishers");
                foreach (DataRow row in pubsDataSet.Tables["publishers"].Rows)
                {
                    row["pub_name"] = "Updated " + DateTime.Now.Minute + DateTime.Now.Second;
                }

                da.Update(pubsDataSet, "publishers");
            }
        }
    }

    private void DaRowUpdated(object sender, SqlRowUpdatedEventArgs e)
    {
        Console.WriteLine("Rows: " + e.RecordsAffected);
    }
Jesse C. Slicer
  • 19,901
  • 3
  • 68
  • 87
  • I think you may have to move the build before the updating of the rows since you are hooking into the event DataRowUpdated. – tsells Jun 25 '12 at 02:14
  • Isn't `da.Update(pubsDataSet, "publishers");` doing the updating, which is happening after the event is hooked? – Jesse C. Slicer Jun 25 '12 at 02:20
  • The update to the database is performed after correct. However I think the wiring up of the event (RowUpdated) needs to be done prior to the data changing. If you look at the MSDN example in the link you will see it is wired up immediately after the data adapter is initialized. – tsells Jun 25 '12 at 12:31
  • You can create the `SqlCommandBuilder` immediately before calling `da.Update()` without a problem. Your code still gives me the ReSharper warning on `builder` though. But it seems to be solved by using `using (new SqlCommandBuilder(da)) da.Update(pubsDataSet, "publishers");`. I'm assuming this will still call `Dispose`, but I'm not sure. Can I test that somehow? – comecme Jun 25 '12 at 16:20
  • You can look at the generated IL and see if it's being disposed properly. – Jesse C. Slicer Jun 25 '12 at 22:02
  • 1
    @JesseC.Slicer: I've tried using ILSpy but I don't know what I should look for in order to check if Dispose is being called. However, I've just created my own class implementing `IDisposable` and added a destructor to it. Even if I don't use a variable, I see `Dispose` being called before the destructor is executed. So yes, a `using` statement still calls `Dispose` if you don't assign the instance to a variable. If you remove the `var builder = ` from your answer I'll accept it. – comecme Jun 30 '12 at 09:08
  • Accepted as promised (a bit late though) – comecme Aug 29 '12 at 14:27
0

My understanding that is during the construction of the command builder object it adds a reference to itself on the DataAdapter so it knows how to build CRUD commands.

Note this section under the remarks of the link you posted above.

*

The SqlDataAdapter does not automatically generate the Transact-SQL statements required to reconcile changes made to a DataSet with the associated instance of SQL Server. However, you can create a SqlCommandBuilder object to automatically generate Transact-SQL statements for single-table updates if you set the SelectCommand property of the SqlDataAdapter. Then, any additional Transact-SQL statements that you do not set are generated by the SqlCommandBuilder.

*

tsells
  • 2,751
  • 1
  • 18
  • 20
  • For resharper - you will just have to live with it as you can't exclude items like this at a granular level. However I do agree it should be used inside a using statement. – tsells Jun 25 '12 at 01:21