Using the ArrayList in Java

I have made an ArrayList of type CartItem, basically it stores all the CartItems in one list.

Now once a CartItem has already been added, instead of adding the same CartItem again, it will increase its quantity by one, rather than adding the element again in the list.

public ArrayList<CartItem> Items = new ArrayList<CartItem>();
public void AddItem(int productId)
{

    CartItem newItem = new CartItem(productId);

    if (Items.equals(newItem))
    {
        System.out.println("equal");
        for (CartItem item:Items)
        {
            if (item.Equals(newItem))
            {

                item.Quantity++;
                return;
            }
       }
   }
    else
   {
        newItem.Quantity = 1;
        Items.add(newItem);
   }
}

the cartItem constructor is as follows

public CartItem(int productId) {

    this.ProductId = productId;
    this.prod = new Product(productId);
    this.Quantity =1;
}

Instead of showing, Books , 3 its showing Bo开发者_如何学Coks 1, Books 1, Books 1

CartItem.Equals(Object) Function

public boolean Equals(CartItem item)
    {
      if(item.prod.Id == this.prod.Id)
      {
           return true;
      }
      return false;
     }

---

Without the code there is very little that we can do. But here are a few pointers:

1. Override equals() and hashcode() in your CartItem class.
2. Use generic ArrayList<CartItem> instead of the raw type.

A possible version of your required equals() methods might be:

public boolean equals(Object obj)
    {
        if (obj == null)
        {
            return false;
        }
        if (getClass() != obj.getClass())
        {
            return false;
        }
        final CartItem other = (CartItem) obj;

        if (this.prodID != other.prodID)
        {
            return false;
        }
        return true;
    }

---

Look what you compare:

ArrayList<CartItem> Items = new ArrayList<CartItem>();
CartItem newItem = new CartItem(productId);
if (Items.equals(newItem))

Items (ArrayList object) is never equal newItem (CartItem object)

edited: What you should do is:

1. Change Items.equals(newItem) to Items.contains(newItem)
2. Your equals in CartItem is incorrect, change it to something like that:

I've kept your naming but it isn't correct :)

public boolean equal(CartItem item) {
    if (!(item instanceof CarItem)) {
        return false;
    }

    return item.itm.Id == itm.Id;
}

---

your class ShoppingCart should be like this:

   public ShoppingCart{

        ArrayList<CartItem> Items = new ArrayList<CartItem>();

        public void AddItem(int productId)
        {
        CartItem newItem = new CartItem(productId);


            for (CartItem item:Items)
            {
                  if (item.Equal(newItem))
                  {
                    item.Quantity++;
                    return;
            }
          }


            newItem.Quantity = 1;
            Items.add(newItem);

        }
  }

---

  ArrayList<CartItem> Items = new ArrayList<CartItem>();
if (Items.equals(newItem))

You are comparing newItem with your List? I suspect his will always fail as I do not think newItem is the same as an empty ArrayList, however your code snippet looks incomplete, so I cannot be 100%.

EDIT:

CartItem.Equal looks wrong:

  public boolean Equal(CartItem item)
    {

        final CartItem a = new CartItem(item.Id);
          if (this.itm.Id != a.ProductId)
          {
              return false;
          }
      return true;
    }

Why construct a new object here? This method should be comparing the parameter item with the current CartItem object (this). Eg:

     public boolean Equal(CartItem item){
          return item.getId() == this.getId();//Implement a getId() method
    }

I would re-visit your CartItem class - why is there a quantity field? Is this class actually supposed to represent a collection of cart items?

Also, why have you not overridden the Object.equals(Object o) method?

---

You are calling:

if (Items.equals(newItem))

ie. .equals() on a List of CartItems.

I am guessing that will never evaluate to true for you, hence why you are never seeing an old item being updated.

You should really re-thingk - Implement a proper, java .eqauls() method on CartItem - you can then use List.contains(CartItem) to check if an item exists.

A better solution is to store your items in a HashMap (keyed by Item.id) to make look-up easier.

---

Your equals method on CartItem is wrong.

EDIT:

I can see you have changed its name to Equal. But your Equal method takes a int but when you use it you pass a CartItem to it. I would not think this compiles.

---

I would suggest a change to your approach.

Forget the CartItem, and just match up your Product to an Integer (quantity) via a Map

You shouldn't need productId either, as that SHOULD be a member of Product.

class Cart {
   private Map<Product,Integer> cart;

   public Cart() {
       cart = new HashMap<Product,Integer>();
   }

   public int getQuantity(Product p) {
       if(cart.contains(p)) return cart.get(p);
       return 0; // not in the cart
   }

   public void add(Product p) {
      add(p,0);
   }

   public void add(Product p, int q) {
      if(q < 0) throw new IllegalArgumentException("Cannot add negative amounts");
      cart.put(p,getQuantity(p) + q);
   }

   // removes are left as exercise for the reader

}

---

Instead of checking for Items.equals(newItem) you need to check for Items.contains(newItem) and of course override equals in CartItem to check product id. However what your solution is doing is iterating over the List of CartItems once in the contains method and again to find the actual CartItem. This should raise alarm bells about your choice of data structure.

What you are trying to do is match product ids to their quantity, this suggests that a Map might be a better data structure. Using a Map would give you an implementation a bit more like:

public Map<Integer, Integer> Items = new HashMap<Integer, Integer>();
public void AddItem(int productId)
{
    if (Items.containsKey(productId))
    {
        int count = Items.get(productId);
        Items.put(productId, count+1);
   }
    else
   {
        Items.put(productId, 1);
   }
}

---

ArrayList uses CartItem.equals() to determine match or not, so you could check CartItem.equals() implementation. BTW, it's nothing with hashcode().

EDIT: Equals is not equals, parameter is also incorrect. Use @Override on equals() to ensure correctness.