Fill TextBox from ListBox on click

Question

I have a form in a WinForms application that has a Textbox called txtSupplier, a button called btnSrchSuppliers, a label called lblSuppName, and a ListBox lstSuppliers.

When the user clicks the button, it goes to a database and fetches a list of suppliers and lists them in the ListBox, selecting a supplier from the list fills the Textbox with his ID and the label with the name.

This code is used and fully working but I feel that it is not quite good.

Private Sub btnSrchSuppliers_Click(sender As Object, e As EventArgs) Handles btnSrchSuppliers.Click
 Dim suppliers As New List(Of SupplierDTO)
 lblSuppName.Text = String.Empty
 If lstSuppliers.Visible Then
 lstSuppliers.Hide()
 Else
 Try
 lstSuppliers.ValueMember = "ID"
 lstSuppliers.DisplayMember = "Description"
 lstSuppliers.BringToFront()
 suppliers = ItemBAL.SelectSuppliers(txtSupplier.Text.Trim()) 'Get Suppliers
 If suppliers.Count = 0 Then
 MsgBox("Supplier not found", MsgBoxStyle.Critical, "Supplier")
 ElseIf suppliers.Count >= 1 Then
 lstSuppliers.DataSource = suppliers
 If suppliers.Count > 1 Then
 lstSuppliers.Visible = True
 Else
 lstSuppliers.SelectedIndex = 0
 lstSuppliers_Click(lstSuppliers, e)
 End If
 End If
 Catch ex As Odbc.OdbcException
 MsgBox(ex.Message, MsgBoxStyle.Critical, "sql error")
 Catch ex As Exception
 MsgBox(ex.Message, MsgBoxStyle.Critical, "general error")
 End Try
 End If
 End Sub

and

Private Sub lstSuppliers_Click(lstSuppliers As ListBox, e As EventArgs) Handles lstSuppliers.Click
 Dim selectedSupp As SMS.DTO.SupplierDTO
 selectedSupp = lstSuppliers.SelectedItem
 lstSuppliers.Visible = False
 txtSupplier.Text = selectedSupp.ID
 lblSuppName.Text = selectedSupp.Description
 End Sub

What can be improved?

Answer 1

As I understand your description, the form loads without data. The user initiates the data loading by clicking a button and then they select the desired supplier. To me, that is asking the user to do too much.

I prefer front-loading the data. Somewhere during program initialization, make your database call using an async method. Maybe something as:

Dim suppliers As New List()
supplier = Await GetSuppliersAsync()
Async Function GetSuppliersAsync()
 Return Task.Run(Function() SupplierDTO().GetList())
End Function

Then as the form loads you can populate the list box with the results from the suppliers object and eliminate one step the user has to perform.

This is how I would approach it. I am open to community scrutiny on this approach.

Edit to answer

Let's say you load a data structure with a list of suppliers on application, or form, init. In that case, the entire btnSrchSuppliers_Click subroutine is obsolete. You can remove it which will clean up the code smell.

• user10191234: Thanks for the suggestion but I also need, if it exists, a better was to handle the click.
• Paul Stoner: Not sure I follow. If you move the database search outside of this process, then the click simply becomes transferring the selected item attributes to the label and textbox. I don't see anything wrong with that approach
• user10191234: Upvoted. But what I am asking for is the code quality, lots of if and a try/catch
• Paul Stoner: @user10191234, I understand now. Apologies for the confusion. I've edited my answer explaining how the button click event handler can be removed by moving the database call. That will clean up your code smell