0

I am making a C# tool that connects to a SQL database to manage users for another program I created. I'm displaying the Users in a ListBox, and I can add/delete users. Things got weird after I deleted some profiles, and I thought that could have something to do with how I delete the row from the database. I'm using an Id that automatically increases with every new user creation. This is my code for deleting from the database:

using (conn = new SqlConnection(connectionString))
using (SqlCommand command = new SqlCommand("DELETE FROM myDatabase WHERE Id = '" + (listBox1.SelectedIndex + 1) + "'", conn))
{
    conn.Open();
    command.ExecuteNonQuery();
}

and this is how I load the users into my listbox:

using (conn = new SqlConnection(connectionString))
using (SqlDataAdapter sqlAdapt = new SqlDataAdapter("SELECT * FROM myDatabase", conn))
{
    DataTable dataTable = new DataTable();
    sqlAdapt.Fill(dataTable);
    listBox1.DisplayMember = "Name";
    listBox1.ValueMember = "Id";
    listBox1.DataSource = dataTable;
}

How can I delete the correct row from the database?

Heretic Monkey
  • 11,687
  • 7
  • 53
  • 122
Dafuqisthis
  • 1
  • 1
  • 6
  • 2
    You should use SelectedValue instead of SelectedIndex. It's still unclear for me what is your problem though. What is "getting weird" for a listbox? – Kinetic Jul 18 '16 at 20:42
  • how many ID's are in the List..? and how can you expect it to delete all of the ID's if there are more than 1 unique ID's in the Database..? have you thought about wrapping the code around a foreach loop as well as reformatting your Delete statement to use proper Parameterized values for the query..? and store the SelectedValue in a variable not to mention you do not need `(' wrapped around your `listBox` object. learn how to construct a parameterized query.. or create a stored procedure and pass in the values properly. – MethodMan Jul 18 '16 at 20:42
  • Please consider using LINQ2SQL or EF. No, this isn't a solution to your problem - which is why it's a comment. – SQLMason Jul 18 '16 at 20:58

2 Answers2

3

You should use the property SelectedValue to find your ID not the SelectedIndex

if(listBox1.SelectedValue != null)
{
    int userID = Convert.ToInt32(listBox1.SelectedValue);
    using (conn = new SqlConnection(connectionString))
    using (SqlCommand command = new SqlCommand("DELETE FROM myDatabase WHERE Id = @uid", conn))
    {

        conn.Open();
        command.Parameters.Add("@uid", MySqlDbType.Int32).Value = userID;
        command.ExecuteNonQuery();
    }
}

The problem with SelectedIndex is that this value goes from 0 to the max number of items in the listbox. This value has nothing to do with the ID of your user automatically calculated by your database. (After just one delete and one add these value are out of synch)

Note also that an sql text should never built using string concatenation. This is a well know security problem called Sql Injection.

Community
  • 1
  • 1
Steve
  • 213,761
  • 22
  • 232
  • 286
  • Steve says "Note also that an sql text should never built using string concatenation" only apply if outside users have access to your input string data. Considering you is using values from a predetermined list, this will not be a problem – JSON Jul 18 '16 at 21:18
  • 1
    And so? Using parameters is better for a lot of reasons. Not just for Sql Injection. Parsing problem is another one. And (not sure with MySql) but it is well known with Sql Server that a parameterized query can be better optimized by the database engine. No don't take this habit even when it is 'safe'. – Steve Jul 18 '16 at 21:20
  • @JSON Note that the OP *also* ticks the selectedindex converting a number to text accidentally - for that to happen with Parameters, it is more obvious. – Ňɏssa Pøngjǣrdenlarp Jul 18 '16 at 21:26
  • No, I agree stevo. I just wanted to point that out. In my opinion it is actually easier to user parameters anyways. Plus you have less chance of screwing up datatypes in your commandText, forgetting those pesky single quotes on strings is something I see a lot on here. – JSON Jul 18 '16 at 21:28
  • @JSON - I get that you are pushing the parameter solution but would like to point out that "values in a predetermined list" can be intercepted and manipulated on the client and should still not be considered safe. – WillC Jul 18 '16 at 22:12
-1
"DELETE FROM myDatabase WHERE Id = '" + (listBox1.SelectedIndex + 1) + "'"

I'm not sure about mySQL, but it looks like you pass a string instead of an int. When you pass a parameter as a number you should remove the " ' ". so, it will look like:

"DELETE FROM myDatabase WHERE Id = " + (listBox1.SelectedValue)
abarisone
  • 3,707
  • 11
  • 35
  • 54
TTomer
  • 356
  • 2
  • 11
  • 3
    `+ (listBox1.SelectedValue)` Incorrect.. you do not need the `+` sign also do not encourage others to use concatenated query's this is introducing Sql Injection.. always recommend using Parameterized Querys – MethodMan Jul 18 '16 at 20:46
  • You are right. however, my point was to teach him that he passed the parameter as a string instead of int. the concept of his question needed clarification, not the proper way to do it. – TTomer Jul 18 '16 at 20:48
  • the point is you do not need to wrap the listbox1 around `( )` regardless.. his question in regards to clarification was straight forward you should never construct query's this way. perhaps you should read up on `Sql Injection` once again your answer is incorrect and you're posting incorrect information which someone could potentially use as an incorrect example on how to do something – MethodMan Jul 18 '16 at 20:51
  • I wanted to keep his question as he asked and just clarify him the main issue with his code. The main issue is that he tried to send string instead of int. not that he could do it better or a way to avoid SQL injection.\ – TTomer Jul 18 '16 at 20:52
  • 1
    cheers, I think you are missing my point in reference to your post non answer. you do not need to wrap the object.value around `( )` – MethodMan Jul 18 '16 at 20:55