7
\$\begingroup\$

I came across this code in our project today. Where possible I'm trying to leave the code base in a better shape than I found it, as I go along, and this method jumped out at me for a number of reasons, mainly the sql string and the try/catch block. I feel there's a less expensive way to do it.

Original Code:

public bool CheckSomething(string paramA, int paramB)
{
    using (var conn = new SqlConnection(Connection))
    {
        conn.Open();
        string sqlCommand = "SELECT ColumnA FROM OurTable WHERE ColumnB = '" + paramA +
                                "' AND ColumnC = " + paramB;

        using (var dbCommand = new SqlCommand(sqlCommand, conn))
        {
            int noOfRecords = -1;
            try
            {
                noOfRecords = (int)dbCommand.ExecuteScalar();
            }
            catch (Exception ex)
            {
            }
            finally 
            {
                dbCommand.Dispose();
                if (conn.State == ConnectionState.Open)
                {
                    conn.Close();
                }

                return noOfRecords > 0;
            }
        }
    }
}

I was thinking of re-writing it as this, but I still think it could be improved further, one of which would be to create an procedure for the sql, but that's unlikely. Was aiming to improve it purely from the code point of view. I'd appreciate thoughts.

Rewritten version:

public bool CheckSomething(string paramA, int paramB)
{
    using (var conn = new SqlConnection(Connection))
    {
        conn.Open();
        string sqlCommand = string.Format("SELECT ColumnA FROM OurTable WHERE ColumnB = '{0}' and ColumnB = {1}", paramA, paramB);

        using (var dbCommand = new SqlCommand(sqlCommand, conn))
        {
            object noOfRecords = dbCommand.ExecuteScalar();
            dbCommand.Dispose();

            if (conn.State == ConnectionState.Open)
            {
                conn.Close();
            }

            return noOfRecords != null;
        }
    }
}
\$\endgroup\$

migrated from stackoverflow.com Nov 23 '11 at 19:02

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ Why in the world would you change noOfRecords (horrible name) to an object and check for null? How is that the same? \$\endgroup\$ – Kyle W Nov 22 '11 at 16:34
  • \$\begingroup\$ Yeah, I noticed that after I posted. The variable name there wasn't changed, that's an oversight. what that check for null is supposed to do, and I may be wrong, is to simply check if a result was returned. The method essentially boils down to, if a result is returned, then true, else false. \$\endgroup\$ – Andrew Johns Nov 22 '11 at 16:38
  • \$\begingroup\$ Which is fundamentally different than what was there. Are you trying to... save the cost of a cast or something?... \$\endgroup\$ – Kyle W Nov 22 '11 at 16:40
  • \$\begingroup\$ I'm simply trying to rewrite the method to return the result it needs as efficiently as possible. The original attempts to get a result, then cast it an int, and then return a comparison on whether that int value is more than 0. The reality is that if a result is returned from that query then the method should return true. To me that is much more simpler to understand, and I'm assuming more efficient, even by a very small margin. \$\endgroup\$ – Andrew Johns Nov 22 '11 at 16:48
  • \$\begingroup\$ Is the intent to check "ColumnA" to see if it has a value > 0, or did the original developer have the misguided idea that ExecuteScalar returns the number of rows? If the first, you're changing the functionality, if the second, you should fix it. If you don't know... you shouldn't change it. That being said, the amount of time you're "saving" is so miniscule compared to the cost of the DB call that it simply doesn't matter. If you think it's easier to understand (and the functionality isn't changing), then by all means go ahead. But don't do it for "speed". \$\endgroup\$ – Kyle W Nov 22 '11 at 17:53
16
\$\begingroup\$
public bool CheckSomething(string paramA, int paramB)
{    
    using (var conn = new SqlConnection(".."))
    using (var comm = new SqlCommand("", conn))
    {
        conn.Open();
        object noOfRecords = comm.ExecuteScalar();
        return noOfRecords != null;
    }
}

There is no need to close or dispose, the using handles that part. This removes the need for a manual try catch or closing logic, leaving a much compressed chunk of code that is functionally equivalent and just as safe.

As for the select statement itself, either use parameterized SQL or a stored procedure as opposed to string concatenation. Parameterized SQL:

string sql = "SELECT ColumnA FROM OurTable WHERE ColumnB = @param1 AND ColumnC = @param2";

using (var comm = new SqlCommand(sql, conn))
{
    comm.Parameters.AddWithValue("@param1", param1);
    comm.Parameters.AddWithValue("@param2", param2);

    conn.Open();
    // etc...
}
\$\endgroup\$
4
\$\begingroup\$

As Adam said, there is no real need to call the Dispose methods because the using clauses are there for that.

However, what I'd really want to change about your code is how the SQL query is built. You should never concatenate strings to create your SQL queries, unless you're willing to invite SQL injections into your code.

You should check out how to use Parameters instead.

\$\endgroup\$
1
\$\begingroup\$

This is terrible:

       catch (Exception ex)
        {
        }

It will hide problems. It's like putting black tape over the idiot lights on your car's dashboard.

\$\endgroup\$
  • \$\begingroup\$ I agree, though siding with the author of the original example, he was probably just using a common (but derided) way of handling casting to an int. If the cast fails, assume a default value. The alternative way to write the original would to make this logic clearer have been to declare int noOfRecords, and then specifically put noOfRecords = -1 in the catch. It still masks a range of issues as to why it caused an exception, but my assumption is that original writer didn't care about why it failed, just to assume that the method will return false in any case where a result isn't returned. \$\endgroup\$ – Andrew Johns Nov 22 '11 at 16:42
0
\$\begingroup\$

I agree that your rewritten version looks much cleaner. My only change would be to make use of SQL parameters.

    string sqlCommand = "SELECT ColumnA FROM OurTable WHERE ColumnB = @paramA and ColumnC = @paramB";

    using (var dbCommand = new SqlCommand(sqlCommand, conn))
    {
        dbCommand.Parameters.Clear();
        dbCommand.Parameters.AddWithValue("paramA",paramA);
        dbCommand.Parameters.AddWithValue("paramB",paramB);
        object noOfRecords = dbCommand.ExecuteScalar();
        dbCommand.Dispose();

SQL parameters are good to use any time you're dealing with user input.

\$\endgroup\$
  • \$\begingroup\$ Yes, good approach here, which was an improvement on mine. Thankfully I don't think we have too many examples in our code base of Sql commands written like the example I provided, but if I do come across any more, I will probably replace the existing code with parameters as you suggest. \$\endgroup\$ – Andrew Johns Nov 22 '11 at 16:44
0
\$\begingroup\$

Addionally avoid using AddWithValue.

An implicit conversion for the parameter value for the sql is done, it can result in bad performance.

If parameter of the command are different SqlServer is not able to cache the execution plan and has to compile the statement again and again. You can see it if you trace/watch that in the sqlprofiler.

Use the "long" version instead and specify the datatype and length of the paramter.

 var parameter = new SqlParameter("@p1", SqlDbType.VarChar, 5);
 parameter.Value = "value";

I recommend to read the accepted answer from AddWithValue difficulty from stack.

Remus Rusanu explained it a bit deeper.

\$\endgroup\$
0
\$\begingroup\$

Here's how I'd code it up. It more or less resembles Adam Houldsworth's answer, but does actually check the int value that the ExecuteScalar() is expected to return from the O.P.

public bool CheckSomething(string paramA, int paramB)
{
    using (var conn = new SqlConnection(Connection))
    using (var command = new SqlCommand("SELECT ColumnA FROM OurTable WHERE ColumnB = @paramA AND ColumnC = @paramB", conn))
    {
        command.Parameters.Add(new SqlParameter("@paramA", SqlDbType.VarChar)).Value = paramA;
        command.Parameters.Add(new SqlParameter("@paramB", SqlDbType.Int)).Value = paramB;
        conn.Open();
        return (int)command.ExecuteScalar() > 0;
    }
}
\$\endgroup\$

Your Answer

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