tags:

views:

1504

answers:

9

After two years of C#, my VB.net is a bit rusty. I have two List. Let's call it originalList and targetList. Here is my C# code.

for(int i = 0; i<originalList.Count; i++)
{
    bool isMatch = false;
    foreach (string t in targetList)
    {
     if(String.Compare(originalList[i], t, true) == 0)
     {
      isMatch = true;
      break;
     }
    }
    if(isMatch)
    {
     originalList.RemoveAt(i);
     i--;
    }
}

and my VB.net code is this

dim i as integer
for i = 0 To originalList.Count - 1
    dim isMatch as boolean = false
    for each t as string in targetList
     if String.compare(originalList(i), t, true) = 0 then
      isMatch = true
      Exit for
     end if
    next

    if isMatch then
     originalList.RemoveAt(i)
     i -= 1
    end if
next

but i got index out of range error with my vb.net code. where did i get it wrong?

thank

A: 

It looks to me like it is a problem with how you are indexing into your strings, not with your for loops. Use a debugger to go through line by line and you will find the problem. I don't want to spare you the effort, as it will help you in the future. :)

skb
A: 

If I'm following your loop correctly, and your first item in the original list is a match, then you'll remove the string from the original list at index 0 and then decrement it to -1, which isn't a valid index value when you go to check the next item.

It looks to me that you'll need to check your index count and if you go below 0, reset it to 0 be able to start at the beginning again.

Dillie-O
Actually that part works just fine. i gets set to -1, then the loop goes to the next iteration (which increments it, now setting it to 0)
Yuliy
+1  A: 

I'd turn it into a while loop to make the condition more clear:

dim i as integer = 0
While i < originalList.Count
    dim isMatch as boolean = false
    for each t as string in targetList
        if String.compare(originalList(i), t, true) = 0 then
                isMatch = true
                Exit for
        end if
    next

    if isMatch then
        originalList.RemoveAt(i)
    else
        i += 1
    end if
next
Michael Haren
Yuliy covers why your version is failing quite nicely. I also agree that copying elements may be easier and clearer.
Michael Haren
Thank. that work. it's hard to go back to VB.net after using C# for two years. sigh...
Jack
I know what you mean. The more I use C#, the less I like using VB.
Michael Haren
+8  A: 

The problem is how the end condition is evaluated. In C#, each time through the loop, originalList.Count is checked In the VB.NET version, the end step of the loop is calculated once, at the entry into the loop.

From MSDN: Number of Iterations. Visual Basic evaluates the iteration values start, end, and step only once, before the loop begins. If your statement block changes end or step, these changes do not affect the iteration of the loop.

That said, the approach being taken is not efficient, as it is presumably using an array-backed list, which would involve lots of moving of data around. It would probably be quicker to simply copy elements to a new list if they don't match target list (doing nothing if they do), and then setting originalList = newList when you're done.

Yuliy
Plus one for finding the reference and suggesting a good alternative.
Michael Haren
Awesome answer. Surely the source of many conversion bugs. Bottom line, don't mutate the list while you iterate it!! That's just nasty.
TheSoftwareJedi
Or just iterate backwards ;)
Cecil Has a Name
+17  A: 

Consider this - it's a far more elegant way of achieving what you're trying to do - and that is remove items from your original list that appear in your target list. Consider the following lists:

Dim OriginalList As New List(Of String)(New String() {"a", "b", "c", "d"})
Dim TargetList As New List(Of String)(New String() {"a", "b", "c"})

And here's how I'd remove all the items from the original that appear in the target...

OriginalList.RemoveAll(Function(OriginalItem) TargetList.Contains(OriginalItem))

Which in C# would be written:

OriginalList.RemoveAll(OriginalItem => TargetList.Contains(OriginalItem));

The less code you use to achieve a task, the less chance that coding bugs could be introduced.

Side Note: This is very similar to an algorithm to test subsets. If you want to find out if set A is a subset of B, then you can iterate through B removing any corresponding items from A, once you've finished iterating through B, if there's any items left in A, then it wasn't a subset of B, if there are no items left, then it was a subset of B.

BenAlabaster
A++++. Would comment again
TheSoftwareJedi
A: 

It's hard to go back to VB.net after two years of C#. Funny thing is i starting to get rusty with C# as well as i dig into VB.net. It wasn't my choice to use VB.net since i advocated for C#. I go cry a little on the inside now.

Jack
+4  A: 
Dim i as integer = 0
Dim t as String

For I = originalList.Count to 1 Step -1
    for each t in targetList
       if String.compare(originalList(i), t, true) = 0 then 
          originalList.RemoveAt(I)
          exit for
       end if
    next t
Next I

The For .... Step -1 is the part you are probably not familiar with as is part of the Basic heritage of VB.NET.

When traversing a collection to REMOVE items by a numerical index you want to start at the end and work your way forward. This removes any issues with the count and items being removed.

The C# Example would been clearer if it looked like this

for(int i = originalList.Count; i<0; i--)
{
    foreach (string t in targetList)
    {
        if(String.Compare(originalList[i], t, true) == 0)
        {
                originalList.RemoveAt(i);
                break;
        }
    }
}
RS Conley
A: 

If you're going to remove items from an enumeration inline, then be sure to iterate it backwards.

Reddog
A: 

-------dim i as integer -------for i = 0 To originalList.Count - 1 ------- dim isMatch as boolean = false ------- for each t as string in targetList ------- if String.compare(originalList(i), t, true) = 0 then ------- isMatch = true ------- Exit for ------- end if

------- next

------- if isMatch then ------- originalList.RemoveAt(i) ------- i -= 1 ------- end if -------next

for i = 0 To originalList.Count - 1

it takes upto originalList.Count and not originalList.Count-1 as u wanted

vinod