CODEWITHSUNDEEP
c#
mugstack
mugstack

Reputation: 19

Make method fewer lines of code

I have a method like below, is there any way to refactor, cleaner way, so that i can make it in fewer lines of code for e.g removal of if / for loops someting like that 

public void CheckProductExistThenAddToCart(CartItem item)
{
    if (CartItems.Count == 0) AddToCart(item);

    bool itemFound = false;
    foreach (var cartItem in CartItems)
    {
        if (cartItem.ProductId.Equals(item.ProductId))
        {
            itemFound = true;
            cartItem.Qty += item.Qty;
            break;
        }
    }

    if (!itemFound)
    {
        AddToCart(item);
    }
}

Upvotes: 1

Views: 184

Answers (4)

Tim Schmelter
Tim Schmelter

Reputation: 460228

First, there is a bug because you are not returning after you have added the missing item. Hence you add Qty to the same item you have added one moment before, hence it's value is doubled.

So instead of:

public void CheckProductExistThenAddToCart(CartItem item)
{
    if (CartItems.Count == 0) AddToCart(item);
    // missing return

    bool itemFound = false;
    foreach (var cartItem in CartItems)
    {
        if (cartItem.ProductId.Equals(item.ProductId))
        {
            itemFound = true; // ypu will find the same item you have justb added
            // ... causes this bug and is less efficient
            cartItem.Qty += item.Qty;
            ...

i would do (also simplified with Linq):

public void CheckProductExistThenAddToCart(CartItem item)
{
    if (CartItems.Count == 0) 
    {
        AddToCart(item);
        return;
    }

    CartItem oldItem = CartItems.FirstOrDefault(ci => ci.ProductId == item.ProductId);
    if(oldItem == null)
        AddToCart(item);
    else
        oldItem.Qty += item.Qty;
}

Upvotes: 1

BartoszKP
BartoszKP

Reputation: 35901

You can use SingleOrDefault if having unique items (in the context of ProductId) should be assured. If it is possible to have more than one and you want to ignore this fact, then change to FirstOrDefault. I find Single better as it states intent explicitly here.

public void CheckProductExistThenAddToCart(CartItem item)
{
  var existingItem = CartItems
      .SingleOrDefault(i => i.ProductId.Equals(item.ProductId));

  if (existingItem == null)
  {
    AddToCart(item);
  }
  else
  {
    existingItem.Qty += item.Qty;
  }
}

Upvotes: 6

Jesko R.
Jesko R.

Reputation: 827

In order to shorten this function you can consider using a 

Dictionary<ProductId, CartItem> dict;

Then instead of looping through the cart just use 

if (dict.ContainsKey(productId))
{
    // add qty
} else {
    // add item to cart
}

Upvotes: 4

Reed Copsey
Reed Copsey

Reputation: 564661

You could use LINQ:

public void CheckProductExistThenAddToCart(CartItem item)
{
     var existingItem = CartItems.FirstOrDefault(ci => ci.ProductID == item.ProductId);
     if (existingItem == null)
          CartItems.Add(item);
     else
          existingItem.Qty += item.Qty;
}

Upvotes: 6

Related Questions