CODEWITHSUNDEEP
c#asp.net-mvc
Ilia Mikhailov
Ilia Mikhailov

Reputation: 35

Can't remove item from List

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");
    }
}

Upvotes: 1

Views: 295

Answers (3)

Markus
Markus

Reputation: 22456

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);

Upvotes: 2

mjwills
mjwills

Reputation: 23898

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.

Upvotes: 0

&#193;ngela
&#193;ngela

Reputation: 1425

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.

Upvotes: 1

Related Questions