Handle IDisposable objects

Question

When I work a lot with objects, that implements IDisposable I am using this pattern, like shown here with a SqlConnection and a SqlCommand.

 private string _connectionString = "";
 public IEnumerable<User> GetUsers()
 {
 List<User> users = new List<User>();
 string sql = "SELECT * FROM [user]";
 InvokeSql(sql, (connection, command) =>
 {
 IDataReader reader = command.ExecuteReader();
 while (reader.Read())
 {
 User user = new User();
 users.Add(user);
 user.Name = reader["name"] as string;
 }
 });
 return users;
 }
 private void InvokeSql(string sql, Action<SqlConnection, SqlCommand> action)
 {
 if (string.IsNullOrWhiteSpace(sql))
 {
 throw new ArgumentNullException("sql");
 }
 if (action == null)
 {
 throw new ArgumentNullException("action");
 }
 using (SqlConnection connection = new SqlConnection(_connectionString))
 using (SqlCommand command = new SqlCommand(sql, connection))
 {
 connection.Open();
 action(connection, command);
 }
 }

So I can reuse the InvokeSql and do not have to bother about the disposing. But is it really a good idea, to use an Action in a using Statement?

• Olivier Jacot-Descombes: Can you briefly explain why you have doubts? What is your concern?
• Chris: I am not sure, mayby the Action will increase the lifetime of the using or something like that
• Malachi: is this the actual code that you are currently using?
• Chris: Yes, I am actually using it
• Moby Disk: If the name column is always a string, I recommend user.Name = (string)reader["name"] instead of using "as." The purpose of as is to coerce the value to null if the type is wrong. In your case, you probably want to throw if the column type changes, rather than get nulls back.

Answer 1

So I can reuse the InvokeSql and do not have to bother about the disposing.

Sure. Except InvokeSql is private, so reuse is rather limited to that class. And if that class has a GetUsers method, I dare expect only User-related stuff in that class... which InvokeSql isn't.

In all likelihood, your model is going to involve more than just User entities; I'd make a base class to encapsulate the CRUD and this InvokeSql method; then a derived type can be responsible for User entities, and another derived type can be responsible for another entity type - something like a "repository".

ExecuteReader returns a SqlDataReader instance, which also implements IDisposable: you're missing a using block here:

IDataReader reader = command.ExecuteReader();
while (reader.Read())

using (SqlDataReader reader = command.ExecuteReader())
{
 while (reader.Read())
 ...

• Chris: It is just a simplified example. I do have a BaseClass (DatabaseBase) to handle the invoke. Here it is protected.
• Mathieu Guindon: @Chris And this is why we strongly encourage users to post their real, actual code when putting something up for peer review on this site.

Answer 2

• There is no need to have a Action<SqlConnection, SqlCommand> parameter if all you need is a SqlCommand object.

• Selecting all columns for just using the Name column of the returned reader is slowing down the whole stuff. You should always only select the columns you need.

• You are using C# 6 so you should take advantage of the nameof expression like

  if (string.IsNullOrWhiteSpace(sql))
  {
   throw new ArgumentNullException(nameof(sql));
  } 
  

  but this thrown exception isn't the correct one, because if sql contains only white space it should instead throw an ArgumentException.

Answer 3

You're making life harder for yourself by not allowing the delegate to have a return type. You could create an overload that accepted a Func instead.

private T InvokeSql<T>(string sql, Func<SqlCommand, T> f)
{
 // same as before but
 return f(command);
}

Now you can simplify:

public IEnumerable<User> GetUsers()
{
 string sql = "SELECT * FROM [user]";
 return InvokeSql(sql, (command) =>
 {
 var users = new List<User>();
 using (var reader = command.ExecuteReader())
 {
 while (reader.Read())
 {
 users.Add(new User
 {
 Name = reader["name"] as string
 });
 }
 return users;
 });
}

You no longer need to close over the list because you can return one directly from the delegate.

---

Having said that, I don't like the API because it's not immediately obvious how to parametrise the SQL. Plus, any parameter values will have to be closed over...

Generally I don't like APIs that make it easy to do the wrong thing - and I think this is a prime example of that.

Answer 4

while (reader.Read())
{
 User user = new User();
 users.Add(user);
 user.Name = reader["name"] as string;
}

this doesn't look like good code to me, I would assign the name property before adding the object to the list. This is much clearer and follows a logical flow of information when reading the code.

1. Create the new user
2. Assign properties
3. Add to list

so your code should look like this instead

while (reader.Read())
{
 User user = new User();
 user.Name = reader["name"] as string;
 users.Add(user);
}

you could also use object initialization and write the same code like this

while (reader.Read())
{
 User user = new User { Name = reader["name"] };
 users.Add(user);
}

• Chris: But the reference is still the same, so I do not see a Problem here. I am doing it that way, because I often forget to add the user at the end
• Mathieu Guindon: @Chris the reference is indeed the same, and it should work. But by putting the assignments first, you're making that clearer.

Answer 5

Calling the action is like calling a method. It is executed synchronously. Therefore the using-statement will dispose and end only after the action has done its job. The using-statement does not keep references any resources after it has terminated.

The action, however, captures the users list. If the action was stored somewhere (e.g. in a static field) then this would increase the lifetime of the list.

I don't see any problem with your procedure regarding your concern about "increase the lifetime of the using or something like that". It is a clever way of simplifying db-calls.

---

You could also write an overloaded version accepting a reader instead of a connection and a command. This would allow you to automatically include the reader in a using-statement as well

private void InvokeSql(string sql, Action<SqlDataReader> action)
{
 if (string.IsNullOrWhiteSpace(sql))
 {
 throw new ArgumentNullException("sql");
 }
 if (action == null)
 {
 throw new ArgumentNullException("action");
 }
 using (SqlConnection connection = new SqlConnection(_connectionString))
 using (SqlCommand command = new SqlCommand(sql, connection))
 {
 connection.Open();
 using (SqlDataReader reader = command.ExecuteReader()) {
 action(reader);
 }
 }
}

• Olivier Jacot-Descombes: It will not make things worse, but you are right, the connection and the command are not needed in the action any more. Btw, the connection is not really needed in the original version as well.
• Heslacher: I have stated this in my answer ;-)