如何重构这种方法？

问题描述

|

private void Update_Record_Click(object sender,EventArgs e)  
    {  
        ConnectionClass.OpenConnection();  

        if (textBox4.Text == \"\" && textBox2.Text == \"\")  
        {  
            MessageBox.Show(\"No value entred for update.\");  
        }  
        else if (textBox4.Text != \"\" && textBox2.Text != \"\")  
        {  
            sqlCommand cmd = new sqlCommand(\"update medicinerecord set quantity=\'\" + textBox2.Text + \"\' where productid=\'\"+comboBox1.Text+\"\'\",ConnectionClass.OpenConnection());  
            cmd.ExecuteNonQuery();  

            cmd = new sqlCommand(\"update myrecord set price=\'\" + textBox4.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
            cmd.ExecuteNonQuery();
            ConnectionClass.CloseConnection();
        }
        else if (textBox2.Text != \"\")
        {
            sqlCommand cmd = new sqlCommand(\"update myrecord set quantity=\'\" + textBox2.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
            cmd.ExecuteNonQuery();
            ConnectionClass.CloseConnection();
        }
        else if (textBox4.Text != \"\")
        {
            sqlCommand cmd = new sqlCommand(\"update myrecord set price=\'\" + textBox4.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
            cmd.ExecuteNonQuery();
            ConnectionClass.CloseConnection();
        }  
}  

它工作正常，但我想使其更短，以便于理解。我该如何重构？     

解决方法

如果您不了解应用程序特定部分的最新信息，则诸如“ 1”之类的语句根本没有任何意义。在这种情况下，似乎它们各自代表一个值，并且至少其中一个值应包含某种东西，以使任何操作合法。研究SQL语句表明，

textBox2

是数量，

textBox4

是价格。首先是将这些控件名称更改为更有意义的名称。 其次，我将检查包装到具有更多描述性名称的方法中：

private bool HasPriceValue()
{
    return !string.IsNullOrEmpty(textBox4.Text);
}

private bool HasQuantityValue()
{
    return !string.IsNullOrEmpty(textBox2.Text);
}

然后可以这样重写if块：

if (!HasPriceValue() && !HasQuantityValue())
{ 
    MessageBox.Show(\"No value entred for update.\"); 
}
else
{
    ConnectionClass.OpenConnection(); 
    if (HasQuantityValue())  
    {  
        SqlCommand cmd = new SqlCommand(\"update medicinerecord set quantity=\'\" + textBox2.Text + \"\' where productid=\'\"+comboBox1.Text+\"\'\",ConnectionClass.OpenConnection());  
        cmd.ExecuteNonQuery();  
    }
    if (HasPriceValue())
    {
        SqlCommand cmd = new SqlCommand(\"update myrecord set price=\'\" + textBox4.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
        cmd.ExecuteNonQuery();
    }
    ConnectionClass.CloseConnection();
}

这样，您就不会在代码中重复执行SQL查询，并且很容易阅读代码并理解其作用。下一步将按照Darin的建议，重写SQL查询以使用参数代替串联字符串（这将打开代码以进行SQL注入攻击）。     ,免责声明：如Darin所建议，我对他的原始解决方案做了一些更改。布朗博士。 该代码很大的事实是最少的问题。您在这里使用SQL注入有更大的问题。您应该使用参数化查询来避免这种情况。 因此，我将从将数据访问逻辑外部化为单独的方法开始：

public void UpdateMedicineRecordQuantity(string tableName,string attributeName,string productId,string attributeValue)
{
    using (var conn = new SqlConnection(\"YOUR ConnectionString HERE\"))
    using (var cmd = conn.CreateCommand())
    {
        conn.Open();
        cmd.CommandText = \"UPDATE \" + tableName + \"SET \" + attributeName+ \" = @attributeValue where productid = @productid\";
        cmd.Parameters.AddWithValue(\"@attributeValue\",attributeValue);
        cmd.Parameters.AddWithValue(\"@productid\",productId);
        cmd.ExecuteNonQuery();
    }
}

接着：

string productId = comboBox1.Text;
string quantity = textBox2.Text;
UpdateMedicineRecordQuantity(\"medicinerecord\",\"quantity\",productId,quantity);

只要您不让用户为这两个参数提供输入，就可以使用\“ tableName \”和\“ attributeName \”作为SQL的动态部分没有安全问题。 您可以在其他情况下继续使用此方法。     ,为每个命令（在

if

语句内部）创建字符串（或字符串数​​组），如果字符串不为null，则随后运行sql。 [注意] 您不会在任何情况下都关闭连接 [/注意]     ,使代码“小”可能不是使其可读或“易于理解”的最佳方法。我发现比简洁的代码具有良好的质量，变量，方法，属性，类名等布局更好的代码，更难理解。 我认为更严重的问题不是代码的大小，而是您容易受到SQL Injection攻击的事实。 这是我写的一篇文章，可能对您有所帮助：SQL注入攻击以及有关如何防止它们的一些技巧 希望对您有所帮助。     ,您的代码正在等待SQL注入。当心\'ol约翰尼表。     ,除了使其更易于理解之外，您可能还希望对其进行重构，以使其更易于测试。 在这种情况下，可能需要摆脱“ 9”个单例（而是使用IoC并注入连接工厂）和MessageBox（例如，引发异常并让周围的代码确定如何显示此消息）     ,我对代码进行了一些简化和优化，并添加了一些注释。

private void Update_Record_Click(object sender,EventArgs e)  
{  
    if (textBox4.Text == \"\" && textBox2.Text == \"\")
    {
        MessageBox.Show(\"No value entered for update.\");  
        return;
    }
    ConnectionClass.OpenConnection();  

    var cmd = new SqlCommand(\"update medicinerecord set quantity=\'\" + textBox2.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
    cmd.ExecuteNonQuery();  

    cmd = null;
    if (textBox2.Text != \"\")
        cmd = new SqlCommand(\"update myrecord set quantity=\'\" + textBox2.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());
    else if (textBox4.Text != \"\")
        cmd = new SqlCommand(\"update myrecord set price=\'\" + textBox4.Text + \"\' where productid=\'\" + comboBox1.Text + \"\'\",ConnectionClass.OpenConnection());

    if (cmd != null) cmd.ExecuteNonQuery();
    ConnectionClass.CloseConnection();
}  

如果只执行单行代码，则可以通过删除条件语句（例如ѭ8）上的花括号来立即简化代码。我为你做了。 在未先转义任何用户输入的使用变量之前，请勿使用字符串连接构建SQL语句。即，

update myrecord set price=\'\" + textbox4.Text + \"\'

..至少应为

update myrecord set price=\'\" + textbox4.Text.Replace(\"\'\",\"\'\'\")

..，以避免可能的SQL注入攻击。 除了测试

.Text == \"\"

之外，通常最好在.NET 4上使用

!string.IsNullOrEmpty()

或

!string.IsNullOrWhitespace()

来覆盖空字符串和空字符串。 将所有显式键入的数据类型替换为“ 17”。 最后，您可以简化代码，方法是在不满足验证条件时立即退出（如前两行所示），并在验证完成后在此方法的顶部和底部指定一个

OpenConnection()

和

CloseConnection()

。 还解决了拼写错误！ :)     ,您可以使用参数，当您不想更新字段时可以将其设置为

NULL

：

UPDATE myrecord SET price = ISNULL(@price,price) -- MSSQL

然后，您可以使用

Command.Parameters.AddWithValue

方法根据文本字段执行一个命令。用

DBNull.Value

表示

NULL

。即使不更改结构，也应该这样做以防止SQL注入攻击。 干杯，马蒂亚斯     ,你可以看到

 cmd.ExecuteNonQuery();                 
    ConnectionClass.CloseConnection();  

是多余的，所以为什么不将它移到最后一个26示波器之后。     ,为什么不简单地再创建一个方法：

void ExecuteCommand(string sql)
{
           SqlCommand cmd = new SqlCommand(sql);  
            cmd.ExecuteNonQuery();
            ConnectionClass.CloseConnection();
}