Could this ExecuteScalar call be written better?

Question

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;
        }
    }
}

Answer 1

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...
}

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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;
    }
}