5

I've been searching on the internet, but I just can't seem to find anything that explain my question (could be that I'm not using the correct search string), so I'm posting here hoping that someone can help me with this. (My program is written is C# using Visual Studio 2010)

I notice that in C#, there are multiple ways of constructing a SQL command.

SqlConnection connection = GetAndOpenConnection(); //function containing connection string and open connection
SqlCommand command = connection.CreateCommand();

Up to this point, I have no questions. The problem I have is with CommandText. I am using several different commands in my code(SELECT/INSERT/UPDATE/DELETE), but lets take SELECT for example.

//Example 1:
command.CommandText = String.Format("SELECT * FROM myTable WHERE name = '{0}'", "bob");

//Example 2:
command.CommandText = "SELECT * FROM myTable WHERE name = @myName";
command.Parameters.Add(new SqlParameter("myName", "bob"));

What are the differences between the above 2 examples? (performance wise/structure wise/etc)

The reason I'm asking is because within the same .cs file, when I use the method in Example 2, sometime the code works properly and sometime it doesn't, then I eventually makes everything like in Example 1, which works every single time.

Is there a significant gain/loss using either method? Which is the more proper way of completing a task like this?

ADDITIONAL QUESTION

Okay, so I see that method 2 is the more appropriate way to do this.

However, there's a problem if I use method 2.

I have a loop that loop through a List<string> names. Inside the loop, when I use method 2 and add name as parameters, I got an error saying that the parameter already exist and cannot be added.

How can I go about solving this?

List<string> names = new List<string> {"adam", "bob", "john"};
foreach(string name in names)
{
    command.CommandText = "SELECT * FROM myTable WHERE name = @myName";
    command.Parameters.Add(new SqlParameter("myName", name));
    reader = command.ExecuteReader();

    while(reader.Read())
    {
        //loop through each cell and print on the Console
    }
}

Also, I know that people mentioned that in the parameter it should be "@myName" instead of "myName". I remember having this issue as I was confused which way to use and had to just test it. "@myName" was not working for me but "myName" is, and this is how it's in the code right now in the parts where I still use method 2. I'm using .Net 4.0, don't know if this will make a difference.

sora0419
  • 2,308
  • 9
  • 39
  • 58

4 Answers4

9

Parameters exist to prevent SQL Injection. For example, consider what happens in case of string.Format if instead of bob there was TextBox1.Text which contained 1';DROP TABLE myTable;'.

SQL Injection is not possible if you have complete control over the parameters, like in your case with string literal for parameter. However you never know how your code might change in future, so as a rule of thumb you should always stick to safer approach with parameters.

If you are facing some particular problems with second approach - search and maybe post here, there is most likely a solution already. For example, in your code snippet actual parameter name is @myName with @ symbol, and that is what should be supplied to the SqlParameter constructor.

Update. In your additional question problem is exactly in parameters naming - it should be @myName:

command.Parameters.Add(new SqlParameter("@myName", name));

Also you should clear parameters collection on each iteration:

command.Parameters.Clear();

Although it would be better to create new command on every iteration to avoid mess - check this thread for details.

Community
  • 1
  • 1
Andrei
  • 55,890
  • 9
  • 87
  • 108
  • I could be wrong, but I thought creating command in each iteration will take up too much space (Listnames is pretty large, I only put small sample size in the question), that's why I declared the SqlCommand outside the loop and just change CommandText inside the loop. I'll try the method you provided. – sora0419 Mar 03 '14 at 18:01
  • @sora0419, unused objects will be garbage collected and won't cause any problems with memory, especially if you wrap them in `using` for proper disposal. – Andrei Mar 03 '14 at 18:03
5

As noted, the first (dynamic) query is susceptible to a SQL injection attack. Further, it must be recompiled each and every time it's executed, raising the execution cost (and potentially blocking while the compile takes place.)

The second (parameterized) query is not susceptible to a SQL injection attack. Further, it's execution plan can be cached, such that it's only compiled once, when it is first executed. At least until the cache expires or is flushed for one reason or other.

Nicholas Carey
  • 71,308
  • 16
  • 93
  • 135
3

Method 1 : vulnerable to SQL injection

Method 2 : safe way to do SQL with a little bit more work

BTW it should be command.Parameters.Add(new SqlParameter("@myName", "bob"));

Steve
  • 11,696
  • 7
  • 43
  • 81
  • I remember getting confused with this. I wasn't sure if its "@myName" or "myName" and had to test it out. "myName" is the one that actually work for me, and its still this way in my code where I still used Method 2. (I have .Net 4.0, not sure if this makes a difference.) – sora0419 Mar 03 '14 at 17:33
3

All of the above answers are correct, but to answer your edit:

You need to dispose of the command after using it in each iteration:

using (var conn = new SqlConnection(connectionString)
{
    conn.open(); //You only need to open the connection once, so we do it outside the loop
    foreach (var name in names)
    {
        using (var cmd = new SqlCommand("SELECT * FROM myTable where name = @MyName", conn)
        {
            cmd.Parameters.AddWithValue("@MyName", name);
            //Do something with the command
        }
        //The command is disposed of here
    }
}
//The connection is disposed of here.
Evan L
  • 3,805
  • 1
  • 22
  • 31