HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpModerate

Could this ExecuteScalar call be written better?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiswrittencouldbetterexecutescalarcall

Problem

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

re

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#6230, answer score: 16

Revisions (0)

No revisions yet.