Может ли это executescalar так звонок быть написана лучше?


Я наткнулся на этот код в наш проект сегодня. Где возможно, я пытаюсь покинуть базу кода в лучшей форме, чем я нашел его, как я иду вперед, и этот метод выпрыгнул на меня из-за ряда причин, в основном SQL-строку и try/catch блок. Мне кажется, что есть менее затратные способы сделать это.

Исходный Код:

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

Я думал о перезаписи его как этот, но я все равно думаю, что это может быть улучшено, одним из которых станет создание процедуры для SQL, но это маловероятно. Стремился он чисто улучшить с точки зрения кода. Я ценю мысли.

Переписанная версия:

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


14039
7
задан 22 ноября 2011 в 04:11 Источник Поделиться
Комментарии
6 ответов

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

Нет необходимости закрывать или удалять, с помощью ручки этой части. Это устраняет необходимость вручную попробовать поймать или закрытия логики, оставляя за собой сильно сжатый кусок кода, который функционально эквивалентен и так же безопасен.

Что же касается самой инструкции Select, либо использовать параметризованные SQL или хранимую процедуру, в отличие от конкатенации строк. Параметризованные 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...
}

16
ответ дан 22 ноября 2011 в 04:11 Источник Поделиться

Как сказал Адам, нет никакой реальной необходимости для вызова распоряжаться методов, потому что , используя положения существуют для этого.

Однако, чего я действительно хочу изменить ваш код, как SQL-запрос строится. Вы должны никогда не конкатенации строк, чтобы создать свой SQL-запросов, если вы не готовы пригласить к SQL-инъекции в код.

Вы должны проверить, как использовать параметры вместо.

4
ответ дан 22 ноября 2011 в 04:11 Источник Поделиться

Это ужасно:

       catch (Exception ex)
{
}

Он скроет проблемы. Это как поставить черную ленту на идиота огни на приборной панели вашего автомобиля.

1
ответ дан 22 ноября 2011 в 04:11 Источник Поделиться

Я согласен, что ваша переписанная версия выглядит намного чище. Мое единственное изменение будет использовать параметры SQL.

    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 хорошо использовать любое время вы имеете дело с пользовательского ввода.

0
ответ дан 22 ноября 2011 в 04:11 Источник Поделиться

Вот как бы я код его. Это более или менее напоминает Адама Houldsworthс ответом, но реально проверить тип int значение, что executescalar так() , как ожидается, вернуться из О. П.

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

0
ответ дан 23 ноября 2011 в 07:11 Источник Поделиться

Дополнительно избегать использования AddWithValue.

Неявное преобразование значения параметров для SQL-это сделано, это может привести к плохой производительности.

Если Параметр команды отличаются от SQLServer не может кэше плана выполнения и приходится снова и снова составить заявление.
Его можно увидеть, если отследить/посмотреть, что в sqlprofiler.

Использовать вместо "длинной" версии и укажите тип данных и длину параметра.

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

Я рекомендую прочитать принятый ответ от AddWithValue сложности из стека.

Ремус Rusanu объяснил это немного глубже.

0
ответ дан 23 ноября 2011 в 09:11 Источник Поделиться