Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have 3 combo boxes with the same 3 selections in each. If the user picks the same selection a 2nd time, the first combo box resets. I have 3 other sections that will do the same with about 15 combo boxes.

Is there a shorter/compact way to code what I'm doing?

Protected Sub ComBox_GER1_SelectedIndexChanged(sender As Object, e As EventArgs) Handles ComBox_GER1.SelectedIndexChanged
    Dim ValueSelected = ComBox_GER1.SelectedIndex

    If ComBox_GER2.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER2.ClearSelection()
    End If
    If ComBox_GER3.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER3.ClearSelection()
    End If

End Sub

Protected Sub ComBox_GER2_SelectedIndexChanged(sender As Object, e As EventArgs) Handles ComBox_GER2.SelectedIndexChanged
    Dim ValueSelected = ComBox_GER2.SelectedIndex

    If ComBox_GER1.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER1.ClearSelection()
    End If
    If ComBox_GER3.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER3.ClearSelection()
    End If
End Sub

Protected Sub ComBox_GER3_SelectedIndexChanged(sender As Object, e As EventArgs) Handles ComBox_GER3.SelectedIndexChanged
    Dim ValueSelected = ComBox_GER3.SelectedIndex

    If ComBox_GER1.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER1.ClearSelection()
    End If
    If ComBox_GER2.SelectedIndex = ValueSelected Then
        System.Threading.Thread.Sleep(500)
        ComBox_GER2.ClearSelection()
    End If
End Sub
share|improve this question
3  
Please don't append edited code from answers. They may get reviewed, which could complicate the review process. You may ask a new question with the updated code when ready. But if this updated is showing errors, then it should be asked on Stack Overflow. –  Jamal May 20 at 14:33
add comment

2 Answers 2

Remove the Thread.Sleep calls. All of them. You don't need them. If two comboboxes have that value it's a whole second you're sleeping for no reason. ASP.NET event handlers run on the server side, which means the user is going to be waiting longer than he needs to, to get a response back from your code.

Now for your handlers you'll have to be careful about the Page Life Cycle sequence of events; I don't do ASP.NET very often (read: never) but I think you'll want to register your event handlers in the Page_Load handler. You'll want to register a handler per "section", not per combobox.

I'd make a method that takes a ComboBox instance and an IEnumerable<ComboBox>, and compares the values of the ComboBox with that of each item in the IEnumerable<ComboBox> (just realized that's the C# notation for generics.. make that (Of ComboBox)) that is not the same reference than ComboBox, and clear selection if the values match.

Private Sub ClearDuplicateSelections(comboBox As ComboBox, comboBoxes As IEnumerable(Of ComboBox))
    For Each cb In comboBoxes
        If Not cb.Equals(comboBox) Then 'there is a better way to do this but I don't VB much
            If cb.SelectedIndex = comboBox.SelectedIndex Then cb.ClearSelection()
        End If
    Next
End Sub

Now all you need is to pass the method a combobox (the "sender") and all the comboboxes in the same "group" or "section". In C# I'd do it like this:

// cast the sender from Object to ComboBox:
var box = (ComboBox)sender; 

// put all comboboxes of that section in an array:
var boxes = new[] { ComBox_GER1, ComBox_GER2, ComBox_GER3 }; 

ClearDuplicateSelections(box, boxes);

Side note, be careful with naming: ValueSelected is lying - it represents the selected index, not the selected value. I almost got caught.

share|improve this answer
add comment

It has been a long time since I've done VB, but here goes:

If you put the comboboxes in an array, you can use 1 handler for all

Protected Sub ComBox_SelectedIndexChanged(sender As Object, e As EventArgs) Handles ComBox_GER1.SelectedIndexChanged ComBox_GER2.SelectedIndexChanged ComBox_GER3.SelectedIndexChanged
    Dim cb = CType(sender, ComboBox) 'cast to ComboBox
    Dim ValueSelected = cb.SelectedIndex

    For Each combo As ComboBox In comboboxes
        If Not combo Is cb And combo.SelectedIndex = ValueSelected Then
            System.Threading.Thread.Sleep(500)
            combo.ClearSelection()
        End If
    Next
End Sub
share|improve this answer
    
I did not declare comboboxes. Make an array, put each ComboBox you want to handle in there and you can then loop through them. I don't know about the ClearSelection –  Dennis_E May 20 at 14:37
    
you didn't indent any of the code inside your Sub and you should, I am going to edit this in because it is standard formatting for VB. and +1 –  Malachi May 20 at 16:34
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.