foreach (variable in checklistbox.checkedItems) in c# vs2012

Question

I'm trying to make a program where the user can select several items in a combo listbox, retrieves its id from my SQL database, returns the id, and inserts the id in another table. I'm not sure if this is the most effective way, and suggestions are welcome. However, I ran into the problem in how to complete my foreach statement. What should I place in the position marked by X below? (in the foreach statement)

Here's my code: (clbGeneral is the name of the checklistbox)

if (clbGeneral.CheckedIndices.Count != 0)
{
    foreach ( X in clbGeneral.CheckedItems)
    {
        con.Open();
        cmd = new SqlCommand("SELECT serv_id from services where serv_name='" + clbGeneral.SelectedItem.ToString() + "';", con);
        serv_id = (int)cmd.ExecuteScalar();

        con.Close();
        transactSQL();
        MessageBox.Show(clbGeneral.SelectedItem.ToString());
    }
}

the transactSQL function:

public void transactSQL() {
    con.Open();
    cmd = new SqlCommand("INSERT INTO transactions VALUES ('" + txtDate.Text + "','" + dataPnts.SelectedRows[0].Cells[3].Value.ToString() + "','" + dataProvs.SelectedRows[0].Cells[4].Value.ToString() + "','" +serv_id.ToString() + "');", con);
    cmd.ExecuteNonQuery();
    con.Close();
}

Sorry, my bad. I'll have to edit it I guess. Thanks

Shrumms Dorke
– Shrumms Dorke

2014-03-02 04:14:33 +00:00
Commented Mar 2, 2014 at 4:14 — Shrumms Dorke
– Shrumms Dorke, Commented Mar 2, 2014 at 4:14

poke · Accepted Answer · 2014-03-02 04:27:45Z

You don’t actually have that much choice there. CheckedListBox.CheckedItems is a CheckedItemCollection which happens to store its elements as plain objects. So the foreach will only be able to get individual items of type object from that:

foreach (object item in clbGeneral.CheckedItems)
{ … }

Of course, you could also use var item, letting the compiler infer the type automatically. That won’t change the variable type though, as the compiler would perform the same thinking process as I did above. So item would still be an object.

Of what type those objects actually are, depends on what items you put into it in the first place. If all those items are of a certain type, you can type-cast them inside the loop.

To continue further though, your SQL statement within the loop should reflect the item of that loop iteration, and not just the selected item. So your command could look like this—but that again depends a bit on what objects you added to the list, and if their ToString value is good enough for the query:

cmd = new SqlCommand("SELECT serv_id from services where serv_name='" + item.ToString() + "';", con);

So that was the first part of my answer. The next part is more of a code review, to make sure you don’t do bad stuff. First of all, for SqlConnections, the recommended way to close it is by not doing it yourself, but using the connection within a using statement. That way you ensure that the connection is closed even if errors might terminate the current code:

using (var con = OpenConnection())
{
    con.Open();
    var cmd = new SqlCommand(…, con);
    cmd.ExecuteScalar();
}

Next, you really shouldn’t just append some text into a SQL query. The chance for SQL injections is just to high. Instead, you should just use parameterized queries, which automatically take care of passing the values properly, escaping stuff as needed etc.:

var cmd = new SqlCommand("SELECT serv_id from services where serv_name=@Name", con);
cmd.Parameters.Add("@Name", SqlDbType.VarChar).Value = item.ToString();
cmd.ExecuteScalar();

Sudhakar Tillapudi · Accepted Answer · 2014-03-02 04:32:40Z

1

CheckedListBox.Items is a collection of Objects and CheckedListBox.CheckedItems returns the subset of objects in CheckedListBox Items only those items whose System.Windows.Forms.CheckState is Checked or Indeterminate.

You can use Type var or object to represent each item in the CheckedItems and call ToString() to get the each checked Item in the foreach loop.

Try This:

foreach (var item in clbGeneral.CheckedItems)
{
    con.Open();
    cmd = new SqlCommand("SELECT serv_id from services where serv_name='" + item 
          .ToString() + "';", con);
    serv_id = (int)cmd.ExecuteScalar();
    con.Close();
    transactSQL();
    MessageBox.Show(item.ToString());
}

edited Mar 2, 2014 at 4:32

answered Mar 2, 2014 at 4:21

Sudhakar Tillapudi

26.3k5 gold badges43 silver badges70 bronze badges

Comments

Kevin · Accepted Answer · 2014-03-02 04:21:16Z

0

foreach (var checkedItem in clbGeneral.CheckedItems)
{

}

answered Mar 2, 2014 at 4:21

Kevin

4,84821 gold badges76 silver badges122 bronze badges

Comments

David · Accepted Answer · 2014-03-02 04:58:40Z

0

What should I place in the position marked by X below?

As noted in other answers, you can just use "var" and let the compiler worry about it.

I'm not sure if this is the most effective way, and suggestions are welcome.

1) If I were you, I wouldn't open and close the connection inside the foreach--I'd open it just before the loop and close it just after. And, actually, I'd wrap it in a try-finally with the close inside the finally. In order for this to work, you'll need to not open and close inside of transactSQL, either. If that breaks other parts of your code, you could introduce a new variable that tracks whether con was open or closed when you entered the method and then refers to it to decide whether or not to call open and/or close.

2) Instead of making 2 round trips to the database, you can do it all in 1 call. This would actually eliminate the need to call transactSQL at all. I believe the following would work for you (or at least something very close to it):

if (clbGeneral.CheckedIndices.Count != 0)
{
    con.Open();
    try
    {
        foreach (var item in clbGeneral.CheckedItems)
        {
            string sql=string.Format("INSERT INTO transactions (<column names here>) " +
            "SELECT '{0}', '{1}', '{2}', serv_id "+ // add "TOP 1" if you might get more than 1 result and only want to do 1 insert
            "FROM services " +
            "WHERE serv_name=@Name); ",
            txtDate.Text, dataPnts.SelectedRows[0].Cells[3].Value.ToString(),
            dataProvs.SelectedRows[0].Cells[4].Value.ToString(), item.ToString());

            cmd = new SqlCommand(sql, con);
            cmd.Parameters.Add("@Name", SqlDbType.VarChar).Value = item.ToString();
            cmd.ExecuteNonQuery();

            MessageBox.Show(clbGeneral.SelectedItem.ToString());
        }
    }
    finally
    {
        con.Close();
    }
}

The first time I run an ad hoc query, I like to put a debugging break point just after I've set sql and before I actually use it. That way I can grab the value of the SQL that was generated and then copy-paste it into Enterprise Manager and make sure it looks correct and doesn't generate and compile time errors before executing. I'd recommend you do that here since I can't check against your DB to make sure my syntax is 100% correct :)

edited Mar 2, 2014 at 4:58

answered Mar 2, 2014 at 4:51

David

5,0414 gold badges38 silver badges63 bronze badges

2 Comments

David Over a year ago

Poke is right about taking advantage of "using" if that is practical in your situation and also about using parameters to prevent sql injection. It would be good to incorporate that into this answer, too.

David Over a year ago

I've updated the code to use parameters as Poke suggests. As long as you are using try-finally, then you can probably get away without the "using" statement. However, if it doesn't cause you to have to do any extra refactoring, the "using" wouldn't hurt anything, either.

Collectives™ on Stack Overflow

foreach (variable in checklistbox.checkedItems) in c# vs2012

4 Answers 4

Comments

Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related