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