JAVA update ArrayList object method not updating

Question

I have an interface test class that implements another super-class. In the test class I have a method that is supposed to update an object from an array list; first it's supposed to check if the objects is in the list, if it is, it will delete and add a new object (replace). If it cant find the object it will throw an exception message. Here is the code I have implemented:

public class ProductDBImpl implements ProductDB {

// field declarations
ArrayList<Product> products = new ArrayList<Product>();
@Override
public void updateProduct(Product product) throws ProductNotFoundException 
{
    // TODO Auto-generated method stub
    Iterator<Product> pritr = products.iterator();
    while(pritr.hasNext())
    {
        Product pr = pritr.next();

        if (!pr.getId().equals(product.getId()))
        {

            throw new ProductNotFoundException("Product does no exist");

        }
        pritr.remove();

    }
    products.add(product);

}

First off I dont know if this is the correct way to do it. And, when I test it with my test client script, I get an error that reads:

Exception in thread "main" productdb.util.AssertionFailedError: should've gotten ProductNotFoundException

The code for the test client is as follows:

    ipod.setId(Integer.MAX_VALUE);
    try {
        productDB.updateProduct(ipod);
        Assert.fail("should've gotten ProductNotFoundException");
    } catch (ProductNotFoundException pnfe) {
        // expecting this
    }

Please help me identify my error. Thanks.

LATEST EDIT I updated my code based on the feedback I was getting RE the first item throwing the error:

public void updateProduct(Product product) throws ProductNotFoundException 
    {
        // TODO Auto-generated method stub
        Iterator<Product> pritr = products.iterator();
        while(pritr.hasNext())
        {
            Product pr = pritr.next();
            System.out.println(pr.getId());
            System.out.println(product.getId());
            if (pr.getId().equals(product.getId()))
            {

                pritr.remove();

            }
            else
            {
                throw new ProductNotFoundException("Product Not Found");
            }

        }
        products.add(product);

    }

ANOTHER UPDATE

public void updateProduct(Product product) throws ProductNotFoundException 
{
    // TODO Auto-generated method stub
    Iterator<Product> pritr = products.iterator();
    boolean match = true;
    while(pritr.hasNext())
    {
        Product pr = pritr.next();
        if (pr.getId().equals(product.getId()))
        {

            pritr.remove();

        }
        else
        {
            match = false;
        }

    }
    if (match == false)
    {
        new ProductNotFoundException("Product not found");
    }
    else
    {
        products.add(product);
    }


}

You're throwing the exception on the first step of the iteration. What do you think that will happen when the desired element is the second or the last one in the list? — Luiggi Mendoza
– Luiggi Mendoza, Commented Aug 21, 2015 at 22:18
Just as @LuiggiMendoza mentions -- you need to walk through your code mentally or on paper as your error should seem obvious then. — Hovercraft Full Of Eels
– Hovercraft Full Of Eels, Commented Aug 21, 2015 at 22:19
So do I have to iterate through the whole list first, then put the exception outside of the while loop? Sorry if questions seem dumb, but I guess i'm still confused and trying to understand. — CircAnalyzer
– CircAnalyzer, Commented Aug 21, 2015 at 22:26
@DeeTee - Dude. I don't know why you are ignoring our advice. But trust me, I've been programming for 40+ years, and teaching people to program for many of those years. If you don't understand what your code says, you >>need<< to hand execute it. We are not hostile ... just impatient with someone who is ignoring what we are saying. — Stephen C
– Stephen C, Commented Aug 21, 2015 at 23:25
"How do you know I havent already tried that?" - We don't, but if you had done it properly, then you would have sorted out your problem. Because that >>is<< a solution. Again, the "root problem" here is that the code doesn't do what you think it does. — Stephen C
– Stephen C, Commented Aug 21, 2015 at 23:30

Hovercraft Full Of Eels · Accepted Answer · 2015-08-22 00:36:35Z

2

Again, walk through your logic. You have this:

public void updateProduct(Product product) throws ProductNotFoundException 
{
    // TODO Auto-generated method stub
    Iterator<Product> pritr = products.iterator();
    boolean match = true;
    while(pritr.hasNext())
    {
        Product pr = pritr.next();
        if (pr.getId().equals(product.getId()))
        {

            pritr.remove();

        }
        else
        {
            match = false;
        }

    }
    if (match == false)
    {
        new ProductNotFoundException("Product not found");
    }
    else
    {
        products.add(product);
    }


}

which translates to:

start method
    set match to true
    while loop
        if product found remove original product
        else match is set to false  // this will always happen one or more times!
    end while loop

    // match is almost guaranteed to be false!
    check match. if false, throw exception
    else if true, add new product
end method

Now assume that the data is:

No match  
No match  
No match  
match  
No match  
No match

And see what happens. With your logic the boolean will end up false when it shouldn't be.

Compare this with data that should throw the exception:

No match  
No match  
No match  
No match  
No match  
No match

And again see what happens.

What you want:

start method
    set match to false
    while loop
        if product found 
             remove original product
             replace with new product
             set match to true
        // no else block needed
    end while loop

    check match. if false, throw exception
    else if true, add new product
end method

or

start method
    // no need for match variable
    while loop
        if product found 
             remove original product
             replace with new product
             set match to true
             return from method // *****
    end while loop

    // if we reach this line, a match was never found
    throw exception

end method

edited Aug 22, 2015 at 0:36

answered Aug 22, 2015 at 0:01

Hovercraft Full Of Eels

286k25 gold badges268 silver badges391 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

CircAnalyzer Over a year ago

In the if product found block, I use remove() then add() to 'replace'. Is this correct?

Hovercraft Full Of Eels Over a year ago

@dee try it! Also check changes to the answer. Again my goal is to get you to be able to walk through your code in your mind. Nothing is more important than this.

CircAnalyzer Over a year ago

Maybe you should stop attempting to help people with poor solutions. Apparently all the things you suggested haven't worked.

Andrew Thompson Over a year ago

@DeeTee Help is what you make of it. If you cannot 'pick up the ball & run with it', please do not blame the person/people who went to efforts to assist.

CircAnalyzer Over a year ago

@Hovercraft Full Of Eels Thanks for all your help. Your solutions, especially the one that worked (the last one) were all correct. I was able to complete my assignment. Sorry for all the rude comments and thanks for your help.

|

Niklas P · Accepted Answer · 2015-08-21 22:33:42Z

2

The Assert.fail is executed when no Exception is thrown and this is the case if:

either the first (this seems to be an unwanted error!) element of the list (products) has an equal id
or the list (products) is empty

What's the content of the list?

answered Aug 21, 2015 at 22:33

Niklas P

3,5472 gold badges17 silver badges19 bronze badges

11 Comments

CircAnalyzer Over a year ago

The contents of the list are objects of type Product.

Stephen C Over a year ago

@DeeTee - That isn't what he asked. He wants to know >>what<< objects are in the list, bot their type.

Niklas P Over a year ago

Okay and what IDs do these products have? The result seems to show that one of these products does have the id Integer.MAX_VALUE.

CircAnalyzer Over a year ago

The IDs are product IDs; integers.

Niklas P Over a year ago

BTW: I do spend a lot of time with trying to understand what you do, you don't answer to my questions and now I am hostile?

|

Collectives™ on Stack Overflow

JAVA update ArrayList object method not updating

2 Answers 2

9 Comments

11 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

9 Comments

11 Comments

Your Answer

Sign up or log in

Post as a guest

Related