How can I refactor this method?

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

It's working correctly but I want to make it shorter so that it's easier to understand. How can I refactor it?

Answer 1

Disclaimer: as suggested by Darin, I changed his original solution a little bit. Doc Brown.

The fact that this code is large is the least problem. You have a far bigger problems with SQL injection here. You should use parametrized queries to avoid this.

So I would start with externalizing the data access logic into a separate method:

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

and then:

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

Using "tableName" and "attributeName" as dynamic parts of your SQL is no security problem as long as you don't let the user provide input for those two parameters.

You could proceed reusing this method for the other cases.

Answer 2

Statments like if (textBox4.Text == "" && textBox2.Text == "") means nothing at all, if you are not up to speed on what that particular part of the application is up to. In this case it seems as if they represent one value each, and that at least one of them should contain something for any operation to be legal. Studying the SQL statements suggests that textBox2 is a quantity and textBox4 is a price. First thing would be to change those control names into something more meaningful.

Second, I'd wrap the checks into methods with more descriptive names:

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

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

Then you can rewrite the if-block as so:

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

This way you don't repeat the SQL queries in the code and it's fairly easy to read the code and understand what it does. The next step would be to rewrite the SQL queries to use parameters instead of concatenated strings (which opens your code for SQL injection attacks), as suggested by Darin.