CODEWITHSUNDEEP
javaarraylistcollections
Newbie
Newbie

Reputation: 146

Deleting from Arraylist

I wanted to optimize the code a bit. All trying to do is remove the product from the array. When I call the method deleteProduct(prod.getId()), it should delete product that I added first.

I can use the for loop, then how would delete the product in the array. Any pointers, please?

public void deleteProduct(int productId) throws ProductNotFoundException {

    Iterator<Product> it = allProducts.iterator();
    Product p= null;
    int pid = productId;
    int i = 0;

    if (!allProducts.isEmpty()) {
        while(it.hasNext()){
            p= it.next();
            i= allProducts.indexOf(p);
            if (p.getId().equals(productId)){
                i= allProducts.indexOf(p);
                allProducts.remove(i);
                System.out.println("Successfully removed the product " + pid);
                return;
            }
        }
    }   
        throw new ProductNotFoundException ("No Such Product");
}

Upvotes: 0

Views: 149

Answers (4)

Mohammed Falha
Mohammed Falha

Reputation: 967

Why you are using loop in the first place just override the .equal method in the product and let you method deleteProduct method take a product not id as parameter then just call allProduct.remove(Product p);

try this 

package test;

public class Product {


    int id;

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + id;
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        Product other = (Product) obj;
        if (id != other.id)
            return false;
        return true;
    }


}

and this

package test;

import java.awt.List;
import java.util.ArrayList;

public class RemoveProduct {

    /**
     * @param args
     */
    java.util.List<Product> productList=new ArrayList<>();
    public static void main(String[] args) {
        // TODO Auto-generated method stub

    }


    public void deleteProduct(Product p)
    {

        productList.remove(p);
    }

}

Upvotes: 1

Andrew Lazarus
Andrew Lazarus

Reputation: 19320

You don't want to use allProducts.remove() because it will invalidate your iterator. You want to call it.remove(), which is guaranteed to leave the iterator valid. There is no need for any indexOf. You need to review what an iterator is doing: it is giving you access to the elements. You don't have to go back and fetch them with indexOf.

And, you don't need allProducts.isEmpty(), because it is redundant with the hasNext text in the while loop. If allProducts is indeed empty, the while condition will be false right at the beginning of the first iteration, and will be skipped.

Upvotes: 1

Luiggi Mendoza
Luiggi Mendoza

Reputation: 85779

You can just use the iterator to remove the item by calling Iterator#remove:

while(it.hasNext()){
    p = it.next();
    if (p.getId().equals(productId)) {
         it.remove();
         System.out.println("Successfully removed the product " + pid);
         return;
    }
}
throw new ProductNotFoundException ("No Such Product");

From comment, using a for loop for iterator:

for(Iterator<Product> it = allProducts.iterator(); it.haNext(); ) {
    p = it.next();
    if (p.getId().equals(productId)) {
         it.remove();
         System.out.println("Successfully removed the product " + pid);
         return;
    }
}
throw new ProductNotFoundException ("No Such Product");

Probably you're asking how can I do this in a enhanced for loop? Answer is, you can't. But since enhanced for uses an iterator behind the scenes, the while loop approach would fit to your needs.

Upvotes: 4

Kris
Kris

Reputation: 8868

I think you could change the List to a Map Implementation. If you use a Map, where product is mapped against a product id, you can directly remove the Map entry from Map just using product Id. Its more easy to retrieve also.

A map can perform far better than a loop. Only case is to take care for memory usage.

Upvotes: 1

Related Questions