HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Three combo boxes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
threeboxescombo

Problem

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

Solution

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, and compares the values of the ComboBox with that of each item in the IEnumerable (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.

Code Snippets

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

Context

StackExchange Code Review Q#51216, answer score: 5

Revisions (0)

No revisions yet.