Search through arraylist, remove item from arraylist, add it elsewhere

Question

public void drop (String name) - if appropriate, remove the item from the ArrayList and add it to the current room. Update the game’s message with one of the following options: 1) the player is not holding that item, 2) the room already has an item, or 3) the player has successfully dropped the item in the room. This is the goal of this method but when I run it it always skipps to the currentMessage in the else statement.

PROBLEM: The problem I am hacing is that when I run this method and try to drop an Item in a room, it doesnt and skips to the else statement and resturns the message "you do not have that item", and I do not know why it is doing this and not working through the first if statement because I am typing an items name I know is in the arraylist.

public void drop(String name)
{      
    for(Item count : myArray){
        if(count.getName().contains(name) && currentRoom.hasItem() == false){
            currentRoom.addItem(count);
            currentMessage = "you have successfully dropped the item in the room";
            myArray.remove(count);
        }
        else if(count.getName().contains(name) && currentRoom.hasItem() == true)
        {
            currentMessage = "the room already has an item";
        }
        else 
        {
            currentMessage = "you do not have that item";
        }
    }
}

So what exactly is the problem?

Mureinik
– Mureinik

2014-11-28 17:56:45 +00:00
Commented Nov 28, 2014 at 17:56 — Mureinik
– Mureinik, Commented Nov 28, 2014 at 17:56

Chris Thompson · Accepted Answer · 2014-11-28 18:06:37Z

3

This is going to throw a ConcurrentModificationException because you cannot use a foreach loop while modifying the list. Instead iterators support the Iterator.remove() method that will allow you to remove an object from the underlying collection:

public void drop(String name)
{   
    Iterator<Item> it = myArray.iterator();
    Item count = it.next();
    while(count != null){
        if(count.getName().contains(name) && currentRoom.hasItem() == false){
            currentRoom.addItem(count);
            currentMessage = "you have successfully dropped the item in the room";
            it.remove();
        }
        else if(count.getName().contains(name) && currentRoom.hasItem() == true)
        {
            currentMessage = "the room already has an item";
        }
        else 
        {
            currentMessage = "you do not have that item";
        }
        count = it.next();
    }
}

answered Nov 28, 2014 at 18:06

Chris Thompson

35.6k12 gold badges86 silver badges110 bronze badges

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

3 Comments

msrd0 Over a year ago

This code could be simplified: while ((count = it.next()) != null)

msrd0 Over a year ago

@AlexWien But the code above could also be turned in a do-while so that you only need one count = ... statement instead of two - and where's the problem with my while loop? I'm sure it'll work ...

AlexWien Over a year ago

@msrd0 It is not one statement, in both solutions your improvement hint and his, the condirtion needs two statements. You just wrote it in one line. This does not save anything.

msrd0 · Accepted Answer · 2014-11-28 17:59:42Z

1

Your problem is that you are not allowed to edit the array while you are iterating through it. Change your for loop like this to get rid of the error. Also you are using the if loop wronly. Don't ask the complete condition to be false but only the one you want to be false with writing an ! before it.

public void drop(String name)
{      
    for (int i = 0; i < myArray.size(); i++) {
        Item count = myArray.get(i);
        if (count.getName().contains(name) && !currentRoom.hasItem()){
            currentRoom.addItem(count);
            currentMessage = "you have successfully dropped the item in the room";
            myArray.remove(count);
            i--; // element removed, so decrease count
        }
        else if(count.getName().contains(name) && currentRoom.hasItem() == true)
        {
            currentMessage = "the room already has an item";
        }
        else 
        {
            currentMessage = "you do not have that item";
        }
    }
}

answered Nov 28, 2014 at 17:59

msrd0

8,52510 gold badges63 silver badges95 bronze badges

5 Comments

msrd0 Over a year ago

@ChrisThompson The OP would get an java.util.ConcurrentModificationException for his code

msrd0 Over a year ago

@Christopher Then please accept my answer so that future visitors know that the question is solved

Gabriel Oliveira Over a year ago

Hey, sorry for the duplicate, I didn't see your answer before posting. Already removed it.

AlexWien Over a year ago

@msrd0 your answer is not the simplest approach.

Martin Over a year ago

In this answer and OP's original solution, any "currentMessage" being set will be reset in the last iteration by "you do not have that item", unless ofcourse the item you're looking for is the last one. Which seems a bit counter intuitive.

Martin · Accepted Answer · 2014-11-28 18:07:24Z

0

Try this;

public void drop(String name)
{
    for (Iterator<Item> it = col.iterator(); it.hasNext();)
    {
        Item count = it.next();
        if(count.getName().contains(name))
        {
            if(currentRoom.hasItem() == false)
            {
               currentRoom.addItem(count);
               currentMessage = "you have successfully dropped the item in the room";
               it.remove();
               return; //Once found return;
            }
            else
            {
               currentMessage = "the room already has an item";
               return; //Once found return or alternatively keep looking
            }
        }           
    }
    //Item never found
    currentMessage = "you do not have that item";       
}

In addition to the ConcurrentModificationException your code also had a logical flaw in it, it set the message after every iteration, whereas you probably wanted it to look through the entire list before setting currentMessage.

answered Nov 28, 2014 at 18:07

Martin

1,15010 silver badges14 bronze badges

2 Comments

msrd0 Over a year ago

The OP doesn't necessarily want to return then, a continue would be better

Martin Over a year ago

According to his question only the current room should be operated on, and so I assume the iteration over the list is only to "find" the current room, not to process all rooms.

Collectives™ on Stack Overflow

Search through arraylist, remove item from arraylist, add it elsewhere

3 Answers 3

3 Comments

5 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

5 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related