CODEWITHSUNDEEP
vb.netvisual-studio-2010visual-studio
user1532468
user1532468

Reputation: 1753

while loop only displaying 1 item

I am using a while loop to populate a second combo based on the value of the first combobox selection. What is happening however, is that the loop is only displaying 1 item in the second combobox instead of about 20. If I set breakpoint on the while loop I can see that all items are being calculated but just not appearing in the combobox.

I would be grateful if someone could point my basic newbie error. Many thanks

Private Sub cmbCustomer_SelectedIndexChanged(ByVal sender As System.Object, _
                                             ByVal e As System.EventArgs) _
    Handles cmbCustomer.SelectedIndexChanged

    sql = "SELECT * from Departments WHERE Customer = '" & cmbCustomer.Text & "'"

    Dim cmd As New OleDb.OleDbCommand

    cmd.CommandText = sql
    cmd.Connection = oledbCnn
    dr = cmd.ExecuteReader

    While dr.Read()
        If (dr.HasRows) Then
            cmbDept.Text = CStr((dr("Name"))) <--- 2nd combobox
        End If
    End While

    cmd.Dispose()
    dr.Close()
End Sub

Upvotes: 0

Views: 221

Answers (2)

Olivier Jacot-Descombes
Olivier Jacot-Descombes

Reputation: 112352

The Text property of the combo box contains only what is displayed for the selected item. You need to add the items to the Items collection:

cmbDept.Items.Add(CStr(dr("Name")))

The combo boxes, list boxes etc. display items by calling their ToString() method. Therefore calling CStr should not even be necessary:

cmbDept.Items.Add(dr("Name"))

You are inserting a value in the SQL statement by concatenating strings. If you are just using your program for yourself, this is okay; however, on productive environments this is dangerous. Someone could enter a value that terminates the SELECT statement and introduces another malicious statement. E.g. a DELETE statement that deletes a whole table. This is called a SQL injection attack.

There are two ways to deal with this:

1) Escape the string:

    sql = "SELECT * FROM Dep WHERE Cust = '" & s.Replace("'", "''") & "'"

2) Use command parameters:

    sql = "SELECT * from Departments WHERE Customer = ?"
    Dim cmd As New OleDbCommand(sql, oledbCnn)
    cmd.Parameters.AddWithValue("@p1", cmbCustomer.Text)

If you are inserting dates this also has the advantage that you don't need to bother about date formats.

You can simplify your loop to:

While dr.Read()
    cmbDept.Text = CStr(dr("Name")) 
End While

There is no need to test for HasRows since dr.Read() would return False anyway if no rows were available.

You can have Dispose called automatically by VB with the Using statement:

Using cmd As New OleDbCommand(sql, oledbCnn)
    'TODO: Work with cmd here.
End Using

Dispose will be called at the end of the Using block, even if an error occurs within the Using block or the Using block is left by Return or another statement.

Upvotes: 5

Karl Anderson
Karl Anderson

Reputation: 34846

You do not need to check if the data reader has rows every iteration, just check it before you loop.

You are not adding the items to the list, but rather setting the Text property of cmbDept, instead do this:

Private Sub cmbCustomer_SelectedIndexChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles cmbCustomer.SelectedIndexChanged
    sql = "SELECT * from Departments WHERE Customer = '" & cmbCustomer.Text & "'"

    Dim cmd As New OleDb.OleDbCommand

    cmd.CommandText = sql
    cmd.Connection = oledbCnn
    dr = cmd.ExecuteReader

    If (dr.HasRows) Then
        While dr.Read()
            cmbDept.Text = CStr((dr("Name"))) <--- 2nd combobox
        End While
    End If

    cmd.Dispose()
    dr.Close()
End Sub

Also, it is highly recommended that you use a parameterized query as to avoid a visit from Little Bobby Tables, like this:

Private Sub cmbCustomer_SelectedIndexChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles cmbCustomer.SelectedIndexChanged
    sql = "SELECT * from Departments WHERE Customer = @Customer"

    Dim cmd As New OleDb.OleDbCommand
    cmd.Parameters.AddWithValue("@Customer", cmbCustomer.Text)

    cmd.CommandText = sql
    cmd.Connection = oledbCnn
    dr = cmd.ExecuteReader

    If (dr.HasRows) Then
        While dr.Read()
            cmbDept.Text = CStr((dr("Name"))) <--- 2nd combobox
        End While
    End If

    cmd.Dispose()
    dr.Close()
End Sub

Upvotes: 4

Related Questions