Most efficient way of finding an item at a certain index in a List<>, modifying the item and then updating the List<>?

Question

Take List<Car>. Each Car has a unique index identifying it, say RegNumber and then another property describing it - say Color for example.

I want to

1. check if the collection has a car with RegNumber 5
2. if it does, change the color
3. if it doesn't, add a new item for that car
4. save the list

This is the way I am currently doing it and I'm asking if there is a better, more efficient way of doing this?

Car car = CarsForSale.Find(c => c.RegNumber == 5);

if (car != null)
{
   foreach (Car car in CarsForSale)
   {
      if (car.RegNumber == 5)
      {
         car.Color = "Red";
         break;
      }
   }
}
else
{
   CarsForSale.Add(new Car(5, "Red"));
}

Save(CarsForSale);

EDIT There are not multiple cars with the same reg - the RegNumber is unique as stated in the question.

This was really just a total dumb@ss moment here anyway that a code review would have spotted. Thanks for all the replies and for not mocking my clearly stoopid question. Of course the item/element returned from the collection is a reference so there is absolutely no need to iterate through the list again...time to go bang my head against a wall I think.

Answer 1

So as Dan already mentioned, if you have a unique property you should use it as a key within a Dictionary<TKey, TValue>.

Cause checking if something is within a Dictionary is an O(1) operation, while within a List it is just O(n) in the worst case (and now imagine you have 1 million cars within your list).

var carsForSale = new Dictionary<int, Car>();

//Create a car which you like to check
var checkCar = new Car(4, Color.Red);

//Use this approach if you want to change only a few properties
//of an existing item
if (carsForSale.ContainsKey(checkCar.RegNum))
{
    carsForSale[checkCar.RegNum].Color = checkCar.Color;
}
else
{
    carsForSale[4] = checkCar;
}

//If you have to take over ALL property settings, you can also
//forget the old item and take the new one.
//The index operator is smart enough to just add a new one
//or to delete an old and add the new in one step.
carsForSale[checkCar.RegNum] = checkCar;

Dummy implementation of the car class:

public class Car
{
    public int RegNum { get; private set; }
    public Color Color { get; set; }

    public Car(int regNum)
        : this(regNum, Color.Empty)
    { }

    public Car(int regNum, Color color)
    {
        RegNum = regNum;
        Color = color;
    }
}

The problem why you are using a Dictionary is, cause you want to explicitly tell what the key is (the RegNum property of your car), but you could also use a Hashset<T> if your Car object would correctly implement Equals() and GetHashCode() but this is a little more complex than you might think. A good explanation can be found in the Essentials C# book.

Answer 2

Once you have the car you're looking for from the LINQ statement, there's no need to loop back through the collection to find the match:

Car car = CarsForSale.Where(c => c.RegNumber == 5).FirstOrDefault();

if(car != null)
{
    car.Color = "Red";
}
else
{
    CarsForSale.Add(new Car(5, "Red"));
}

Save(CarsForSale);

Or if there are going to be multiple Cars with the same RegNumber:

var cars = CarsForSale.Where(c => c.RegNumber == 5);

if(cars.Any())
{
    foreach(Car car in cars)
        car.Color = "Red";
}
else
{
    CarsForSale.Add(new Car(5, "Red"));
}

Save(CarsForSale);

Answer 3

Well first off you don't need your test for car.RegNumber == 5 in the loop - you've already found the first car that match this criterion from your statement:

Car car = CarsForSale.Find(c => c.RegNumber == 5);

In fact your whole loop is redundant, you can just have:

if (car != null)
{
    car.Color = "Red";
}
else
{
    CarsForSale.Add(new Car(5, "Red"));
}

Unless you want to find all cars that have RegNumber equal to 5, in which case your first line is incorrect as that will only find the first car that matches the criteria. To find all the cars you want something along these lines:

var cars = CarsForSale.Where(c => c.RegNumber == 5);

foreach (Car car in cars)
{
    car.Color = "Red";
}

if (!car.Any())
{
    CarsForSale.Add(new Car(5, "Red"));
}

With your original code the compiler should have warned you that the redefinition of car in the loop would hide the original definition (the one I've quoted).

Answer 4

Update

In response to this comment you've left on a couple of other answers:

What if there are several cars with the RegNumber of 5?

If it's possible for multiple cars to have the same RegNumber, then calling Find is not the right approach. Find is just enumerating over the list to find a match; you'd be better off skipping it and keeping your foreach loop.

You could, however, make your code more concise by using Where instead:

var matches = CarsForSale.Where(c => c.RegNumber == 5);
int numMatches = 0;

foreach (Car match in matches )
{
    match.Color = "Red";
    ++numMatches;
}
if (numMatches == 0)
{
   CarsForSale.Add(new Car(5, "Red"));
}

---

Original answer

That whole foreach loop is redundant: you're basically doing the same work you already did by calling Find.

So the code can be simplified:

Car car = CarsForSale.Find(c => c.RegNumber == 5);

if (car != null)
{
    car.Color = "Red";
}
else
{
   CarsForSale.Add(new Car(5, "Red"));
}

That said, if you're looking up cars in your List<Car> by RegNumber, it would make sense to use a Dictionary<int, Car> instead of a List<Car>:

Car car;
if (CarsForSale.TryGetValue(5, out car))
{
    car.Color = "Red";
}
else
{
    CarsForSale[5] = car = new Car(5, "Red");
}

Answer 5

Why are you re-iterating through the list when you already have a result?

This will achieve the same outcome:

Car car = CarsForSale.Find(c => c.RegNumber == 5);
if (car != null)
{
   car.Color = "Red";
}
else
{
   CarsForSale.Add(new Car(5, "Red"));
}
Save(CarsForSale);

The result from the Find method of CarsForSale, if it returns a result, will be a reference type, which means any changes to car will change the item in CarsForSale as well. I'm guessing you thought that the result from Find would be disconnected from the actual item in CarsForSale, hence the unnecessary foreach loop?