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?

use the zoom out feature of your editor to make the code small..... — David Heffernan, May 26 '11 at 12:18
If you're looking to improve other's understanding of it, you may also consider giving your controls meaningful ID's. — schummbo, May 26 '11 at 12:20
You could also use K&R style braces instead of BSD. That would save you a line with each block. — Yuck, May 26 '11 at 12:25

score 6 · Answer 1 · edited May 26 '11 at 12:46

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.

This should be extended to answer the original question: the method "UpdateMedicineRecordQuantity" should get two additional parameters named "tableName" and "columnName", making it reusable at all four places of the original code. Otherwise, I bet the OP would go and copy/paste/modify your example above four times. — Doc Brown, May 26 '11 at 12:36
@Doc Brown, feel free to edit my answer and include this refactoring. — Darin Dimitrov, May 26 '11 at 12:38

score 3 · Accepted Answer · edited May 23 '17 at 12:19

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.

Thanks for this great help next time i will keep in mind with meaningul name of controls. its realy worhty for me. And one thing its a simple window application and all operation will run in background so how is it underattack of SQLInjection. — avirk, May 26 '11 at 13:46
@avirk: a user can enter specially crafted SQL commands into one of the text boxes and perform any kind of operation in the database (including dropping tables). See here for further explanation: [XKCD SQL injection - please explain](http://stackoverflow.com/questions/332365/xkcd-sql-injection-please-explain) — Fredrik Mörk, May 26 '11 at 13:48
@avirk, see [my answer](http://stackoverflow.com/questions/6138301/how-can-i-refactor-this-method/6138413#6138413) which uses parametrized queries, that's how you prevent it. — Darin Dimitrov, May 26 '11 at 14:11

score 1 · Answer 3 · answered May 26 '11 at 12:18

1

create strings (or an array of strings) for each of your commands (inside of your if statements), if the strings are not null, then run your sql afterwards.

[note]
you don't close your connection in every case
[/note]

answered May 26 '11 at 12:18

KevinDTimm

14,226
3
42
60

Brian Agnew · Answer 4 · 2011-05-26T12:33:07.657

1

As well as making it easier to understand, you may want to refactor it so it's easier to test.

In this case that may entail getting rid of the ConnectionClass singletons (and instead use IoC and inject a connection factory) and the MessageBox (e.g. throw an exception and let the surrounding code determine how to display this message)

edited May 26 '11 at 12:33

answered May 26 '11 at 12:22

Brian Agnew

268,207
37
334
440

score 1 · Answer 5 · answered May 26 '11 at 12:24

Making the code "small" may not be the best way to make it readable or "easy to understand". I find more terse code to be much more difficult to understand than code that is well laid out with good quality variable, method, property, class names etc.

I think the more serious issue is not the size of the code, but the fact that you are susceptible to SQL Injection attacks.

Here is an article I wrote way back that you might find useful: SQL Injection attacks and some tips on how to prevent them

I hope that helps.

score 1 · Answer 6 · answered May 26 '11 at 12:27

1

Your code is SQL Injection waiting to happen. Watch out for 'ol Johnny Tables.

answered May 26 '11 at 12:27

dolphy

472
1
6
14

Well in this cartoon they call him Bobby Tables but same thing http://xkcd.com/327/ – dolphy May 26 '11 at 13:54

score 1 · Answer 7 · answered May 26 '11 at 12:34

I have streamlined and optimised your code a bit, with a few comments.

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

You can immediately simplify your code by removing braces on conditional statements such as if if you are only executing single lines of code. I have done this for you.
Don't build your SQL statements using string concatenation without first escaping any user inputted variables utilised. Ie, update myrecord set price='" + textbox4.Text + "'.. should be at the very least update myrecord set price='" + textbox4.Text.Replace("'", "''").. to avoid possible SQL injection attacks.
Instead of testing .Text == "", it is generally preferred to use !string.IsNullOrEmpty(), or !string.IsNullOrWhitespace() on .NET 4 to cover both null and empty strings.
Replace any explicitly typed data types for var.
and finally, you could simplify this code by returning out as soon as validation is not met (as seen in first couple of lines) and also specifying one OpenConnection() and CloseConnection() at the top and bottom of this method after the validation has occurred.

Also a spelling mistake fixed! :)

score 0 · Answer 8 · answered May 26 '11 at 12:23

You can use parameters, that you may set to NULL when you don't want to update a field:

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

Then you can one single command depending on the text fields with Command.Parameters.AddWithValue method. Use DBNull.Value for NULL. Even if you do not change the structure, you should do this to prevent SQL injection attacks.

Cheers, Matthias

score 0 · Answer 9 · answered May 26 '11 at 12:23

0

You can see that

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

is reduntant, so why not move it after the last else if scope.

answered May 26 '11 at 12:23

FIre Panda

6,537
2
25
38

score 0 · Answer 10 · answered May 26 '11 at 12:27

0

why don't you simply create one more method :

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

answered May 26 '11 at 12:27

Dipti Mehta

537
3
13

How can I refactor this method?

10 Answers10