如何重构此方法

本文关键字：此方法 重构 何重构 | 更新日期: 2023-09-27 17:57:54

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

它工作正常，但我想把它缩短，这样更容易理解。我该如何重构它？

如何重构此方法

免责声明：根据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"answers"attributeName"作为SQL的动态部分并没有安全问题，只要不让用户提供这两个参数的输入即可。

您可以继续对其他情况重用此方法。

如果你没有跟上应用程序特定部分的进度，那么像if (textBox4.Text == "" && textBox2.Text == "")这样的统计信息就没有任何意义。在这种情况下，它们似乎每个代表一个值，并且其中至少有一个应该包含一些内容，以便任何操作都是合法的。研究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注入攻击打开代码(。

为每个命令(在if语句中(创建字符串(或字符串数组(，如果字符串不为null，则随后运行sql。

[注意]
并非在任何情况下都关闭连接
[/note]

使代码"小"可能不是使其可读或"易于理解"的最佳方式。我发现更简洁的代码比那些有着良好质量的变量、方法、属性、类名等的代码更难理解。

我认为更严重的问题不是代码的大小，而是您容易受到SQL注入攻击的事实。

这是我很久以前写的一篇文章，你可能会发现它很有用：SQL注入攻击和一些关于如何防止它们的提示

我希望这能有所帮助。

您的代码正在等待SQL注入。小心老Johnny Tables。

为了使其更容易理解，您可能需要对其进行重构，使其更易于测试。

在这种情况下，这可能需要去掉ConnectionClass singleton(而是使用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();
}
  1. 如果只执行单行代码，则可以通过在条件语句(如if(上删除大括号来立即简化代码。我这样做是为了你
  2. 不要在不首先转义任何用户输入的变量的情况下使用字符串串联来构建SQL语句。即update myrecord set price='" + textbox4.Text + "'。。应至少为update myrecord set price='" + textbox4.Text.Replace("'", "''")。。以避免可能的SQL注入攻击
  3. 与测试.Text == ""不同，通常最好在.NET4上使用!string.IsNullOrEmpty()或!string.IsNullOrWhitespace()来覆盖空字符串和空字符串
  4. 替换var的任何显式类型的数据类型
  5. 最后，您可以通过在未满足验证时立即返回(如前几行所示(并在验证发生后在该方法的顶部和底部指定一个OpenConnection()和CloseConnection()来简化此代码

还修复了一个拼写错误！：(

您可以使用参数，当您不想更新字段时，可以将其设置为NULL：

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

然后，您可以使用Command.Parameters.AddWithValue方法根据文本字段创建一个命令。NULL使用DBNull.Value。即使不更改结构，也应该这样做以防止SQL注入攻击。

干杯，Matthias

您可以看到

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

是多余的，所以为什么不把它移到最后一个else if作用域之后呢。

为什么不简单地再创建一个方法：

void ExecuteCommand(string sql)
{
           SqlCommand cmd = new SqlCommand(sql);  
            cmd.ExecuteNonQuery();
            ConnectionClass.CloseConnection();
}
相关文章：