CODEWITHSUNDEEP
vb.netlistloopsforeach
gromit1
gromit1

Reputation: 587

Removing Items in a List While Iterating Through It with For Each Loop

I have a list named NeededList I need to check each item in this list to see if it exists in my database. If it does exist in the database I need to remove it from the list. But I can't change the list while I'm iterating through it. How can I make this work?

Here is my code so far:

For Each Needed In NeededList
        Dim Ticker = Needed.Split("-")(0).Trim()
        Dim Year = Needed.Split("-")(1).Trim()
        Dim Period = Needed.Split("-")(2).Trim()
        Dim Table = Needed.Split("-")(3).Trim()
        Dim dr As OleDbDataReader
        Dim cmd2 As New OleDb.OleDbCommand("SELECT * FROM " & Table & " WHERE Ticker = ? AND [Year] = ? AND Period = ?", con)
        cmd2.Parameters.AddWithValue("?", Ticker)
        cmd2.Parameters.AddWithValue("?", Year)
        cmd2.Parameters.AddWithValue("?", Period)
        dr = cmd2.ExecuteReader
        If dr.HasRows Then
            NeededList.Remove(Needed)
        End If
Next

Upvotes: 21

Views: 46375

Answers (8)

Nolios
Nolios

Reputation: 392

Lists grow and shrink from their end, you can solve the problem by iterating over the list in reverse order or looping a copy of the list.

Ideally For ... Step -1 should be used if you are concerned with performance. On the other hand, Reverse() or ToList() is more concise and readable if performance is not a concern.

Implementation:

For Each Needed In NeededList.Reverse()
    '...
    If dr.HasRows Then
        NeededList.Remove(Needed)
    End If
Next

Benchmark Results:

ReverseToList:   00:00:00.0005484
CopyWithToList:  00:00:00.0017638
CopyWithForeach: 00:00:00.0141009

Benchmark Code:

Imports System
Imports System.Linq
Imports System.Collections.Generic
Imports System.Diagnostics

Public Class Program
    Public Shared Sub Main()
        Dim items As New List(Of Integer)()
        
        'Initialise test data
        items = Enumerable.Range(0, 1000000).ToList()

        'Test the methods
        Reverse(items)
        CopyWithToList(items)
        CopyWithForeach(items)
    End Sub

    Public Shared Sub Reverse(Of T)(list As List(Of T))
        Dim sw = Stopwatch.StartNew()
        list.Reverse()
        sw.Stop()
        Console.WriteLine("ReversedList:    {0}", sw.Elapsed)
    End Sub

    Public Shared Sub CopyWithToList(Of T)(list As List(Of T))
        Dim sw = Stopwatch.StartNew()
        Dim copy As List(Of T) = list.ToList()
        sw.Stop()
        Console.WriteLine("CopyWithToList:  {0}", sw.Elapsed)
    End Sub

    Public Shared Sub CopyWithForeach(Of T)(list As List(Of T))
        Dim sw = Stopwatch.StartNew()
        Dim copy As List(Of T) = New List(Of T)()
        For Each item As T In list
            copy.Add(item)
        Next
        sw.Stop()

        Console.WriteLine("CopyWithForeach: {0}", sw.Elapsed)
    End Sub

End Class

Upvotes: 2

Steve
Steve

Reputation: 216293

No you can't do that using For Each. The reason is explained in this answer where the author tries to pinpoint the logical problems and the practical inefficiencies that such implementation could trigger.

But you can do that using the old fashioned for .. loop.
The trick is to start from the end and looping backwards.

For x = NeededList.Count - 1 to 0 Step -1
    ' Get the element to evaluate....
    Dim Needed = NeededList(x)
    .....
    If dr.HasRows Then
        NeededList.RemoveAt(x)
    End If
Next

You need to approach the loop in this way because you don't want to skip an element because the current one has just been deleted.

For example, in a forward loop, suppose that you remove the fourth element in the collection, after that, the fifth element slips down to the fourth position. But then the indexer goes up to 5. In this way, the previous fifth element (now in the fourth position) is never evaluated by the logic inside the loop.
Of course you could try to change the value of the indexer but this ends always in bad code and bugs waiting to happen.

Upvotes: 42

Pec1983
Pec1983

Reputation: 376

No you can not remove from a List that you are working on e.g.

For Each Str As String In listOfStrings 
                    If Str.Equals("Pat") Then
                        Dim index = listOfStrings.IndexOf(Str)
                        listOfStrings .RemoveAt(index)
                    End If
                Next

But this way will work make a copy of your list and delete from it e.g.

For Each Str As String In listOfStrings 
                    If Str.Equals("Pat") Then
                        Dim index = listOfStringsCopy.IndexOf(Str)
                        listOfStringsCopy.RemoveAt(index)
                    End If
                Next

Upvotes: 0

C66
C66

Reputation: 1

How about this (no iteration needed):

NeededList = (NeededList.Where(Function(Needed) IsNeeded(Needed)).ToList

Function IsNeeded(Needed As ...) As Boolean
    ...
    Return Not dr.HasRows
End Function

Upvotes: 0

mikro
mikro

Reputation: 525

You can also invert the order of the list's elements and still use For Each using the IEnumerable Cast and Reverse extensions.

Simple example using a List(Of String):

For Each Needed In NeededList.Cast(Of List(Of String)).Reverse()
    If dr.HasRows Then
        NeededList.Remove(Needed)
    End If
Next

Upvotes: 0

Henk Holterman
Henk Holterman

Reputation: 273264

Go for safe and make a copy with ToList():

For Each Needed In NeededList.ToList()
    Dim Ticker = Needed.Split("-")(0).Trim()
    ...
    If dr.HasRows Then
        NeededList.Remove(Needed)
    End If
Next

Upvotes: 21

tezzo
tezzo

Reputation: 11105

You can use a For loop iterating through every index with Step -1.

For i as Integer = NeededList.Count - 1 to 0 Step -1

    Dim Needed = NeededList(i)

    'this is a copy of your code
    Dim Ticker = Needed.Split("-")(0).Trim()
    Dim Year = Needed.Split("-")(1).Trim()
    Dim Period = Needed.Split("-")(2).Trim()
    Dim Table = Needed.Split("-")(3).Trim()

    Dim dr As OleDbDataReader
    Dim cmd2 As New OleDb.OleDbCommand("SELECT * FROM " & Table & " WHERE Ticker = ? AND [Year] = ? AND Period = ?", con)
    cmd2.Parameters.AddWithValue("?", Ticker)
    cmd2.Parameters.AddWithValue("?", Year)
    cmd2.Parameters.AddWithValue("?", Period)
    dr = cmd2.ExecuteReader

    'MODIFIED CODE
    If dr.HasRows Then NeededList.RemoveAt(i)

Next i

Upvotes: 4

nhgrif
nhgrif

Reputation: 62062

The contents of an array (or anything else you can fast enumerate with For Each can not be modified with a For Each loop. You need to use a simple For loop and iterate through every index.

Hint: Because you'll be deleting indexes, I suggest starting at the last index and work your way toward the first index so you don't skip over one every time you delete one.

Upvotes: 3

Related Questions