Why do I need to perform a deep clone to get this code to work?

Question

The following code works. However, it works because I end up creating a deep clone of the suppliers. If I do not perform a deep clone then we get an error suggesting that the supplier objects have changed and the attempt to amend the supplier table has failed. This only happens if the following line is run: foreach (Supplier suppliers in exceptions). Oddly, this occurs irrespective whether the Delete() method is executed. Why does this happen? I have posted the working code below for your inspection. As I say, if you try looping without deep cloning then it does not work... Any ideas?

public void DeleteSuppliers(IList<Supplier> suppliers, Int32 parentID)
       {
           // If a supplier has been deleted on the form we need to delete from the database.
           // Get the suppliers from the database
       List<Supplier> dbSuppliers = Supplier.FindAllByParentID(parentID);

       // So return any suppliers that are in the database that are not now on this form
       IEnumerable<Supplier> results = dbSuppliers.Where(f => !Suppliers.Any(d => d.Id == f.Id));

       IList<Supplier> exceptions = null;

       // code guard
       if (results != null)
       {
           // cast as a list
           IList<Supplier> tempList = (IList<Supplier>)results.ToList();

           // deep clone otherwise there would be an error
           exceptions = (IList<Supplier>)ObjectHelper.DeepClone(tempList);

           // explicit clean up
           tempList = null;
       }

       // Delete the exceptions from the database
       if (exceptions != null)
       {
           // walk the suppliers that were deleted from the form
           foreach (Supplier suppliers in exceptions)
           {
               // delete the supplier from the database
               suppliers.Delete();
           }
       }
   }

BTW, I would like to point out an unrelated pointless piece of code here that I see frequently in code reviews involving LINQ for some reason. Your test for "results != null" is pointless. That will never be false. It will always refer to an IEnumerable instance. There may be 0 objects in the collection, but the collection instance will always exist. — BlueMonkMN, Mar 27 '11 at 12:04
so I should just use results.Count > 0 or something instead? — Phil C, Mar 27 '11 at 12:15

score 1 · Answer 1 · answered Mar 27 '11 at 12:11

1

I think the error is about the collection being enumerated having changed. You're not allowed to change the collection being enumerated by a foreach statement (or anything that enumerated an IEnumerable, if I recall correctly).

But if you make a clone then the collection you're enumerating is separate from the collection being affected by the Delete.

Have you tried a shallow copy? I would think that would work just as well. A shallow copy could be created with ToArray.

answered Mar 27 '11 at 12:11

BlueMonkMN

25,079
9
80
146

I thought that was the reason until I noticed that it did not even need to run Delete() to create the error. I did create a shallow copy to another IList but not to an array. How would that be different as a shallow copy would just contain a reference back to the original objects irrespective of its collection; or am I missing something? – Phil C Mar 27 '11 at 12:22
How did you create the shallow copy? With `ToList`? – CodesInChaos Mar 27 '11 at 12:45
You say that the problem occurs even if you don't call Delete? So you an just have an empty loop that enumerates each item in the collection without doing anything and you get the error? Then there must be something wrong in the enumerator. – BlueMonkMN Mar 28 '11 at 13:14
I resolved the issue by reordering the execution flow. Originally, this piece of code was execute last. The error went away when I executed it first. – Phil C Apr 09 '11 at 08:55
I have still upvoted your answer because what you said should have worked – Phil C Apr 09 '11 at 08:56

score 0 · Accepted Answer · answered Apr 09 '11 at 08:55

0

I resolved the issue by reordering the execution flow. Originally, this piece of code was execute last. The error went away when I executed it first.

answered Apr 09 '11 at 08:55

Phil C

3,687
4
29
51

Why do I need to perform a deep clone to get this code to work?

2 Answers2