recursive loop is updating all properties in generic list

Question

I have the following method it creates a generic list recursively. I'm getting some interesting results. The property CurrentAllocation is always overwritten with the last value.

Here is the line in question.

courierTypeRegion.CurrentAllocation = remaining;
courierTypeRegionOutput.Add(courierTypeRegion);

Here is the whole method

public static List<CourierTypeRegion> FindClosest2(decimal quantity, decimal remaining, ICollection<CourierTypeRegion> courierTypeRegions, List<CourierTypeRegion> courierTypeRegionOutput)
        {
            var processed = false;
            var courierOrderByDesc = courierTypeRegions.OrderByDescending(x => x.CourierType.PalletsPerTrailer).ToList();
            var courierCount = courierOrderByDesc.Count();
            var courierCurrent = 0;
            foreach (var courierTypeRegion in courierOrderByDesc)
            {

                if (remaining >= courierTypeRegion.CourierType.PalletsPerTrailer && !processed)
                {
                    courierTypeRegion.CurrentAllocation = courierTypeRegion.CourierType.PalletsPerTrailer;
                    courierTypeRegionOutput.Add(courierTypeRegion);
                    processed = true;
                }

                if (!processed)
                {
                    if (courierOrderByDesc[courierCurrent + 1] != null)
                    {
                        if (remaining > courierOrderByDesc[courierCurrent + 1].CourierType.PalletsPerTrailer)
                        {
                            courierTypeRegion.CurrentAllocation = remaining;
                            courierTypeRegionOutput.Add(courierTypeRegion);
                            processed = true;
                        }
                    }
                }
                courierCurrent++;
            }

            if (!processed)
            {
                if (courierTypeRegions.Count > 0)
                {
                    var courierTypeRegionRemaining =
                        courierTypeRegions.Where(x => x.CourierType.PalletsPerTrailer >= remaining).OrderByDescending(
                            x => x.CourierType.PalletsPerTrailer).SingleOrDefault();
                    if (courierTypeRegionRemaining != null) courierTypeRegionOutput.Add(courierTypeRegionRemaining);
                    processed = true;
                }
            }


            var currentRemaining = quantity - courierTypeRegionOutput.Sum(x => x.CourierType.PalletsPerTrailer);

            if (currentRemaining > 0)
            {
                FindClosest(quantity, currentRemaining, courierTypeRegions, courierTypeRegionOutput);
            }

            return courierTypeRegionOutput;
        }

Ben Robinson · Accepted Answer · 2011-06-07T10:30:26.403

1

'CourierTypeRegion' is one instance that lives for the entire foreach loop, it is not instantiated and destroyed every loop iteration. You are repeatedly adding the same instance to your list. You end up with a list where all items reference the the last value in the loop.

You need to change your foreach loop as follows:

foreach (var courierTypeRegion in courierOrderByDesc)
            {
var courierRegionCopy = courierTypeRegion;    
                if (remaining >= courierTypeRegion.CourierType.PalletsPerTrailer && !processed)
                {
                    courierRegionCopy.CurrentAllocation = courierTypeRegion.CourierType.PalletsPerTrailer;
                    courierTypeRegionOutput.Add(courierRegionCopy);
                    processed = true;
                }

                if (!processed)
                {
                    if (courierOrderByDesc[courierCurrent + 1] != null)
                    {
                        if (remaining > courierOrderByDesc[courierCurrent + 1].CourierType.PalletsPerTrailer)
                        {
                            courierRegionCopy.CurrentAllocation = remaining;
                            courierTypeRegionOutput.Add(courierRegionCopy);
                            processed = true;
                        }
                    }
                }
                courierCurrent++;
            }

edited Jun 07 '11 at 10:30

answered Jun 07 '11 at 10:23

Ben Robinson

21,601
5
62
79

1

I don't understand what you mean. `courierTypeRegion` is the loop variable in the foreach loop, it will point to each element in `courierOrderByDesc`. Your `courierRegionCopy` is just another reference to the `courierTypeRegion`. – Anders Abel Jun 07 '11 at 10:34
The loop variable courierTypeRegion is created at the start of the loop and destroyed at the end of it. Each iteration it is updated to reference a new item in the collection being looped through. If you this to a list you are adding the same thing to the list. Each time the loop variable is updated, you update the entire list as all items are a copy of the same loop variable. By creating a new variable in the loop and adding that to the list the list items will reference a copy of the loop variable as it was at that point in the loop. – Ben Robinson Jun 07 '11 at 11:01
For people who don't understand my answer (i.e. whoever voted me down) see this SO post http://stackoverflow.com/questions/512166/c-the-foreach-identifier-and-closures and google for "closing over the loop variable" – Ben Robinson Jun 07 '11 at 11:06
a closed-over loop var requires a lambda or anon method... I don't see one here. – H H Jun 08 '11 at 17:13

recursive loop is updating all properties in generic list

1 Answers1