5
\$\begingroup\$

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?

\$\endgroup\$
8
  • \$\begingroup\$ Can you briefly explain why you have doubts? What is your concern? \$\endgroup\$ Apr 19 '16 at 14:01
  • \$\begingroup\$ I am not sure, mayby the Action will increase the lifetime of the using or something like that \$\endgroup\$
    – Chris
    Apr 19 '16 at 14:07
  • 2
    \$\begingroup\$ is this the actual code that you are currently using? \$\endgroup\$
    – Malachi
    Apr 19 '16 at 14:11
  • \$\begingroup\$ Yes, I am actually using it \$\endgroup\$
    – Chris
    Apr 19 '16 at 14:32
  • \$\begingroup\$ 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. \$\endgroup\$
    – Moby Disk
    Apr 19 '16 at 18:31
5
\$\begingroup\$

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())
    ...
\$\endgroup\$
3
  • \$\begingroup\$ Yes, I am missing a using. Good Point thanks. \$\endgroup\$
    – Chris
    Apr 19 '16 at 14:34
  • \$\begingroup\$ It is just a simplified example. I do have a BaseClass (DatabaseBase) to handle the invoke. Here it is protected. \$\endgroup\$
    – Chris
    Apr 19 '16 at 14:36
  • 8
    \$\begingroup\$ @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. \$\endgroup\$ Apr 19 '16 at 14:37
3
\$\begingroup\$
  • 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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$
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);
}
\$\endgroup\$
2
  • \$\begingroup\$ 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 \$\endgroup\$
    – Chris
    Apr 19 '16 at 14:31
  • 1
    \$\begingroup\$ @Chris the reference is indeed the same, and it should work. But by putting the assignments first, you're making that clearer. \$\endgroup\$ Apr 19 '16 at 14:32
2
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Apr 19 '16 at 15:07
  • 1
    \$\begingroup\$ I have stated this in my answer ;-) \$\endgroup\$
    – Heslacher
    Apr 19 '16 at 15:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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