Why is NullReferenceException is thrown when I'm trying to remove an item from my cart?

Question

I'm writing my first .NET Core Web Application using MVC. It's a store so naturally it has a cart, I can add items to my cart (these items are called albums in my code because I'll be selling music albums). Whenever I try to use my RemoveFromCart function I get NullReferenceException error thrown. This is my function. These are my models for Album and Cart

public class Album
{
    [Key]
    public int Id { get; set; }
    public int Price { get; set; }
    public string Description { get; set; }
    public string AlbumName { get; set; }
    public DateOnly ReleaseDate { get; set; }
    public string ImageUrl { get; set; }
    public Genre Genre { get; set; }
    public ICollection<Artist_Album> Artists_Album { get; }
    public virtual ICollection<Cart> Carts { get; set; }

}

 public class Cart
 {
     [Key]
     public string CartId { get; set; }
     public virtual ICollection<Album> Albums { get; set; }
     public int Count { get; set; }
     public int TotalPrice { get; set; } 
     public System.DateTime DateCreated { get; set; }
 }

//REMOVE ALBUM FROM CART
public async Task<IActionResult> RemoveFromCart(int id)
{
    var album = await _context.Albums.SingleOrDefaultAsync(a => a.Id == id);
    var cartId = GetCartId();
    var cartItem = await _context.Carts
        .Include(c => c.Albums)
        .SingleOrDefaultAsync(c => c.CartId == cartId);

    if (cartItem == null)
    {
        return NotFound();
    }
    else
    {
        if (cartItem.Albums.Any(a => a.Id == id))
        {
            cartItem.Albums.Remove(album);
            cartItem.Count--;

            // Optionally, remove the entire cart item if count reaches zero
            if (cartItem.Count <= 0)
            {
                _context.Carts.Remove(cartItem);
            }
        }
    }
    await _context.SaveChangesAsync();
    return RedirectToAction("Index");
}

And this is how I'm displaying it on my Razor page in my Cart Edit view:

@foreach (var album in Model.Albums)
{
    <tr>
        <td>@album.AlbumName</td>
        <td>
            <form asp-action="RemoveFromCart" asp-controller="Carts" method="post" onsubmit="return confirm('Are you sure you want to remove this album from the cart?');">
                <input type="hidden" name="albumId" value="@album.Id" />
                <button type="submit" class="btn btn-link">Remove</button>
            </form>
        </td>
    </tr>
}

Full exception:
System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=MusicStore
  StackTrace:
   at AspNetCoreGeneratedDocument.Views_Carts_Edit.<<ExecuteAsync>b__23_0>d.MoveNext() in D:\MusicStore\Views\Carts\Edit.cshtml:line 29

I'm new to .NET and C# so if there's anything I forgot to provide let me know.

I do know that you can't remove an item from a list while iterating through said list and I think that's the problem here but I can't think of any way to get the result I want.

Answer 1

I can't see for sure, but I have some ideas.

First look here:

cartItem.Albums.Any(a => a.Id == id)

The code has already checked that cartItem has a value, but has not yet ensured the cartItem has any Albums. It's possible this is handled by the type definition (it's how I'd do it), but we can't see that in the question, so it's worth mentioning.

Second, look here:

cartItem.Albums.Remove(album);

It's possible at this point to have a null album. We don't know what type you're using for Albums; if it's a List<T>, the argument to Remove() is allowed to be null. But if it's something else, this could cause a problem.

Also, if it is a List<T> I'd remove the Count field from the type used for CartItem. Instead of maintaining this yourself, let the List for the .Albums property do this work for you. If you really want, you could have a get property (no set) like this:

public int Count {get { return this.Albums.Count; } }

Either way, it should be possible to get this down to a single context expression, which will be much less code and waaaay more efficient. I can't write the code for you (I'm not an EF expert: I'd do raw SQL), but I know enough to know what you have is three full round-trips to and from the DB instead of just one, which will always be much slower.

Finally, the context neither knows not cares what you do with cartItem after retrieving it. That is, SaveChangesAync() won't be enough here. This only saves changes for things you do when acting on the context in the first place.