Best Practices for SQL Statements/Connections in Get() Request

Question

For simple lookups, I need to perform some SQL statements on a DB2 machine. I'm not able to use an ORM at the moment. I have a working example through this code, however I'm wondering if it can be optimized more as this would essentially create a connection on each request. And that just seems like bad programming.

Is there a way I can optimize this Get() request to leave a connection open? Nesting using statements seems dirty, as well. How should I handle the fact that Get() really wants to return an object of User no matter what; even in error? Can I put this connection in the start of the program so that I can use it over and over again? What are some of the best practices for this?

public class UsersController : ApiController
{
    String constr = WebConfigurationManager.ConnectionStrings["DB2Connection"].ConnectionString;

    public User Get([FromUri] User cst)
    {
        if (cst == null)
        {
            throw new HttpResponseException(HttpStatusCode.NotFound);
        }
        else
        {
            using (OdbcConnection DB2Conn = new OdbcConnection(constr))
            {
                DB2Conn.Open();
                using (OdbcCommand com = new OdbcCommand(
                    // Generic SQL Statement
                    "SELECT * FROM [TABLE] WHERE customerNumber = ?", DB2Conn))
                {
                    com.Parameters.AddWithValue("@var", cst.customerNumber);

                    using (OdbcDataReader reader = com.ExecuteReader())
                    {
                        try
                        {
                            while (reader.Read())
                            {
                                cst.name = (string)reader["name"];

                                return cst;
                            }
                        }
                        catch
                        {
                            throw;
                        }
                    }
                }
            }
            return cst;
        }
    }
}

I found a great question that doesn't really have detailed answers, I feel like similar solutions exist for both of these questions...

Answer 1

And that just seems like bad programming.

Why do you think that?

The underlying system should be maintaining connections in a connection pool for you. Creating a connection should be very optimized already.

From a logical perspective, what you're doing now is exactly what you want to be doing. Create the connection, use it, and dispose of it immediately. This allows other threads/processes/etc. to use it from the connection pool now that you're done with it.

This also avoids the myriad of problems which arise from manually maintaining your open connections outside of the code that uses them.

Is there a way I can optimize this Get() request to leave a connection open?

Have you measured an actual performance problem? If not, there's nothing to optimize.

And there's a very good chance that hanging on to open connections in a static context in your web application is going to have drastic performance implications.

---

In short... You're already doing this correctly. (Well, except for that unnecessary try/catch. You can remove that.)

---

Edit: If you're just looking to improve the readability of the code (which itself is a matter of personal preference), this seems readable to me:

public User Get([FromUri] User cst)
{
    if (cst == null)
        throw new HttpResponseException(HttpStatusCode.NotFound);

    using (var DB2Conn = new OdbcConnection(constr))
    using (var com = new OdbcCommand("SELECT * FROM [TABLE] WHERE customerNumber = ?", DB2Conn))
    {
        com.Parameters.AddWithValue("@var", cst.customerNumber);
        DB2Conn.Open();
        using (OdbcDataReader reader = com.ExecuteReader())
            while (reader.Read())
            {
                cst.name = (string)reader["name"]
                return cst;
            }
    }
    return cst;
}

Note that you can further improve it by re-addressing the logic of that SQL query. Since you're fetching one value from one record then you don't need to loop over a data reader. Just fetch a single literal and return it. Note that this is free-hand and untested, but it might look something like this:

public User Get([FromUri] User cst)
{
    if (cst == null)
        throw new HttpResponseException(HttpStatusCode.NotFound);

    using (var DB2Conn = new OdbcConnection(constr))
    using (var com = new OdbcCommand("SELECT name FROM [TABLE] WHERE customerNumber = ? FETCH FIRST 1 ROWS ONLY", DB2Conn))
    {
        com.Parameters.AddWithValue("@var", cst.customerNumber);
        DB2Conn.Open();
        cst.name = (string)com.ExecuteScalar();
    }
    return cst;
}

Answer 2

@David's answer addresses your actual questions perfectly but here's some other observations that may make your code a little more pallatable to you:

• remove the try/catch block - all you're doing is re-throwing the exception which is what will happen if you don't use a try/catch at all. Don't catch the exception unless you can do something about it. (I see now that @David's answer addresses that - either it was added after I read it or I missed it - my apologies for the overlap but it's worth reinforcing)
• Change your query to just pull name and use ExecuteScalar instead of ExecuteReader. You are taking the name value from the first record and exiting the while loop. ExecuteScalar returns the value from the first column in the first record, so you can eliminate the while loop and the using there.