我该如何重构这种方法？

Question

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

它工作正常，但我想缩短它以便更容易理解。我如何重构它？

Answer 1

Statments像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 (!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查询中的代码和它很容易阅读代码并理解它做什么。下一步是重写SQL查询以使用参数而不是连接字符串（这会打开您的代码以执行SQL注入攻击），as suggested by Darin。

Answer 2

为每个命令（在if语句中）创建字符串（或字符串数​​组），如果字符串不为空，则在之后运行sql。

【注意事项】
你不要关闭您的连接在任何情况下
[/注]

Answer 3

除了使它更容易理解，你可能要重构它，所以它是容易测试。

在这种情况下可能需要摆脱ConnectionClass单身的（而是使用IOC和注入连接工厂）和在MessageBox（例如抛出异常，并让周围的代码确定如何显示这条消息）

Answer 4

您可以使用参数，你可以设置为NULL，当你不希望更新的字段：

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

然后你可以一个命令取决于与Command.Parameters.AddWithValue法的文本字段。对于NULL，使用DBNull.Value。即使你不改变结构，你也应该这样做以防止SQL注入攻击。

欢呼声中，马蒂亚斯

Answer 5

你可以看到，

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

是reduntant，为什么最后else if范围后，不动它。

Answer 6

使代码“小”可能不是使其易读或“易于理解”的最佳方式。我发现更多简洁的代码比代码更好理解，这些代码是用良好的质量变量，方法，属性，类名等来编写的。

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

这里是一个文章我写回来的路上，你可能会发现有用：SQL Injection attacks and some tips on how to prevent them

我希望帮助。

Answer 7

声明：正如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); 

使用“表名”和“的attributeName”为您的SQL的动态部位只要您不让用户为这两个参数提供输入，就没有安全问题。

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

Answer 8

你为什么不干脆再创建一个方法：

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

Answer 9

你的代码是SQL注入等待发生。注意'ol Johnny Tables。

Answer 10

我已简化并优化了您的代码，并提供了一些评论。

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 injection攻击。
3. 代替测试.Text == ""，通常优选在.NET 4上使用!string.IsNullOrEmpty()或!string.IsNullOrWhitespace()来覆盖空字符串和空字符串。
4. 替换var的任何明确类型的数据类型。
5. 最后，只要不满足验证条件（如前几行所示），您可以简化该代码，并在验证发生后在此方法的顶部和底部指定一个OpenConnection()和CloseConnection() 。

还修正了拼写错误！ :)