我該如何重構這種方法？

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() 。

還修正了拼寫錯誤！ :)