Modifying list inside list in C#

Question

I am trying to modify the value inside the inner list by iterating the list. However, i keep getting the same outcome for the inner list irrespective of the outer list. I expect the outcome of the maxSpeed is different for different vehicle.

Hope someone could help on this.

Note that this is not a random number generator problem. This is just a sample code that I produce and this issue do exist in my project code without the use of random number generator.

Run C# code here

List<Vehicle> mainList = new List<Vehicle>();
List<Properties> defaultPropertyList = new List<Properties>{
    new Properties() { maxSpeed = 0, isTwoDoor = true },
    new Properties() { maxSpeed = 0, isTwoDoor = true },
};


mainList.Add(
    new Vehicle() {
        number = 1,
        property = new List<Properties>(defaultPropertyList)
    }
);
mainList.Add(
    new Vehicle() {
        number = 2,
        property = new List<Properties>(defaultPropertyList)
    }
);           

foreach(Vehicle vehicle in mainList) {
    Random rnd = new Random();
    vehicle.property.ForEach(x => x.maxSpeed =  rnd.Next(1, 100));
}

Try to seed the Random number generator to get better results — Gururaj, May 10 '17 at 12:57
If you `Properties` is a class then it will be passed by reference. This way, both `Vehicle` objects will get a unique list. But each unique list will have references to the **same** `Properties` objects. Either copy your objects or use value types (like a struct). — Adwaenyth, May 10 '17 at 12:59
@Gururaj i don't think it is an issue of the random number generator based on the result. — vincentsty, May 10 '17 at 12:59
Possible duplicate of [Random number generator only generating one random number](http://stackoverflow.com/questions/767999/random-number-generator-only-generating-one-random-number) — John H, May 10 '17 at 12:59
That's a really bad way to use `Random` - I suggest reading this: http://csharpindepth.com/Articles/Chapter12/Random.aspx — DigiFriend, May 10 '17 at 13:00
@vincentsty, I do think it has to do with the random number generator. See John H's link. — adv12, May 10 '17 at 13:00

Yaakov Ellis · Answer 1 · 2017-05-10T13:19:22.623

5

The problem is that when you initialize the property field for each vehicle, you are adding references to the same two Properties objects (defined at the top). So they exist in all vehicles, and when you modify one, it is modified in both places (since it is the same reference).

You will want to instead make copies of the objects in the defaultPropertyList when you initialize a new vehicle. Then they will exist independently.

One approach: Make a new constructor for Vehicle that takes in the default properties, and copy them there.

public Vehicle(List<Properties> defaultProps) 
{
  property = new List<Properties>();
  foreach (var p in defaultProps)
  {
    var newProp = new Properties {
                                    maxSpeed = p.maxSpeed,
                                    isTwoDoor = p.isTwoDoor
                                    // add any other properties here
                                 };
    property.Add(newProp);
  }
}

edited May 10 '17 at 13:19

answered May 10 '17 at 12:59

Yaakov Ellis

40,752
27
129
174

A bit confusing for me ... can you provide a sample code of implementation. I got method must have a return type with your code snipper – vincentsty May 10 '17 at 13:14
@vincentsty I added some more detail to the snippet – Yaakov Ellis May 10 '17 at 13:19
I got an error saying method is not defined. http://rextester.com/IUR51211 – vincentsty May 10 '17 at 13:24
@vincentsty you need to move the `Vehicle` constructor inside the `Vehicle` class. Right now it is above the class so the compiler thinks that it is a method and wants a return type. – Yaakov Ellis May 10 '17 at 13:26
The speed still remains the same for both vehicle. http://rextester.com/ZROR44026 – vincentsty May 10 '17 at 13:48

Scott Hannen · Answer 2 · 2017-05-10T13:21:52.333

defaultPropertyList is just one list containing one set of items. Each Vehicle has the same instance of List<Properties>. There's not one per Vehicle. There's just one, and they're all sharing it. That's why no matter how you change the properties, they all have the same properties.

To fix it, don't create one and share it. Just create as many as you need. Maybe this isn't you, but when I started off I was afraid of creating lots of objects and thought I could optimize by creating as few as possible.

While it's true that we don't want to unnecessarily create lots of expensive objects, there's no need to be stingy about it. Create as many as you need. For example, in a web application it's impossible to even keep track of how many objects get created just to respond to a single request.

You could just do this:

Random rnd = new Random(); // Despite what I just said,
                           // you only need one of these this time.
foreach(Vehicle vehicle in mainList) {        
    vehicle.property = new List<Properties>{
        new Properties() { maxSpeed = rnd.Next(1, 100), isTwoDoor = true },
        new Properties() { maxSpeed = rnd.Next(1, 100), isTwoDoor = true },
}

It would be easier if `vehicle.property.ForEach()` could be retain in this case. Only the maxSpeed need to be changed (and do mind that there are many property to remain the same value as well in actual case) and it seems to me that it is redundant to redeclare each and every property again in such way. — vincentsty, May 10 '17 at 14:04

Modifying list inside list in C#

2 Answers2