Can't remove item from List

Question

I try to remove item from products in DeleteProduct method and Remove method returns true but all items still in list and I can't figure out why.

As I understend products list creates at the first start of the application and at static constructor it fills with items. Then in DeleteProduct method one of the items removes and executes Products Method.

Please someone explane it!

public class ProductController
    : Controller
{
    private static IEnumerable<ProductViewModel> products = null;

    static ProductController()
    {
        products = new List<ProductViewModel>
        {
            new ProductViewModel{Id = 0, Description = "Milk", Price = 21.0, IsAvailable = true, LastUpdate = DateTime.Now},
            new ProductViewModel{Id = 1, Description = "Bread", Price = 10.10, IsAvailable = false, LastUpdate = DateTime.Now},
            new ProductViewModel{Id = 2, Description = "Soure creame", Price = 34.5, IsAvailable = true, LastUpdate = DateTime.Now},
            new ProductViewModel{Id = 3, Description = "Chocolate", Price = 31.0, IsAvailable = true, LastUpdate = DateTime.Now},
            new ProductViewModel{Id = 4, Description = "Apples", Price = 1.0, IsAvailable = true, LastUpdate = DateTime.Now},
        };
    }
    public ActionResult Products()
    {
        return View(products);
    }

    public ActionResult DeleteProduct(int id)
    {
        var product = products.FirstOrDefault(p => p.Id == id);

        if (product != null)
            products.ToList().Remove(product);

        return RedirectToAction("Products");
    }
}

What you doing is terrible practice (do not use `static` variables), but it would work if you first tested if `products` is `null`, and only then assign the list to it. — , Dec 02 '17 at 11:28
[ASP.NET MVC How safe are static variables](https://stackoverflow.com/questions/14225113/asp-net-mvc-how-safe-are-static-variables) — , Dec 02 '17 at 11:33
Suggest you also read [ASP.NET MVC controllers static methods](https://stackoverflow.com/questions/6126957/asp-net-mvc-controllers-static-methods) — , Dec 02 '17 at 11:35
@StephenMuecke I definitely will not store data in static variables and also use static variables. Just wanted to create test example — Ilia Mikhailov, Dec 02 '17 at 13:19

Markus · Accepted Answer · 2017-12-02T12:05:58.700

2

The problem is in that line:

products.ToList().Remove(product);

By calling ToList() you generate a new list and remove the product from the newly created list. In order to make the code work, change the products field to type List:

private static List<ProductViewModel> products = null;

This way, you can use the methods of a list and do not have to cast or use ToList(). You can call Remove directly on the products field:

products.Remove(product);

edited Dec 02 '17 at 12:05

answered Dec 02 '17 at 11:31

Markus

20,838
4
31
55

@mjwills thanks for the hint. I'll update the answer. – Markus Dec 02 '17 at 12:05
Thank you! I forgot that ToList generates new list – Ilia Mikhailov Dec 02 '17 at 14:00

score 1 · Answer 2 · answered Dec 02 '17 at 11:30

When you do products.ToList() you are creating a new list. So, when you do products.ToList().Remove(product), you are removing the product from this new list.

You could do something like:

if (product != null)
{
    List<ProductViewModel> productList = products.ToList();
    productList.Remove(product);
    products = productList;
}

Or, you could change so the static variable products is of type List<ProductViewModel>, so you wouldn't have to create a new instance.

score 0 · Answer 3 · answered Dec 02 '17 at 11:46

Consider replacing:

var product = products.FirstOrDefault(p => p.Id == id);

if (product != null)
    products.ToList().Remove(product);

with:

products.Remove(product);

and replacing:

private static IEnumerable<ProductViewModel> products = null;

with:

private static List<ProductViewModel> products = null;

The ToList is unnecessary since it creates a new List and them removes the product from that new List (which is basically pointless).

The FirstOrDefault is unnecessary since Remove will take care of the only remove this item if it is already there problem for you. The code as is basically checks twice whether the item is there before removing it.

The IEnumerable has no benefits over leaving it as its 'raw' type (List), and using IEnumerable removes the ability to call Remove.

Also consider using ConcurrentBag rather than List - for thread safety.

Can't remove item from List

3 Answers3