0

I have a foreach loop that cycles through each List from a master list List> and if a list contains an item, I want to remove it from the master list. My code works but even though the master list has fewer items, it still runs through each loop like it is reading from the original master list. I'm not sure how to properly remove items from the master list so that the loop behaves accordingly and only continues to execute for the remaining items in the correct master list.

public void GetBestCombo(List<List<SomeClass>> combosList)
    {
        foreach (var combo in combosList.ToList())
        {
            if (combo.ElementAt(0).ReturnAmount < 0)
            {
                combosList.RemoveAll(j => j.Contains(combo.ElementAt(0)));
            }
        }
    }

This code works fine without any errors and my combosList goes from 16 items to 1 item but keeps on running through each iteration in the loop. I also remove everything and it still keeps running.

Here is some input and ouput information:

List> combosList contains 16 elements/lists

After RemoveAll code is run, combosList contains 1 element/list

Foreach loop keeps cycling through each item in the original combosList (16 items) when it should instead break the foreach loop

DarthVegan
  • 1,719
  • 7
  • 25
  • 42
  • `ToList` basically creates a shallow copy of the list, so the one you're calling `RemoveAll` on is different to the one you're using in `foreach`. – Kirk Larkin Nov 18 '17 at 21:45
  • @serpent5 How do I fix this because if I didn't have the combosList.ToList() there it gave me an exception that I couldn't edit the list while it was enumerating through the collection? – DarthVegan Nov 18 '17 at 21:47
  • @serpent5 This isn't a duplicate because I'm not having trouble deleting items but having issues with exiting the loop properly – DarthVegan Nov 18 '17 at 21:53
  • You added a call to `ToList` to avoid a `foreach` exception - That original exception is the real problem you want to address. Eric Lippert's answer in the marked duplicate gives multiple suggestions, at least one of which will work for you. – Kirk Larkin Nov 18 '17 at 21:58
  • @serpent5 His first suggestion in his post is to use RemoveAll which is best and that is exactly what I'm doing in my code. I also can't wait for the foreach loop to finish and then do a second loop because the reason why I'm trying to delete items from the loop is to narrow down the amount of items that the loop has to go through – DarthVegan Nov 18 '17 at 22:02
  • Can you please restate your problem? It's not clear what you want. Your code works fine for me. Please provide some sample input, actual output, and expected output. – Rufus L Nov 18 '17 at 22:02
  • @RufusL I stated my problem below my code but basically I'm able to remove items from the list that the foreach loop is cycling through but after the items are removed, the foreach loop keeps going even though it should stop – DarthVegan Nov 18 '17 at 22:04
  • If you can restate your problem more clearly, provide some sample input / output, perhaps use a native type instead of `SomeClass`, it could help get this repoened – Rufus L Nov 18 '17 at 22:04
  • What do you mean "the foreach loop keeps going"? You get an infinite loop? I am testing your code and it works fine, so you need to provide more data. – Rufus L Nov 18 '17 at 22:06
  • @RufusL I added more information to explain better. Hopefully this helps – DarthVegan Nov 18 '17 at 22:10
  • @serpent5 This question is doing exactly what Eric Lippert suggested in his first answer. Taking the original list and calling `ToList()` on it. Also, the code works fine. How is this a duplicate? – Rufus L Nov 18 '17 at 22:10
  • @Evk This question is doing exactly what Eric Lippert suggested in his first answer. Taking the original list and calling `ToList()` on it. Also, the code works fine. How is this a duplicate? – Rufus L Nov 18 '17 at 22:10
  • @user3610374 Your statement does not make sense to me. You say *"Foreach loop keeps cycling through each item in the original combosList"*. This is true. It's exactly what you're telling it to do. You've provided a **copy** of the original list and are iterating over it, and you're removing items from the original list (but not the copy). Then you say *"it should instead break the foreach"*. What do you mean? *What* should break the foreach? There is no call to `break;` if an item is found. What are you expecting would trigger a break? – Rufus L Nov 18 '17 at 22:15
  • @RufusL That is exactly my point. I want to remove items from the same list that the foreach loop is executing from so that I can both remove items and stop executing the loop when it runs out of elements. I don't know how else to explain my issue. I don't know of any other way to do this other than calling ToList() so I don't get the foreach exception – DarthVegan Nov 18 '17 at 22:20
  • I see, well now you've described it more clearly. The compiler already told you that you can't do that with a `foreach`, and you've worked around that issue in a way that iterates over every item. I'm not sure there is a good way to call `RemoveAll` on a list that you're iterating over. If you were just removing one item, you could walk the list backwards in a `for` loop, but that touches each item. Alternatively, you could just get rid of the `foreach` and do it in one line: `combosList.RemoveAll(combo => combo.ElementAt(0).ReturnAmount < 0);` – Rufus L Nov 18 '17 at 22:32
  • @RufusL I will explain why I need to do it the way I'm trying to do it currently. The master list in this case is a full list of combinations that are possible with a total of 21 inputs but only choosing 15 of them at a time. Each input is a list of doubles and I'm running calculations on each input and if the input is bad then I need to remove that input from all combinations that contains it at the same time. My example with the pseudo code isn't the greatest because normally I have a 100,000 combinations usually which would take forever if I did them all one at a time – DarthVegan Nov 19 '17 at 00:53

0 Answers0