VB.NET 2008 Application crashing during Do Loop

Question

I am writing an application to compare each item on listbox1 to all items on listbox2. If the item is found, then remove it from both lists. The goal is to only have the items that were not found remain on both lists.

The problem is, the application just hangs and I never get any results. I looked at my code several times and I cannot figure out what's going on (programming noob I know...).

Can anybody help me with this?

Code Snippet:

    Private Sub Button2_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button2.Click

    Dim a As String
    Dim b As String
    Dim y As String

    For i As Integer = 0 To ListBox1.Items.Count - 1
        a = ListBox1.Items(i)
        y = 1
        Do While y = 1
            For x As Integer = 0 To ListBox2.Items.Count - 1
                b = ListBox2.Items(x)
                Dim res As Int16 = String.Compare(a, b)
                If res = 0 Then
                    y = 0
                    ListBox2.Items.Remove(i)
                    ListBox2.Items.Remove(x)
                ElseIf x = ListBox1.Items.Count Then
                    Exit Do
                End If
            Next
        Loop
    Next
End Sub

Answer 1

if ListBox1.Items.Count is more that ListBox2.Items.Count - 1, X will never equal ListBox1.Items.Count, so the Exit Do will never run, and the code will just loop endlessly in the

Do While y = 1

Have you considered using Linq for example, for easier list management?

edit: Additionally it's wrong to delete an item from the list you are traversing with a for (it's downright illegal to do that with a For Each) because each deletion will offset the loop counter.

edit2: here's a Linq snippet that accomplishes the task:

    Dim itemsFirst = (From item As String In ListBox1.Items Select item)
    Dim itemsSecond = (From item As String In ListBox2.Items Select item)

    Dim dupes = System.Linq.Enumerable.Intersect(itemsFirst, itemsSecond).ToList
    For Each item In dupes
        ListBox1.Items.Remove(item)
        ListBox2.Items.Remove(item)
    Next item

what is does is basically extract the strings from both list (this is necessary because the ListBox.Items collection is a little weird)

After that we run the intersect method and copy the results into a list. (the .ToList part) The copying is a required part because, otherwise dupes would just be a subset of the Items of the ListBox, and once again we would be trying to lift ourselves by pulling on our shoestrings.

The last part is just a simple delete loop, that removes the items from the collection

Answer 2

you have

ElseIf x = ListBox1.Items.Count Then
    Exit Do

when it should be

ElseIf x = ListBox1.Items.Count - 1 Then
    Exit Do

because your for loop will change X to count, and then exit without iterating at that value.

Not only that, but why is there a Do loop anyway? There's no need to keep iterating the same inner listbox looking for duplicates, is there?

And thirdly, you shouldn't remove things while you iterate through them. In your case the for loops are reusing count, so it's "safe" but the remove operation will reindex things, so you should subtract 1 from your i and x iterators when you remove, so that the next one isn't skipped by the reindexing.

On second thought, maybe you put that Do loop in there to cover the elements skipped the previous time around, as mentioned in my third point.

Answer 3

If you're using visual studio 2008 or later:

Dim dupes = Listbox1.Items.Cast(Of String)().Intersect(Listbox2.Items.Cast(Of String)()).ToArray() 
For Each item As String in dupes
    Listbox1.Items.Remove(item)
    Listbox2.Items.Remove(item)
Next item

Answer 4

I ran a test of three different methods. They are Joels, sweko, and mine. I was doing this to test performance, but I found out the results are not the same, the listboxes aren't the same. Here is the code I used to test, so you can be the judge. Probably some dumb mistake on my part.

Dim stpw As New Stopwatch

Private Sub Button1_Click(ByVal sender As System.Object, _
                          ByVal e As System.EventArgs) Handles Button1.Click
    Debug.WriteLine("")
    loadLBTD() ''#load test data

    doSTPW("Test 1 Start", False) ''#mine
    ''#get rid of dupes <<<<<<<<<<<<<<
    Dim dupeL As New List(Of String)
    For x As Integer = ListBox1.Items.Count - 1 To 0 Step -1
        If ListBox2.Items.Contains(ListBox1.Items(x)) Then
            dupeL.Add(ListBox1.Items(x))
            ListBox1.Items.RemoveAt(x)
        End If
    Next
    For Each s As String In dupeL
        ListBox2.Items.Remove(s)
    Next
    doSTPW("Test 1 End")

    loadLBTD() ''#load test data

    doSTPW("Test 2 Start", False) ''#sweko
    ''#get rid of dupes <<<<<<<<<<<<<<
    ''#I had to set Option Strict to Off to get this to work <<<<<<<
    Dim itemsFirst = (From item As String In ListBox1.Items Select item)
    Dim itemsSecond = (From item As String In ListBox2.Items Select item)

    Dim dupes = System.Linq.Enumerable.Intersect(itemsFirst, itemsSecond).ToList
    For Each item In dupes
        ListBox1.Items.Remove(item)
        ListBox2.Items.Remove(item)
    Next item
    doSTPW("Test 2 End")

    loadLBTD() ''#load test data

    doSTPW("Test 3 Start", False) ''#joel
    ''#get rid of dupes <<<<<<<<<<<<<<
    Dim dupes2 = ListBox1.Items.Cast(Of String)().Intersect(ListBox2.Items.Cast(Of String)()).ToArray()
    For Each item As String In dupes2
        ListBox1.Items.Remove(item)
        ListBox2.Items.Remove(item)
    Next item
    doSTPW("Test 3 End")
End Sub

Private Sub doSTPW(ByVal someText As String, Optional ByVal showTM As Boolean = True)
    stpw.Stop() ''#stop the clock
    If flip Then Debug.Write("'T ") Else Debug.Write("'F ")
    Debug.Write("LBCnts " & ListBox1.Items.Count & " " & ListBox2.Items.Count)
    Dim s As String
    If showTM Then
        s = String.Format("  {0}  {1}", someText, stpw.ElapsedTicks.ToString("N0"))
    Else
        s = String.Format("  {0}", someText)
    End If
    Debug.WriteLine(s)
    stpw.Reset() ''#reset and start clock
    stpw.Start()
End Sub

Dim flip As Boolean = False
Private Sub loadLBTD()
    ''#Create test data 
    Dim tl1() As String = New String() {"A", "X", "y", "z", "B", "w", "X", "y", "z"}
    Dim tl2() As String = New String() {"A", "y", "z", "Q", "A", "y", "z", "Q", "A", "y", "z", "Q"}
    ListBox1.Items.Clear()
    ListBox2.Items.Clear()
    ''#load listboxes
    If flip Then
        ListBox1.Items.AddRange(tl2)
        ListBox2.Items.AddRange(tl1)
    Else
        ListBox1.Items.AddRange(tl1)
        ListBox2.Items.AddRange(tl2)
    End If
    ''#end of test data setup
End Sub

Also, as expected, LINQ is more concise but slower. If the code is used infrequently it doesn't matter. I had a bad experience with LINQ and a Sieve of Eratosthenes.