-3

This is a very straightforward task, but I feel I'm overlooking something. I have multiple objects that I'm trying to add to an ArrayList, and each of them has an identifying name in the form of a String. I need to be able to find (interact) with the objects in the ArrayList by calling the string name. So I tried this:

In my item class I have: private String itemName;

public Item(String name)
{
    itemName = name;
}

So I can give it a name to be used by the user.

Then in my class that interacts with the object, I create an ArrayList:

private ArrayList<Item> items = new ArrayList<Item>();

I add an object to the arrayList first by it's actual object name, but I need to be able to interact with it using it's String name, so I tried this:

public void removeItem(String itemName)
{
    for (int i = 0; i < items.size(); i++)
    {
        if (items.get(i).toString() == itemName)
        {
            items.remove(i);
        }
        break;
    }

}

But it's not removing the item. If all of this is confusing, in essence I'm trying to create an OBJECT that I can give a STRING name (like I did with the item above), then have the ability to add the OBJECT to an ArrayList, and then finally be able to remove, or get, or do something with the OBJECTS in the ArrayList by calling the STRING name. I know I need to iterate through the ArrayList, but I can't actually get the object.

Thanks for any help.

Jordan Plahn
  • 375
  • 4
  • 12

5 Answers5

4

You are dong three mistakes here:

  • You are using items.get(i).toString() which will not give you itemName for your Item. It will just give you a string representation of your Item class, returned by Object class's toString method, if you don't override one. However, this might work, if you have overriden a toString method, and returned the itemName from that. But, that I don't see. And even if you have overriden that, I suggest you to have getter and setter for your itemName field, and use that to return the itemName.

  • You are comparing strings using == operator, which will not give you correct result. You should always compare the string using equals method.

So, your if statement should look like: 

 if (items.get(i).getName().equals(itemName))
  • 3rd problem is, you are trying to modify the List that you are iterating upon. This will not work out, and may throw ConcurrentModificationException. You should use Iterator to remove elements from the List while iterating.

See for more details about those two problems, and how to solve them:

Further, you can consider overriding equals method in your class, and then you can directly compare your instances using equals method.

Now, having pointed out the some logical problems with your code, it's time to point out some design problems.

Given your requirement, it seems like you need to use a HashMap, rather than a List of some custom type storing your attribute. You can create a map like this:

Map<String, Integer> map = new HashMap<String, Integer>();

which will contain the mapping of itemName to respective Item, and then getting the Item for a particular itemName is as simple as map.get(itemName).

Community
  • 1
  • 1
Rohit Jain
  • 209,639
  • 45
  • 409
  • 525
3

It sounds like you should be using a Map for this, for instance java.util.HashMap<String, Item>. The Map interface provides exactly those operations you're looking for, and it is also iterable.

Gustav Barkefors
  • 5,016
  • 26
  • 30
1

add a getter to your object to get the name, like so:

public class Item {
   private final String name; //once given cannot change
   public Item(String name) {
      this.name = name; //yhis.name to distinguish between 2 variabled both called "name"
   }
   public String getName() {
      return name; //this.name not required as no other variable called "name" is in scope
   }
}

then you could find your Item like this:

for (Item item : theList) {
   if (item.getName.equals(requiredName)) {
      //got you!
   }
}

generally speaking, dont ever compare strings with ==. also, if you want to remove an item from a list youre iterating over you have to use the (older) iterator syntax:

Iterator<Item> iter = theList.iterator();
while (iter.hasNext()) {
   Item item = iter.next();
   if (item.getName.equals(requiredName)) {
      //got you!
      iter.remove();
      break; //no need to go over the rest of the list
   }
}

and lastly, if all you want is to look up items by their name a list is not your best collection since finding the item may require traversing the entire list. maps (hashmap specifically) will give you much better performance for this type of operation. you could use the name as the key

radai
  • 23,949
  • 10
  • 71
  • 115
0

my guess is that you items.get(i).toString() does not do what you think it does. Why don't you use some thing like items.get(i).name or create getters or setters for name in Item object and retrieve name by items.get(i).getName()

mike.tihonchik
  • 6,855
  • 3
  • 32
  • 47
0

There is this - the way you implemented the removeItem, you can also do it directly using ArrayList.remove(item.itemName)- that's just before you get all stuck up with ConcurrentModificationException and reimplementing stuff that already exists - look at the library!! Read the documentation of ArrayList!

For clarification: in Java (and mostly really only in Java): == means comparison of references.

So:

String a = "A";
String b = new StringBuilder("A").toString();
if (a == b) // --> false
if (a.equals(b)) // --> true

You could also consider using org.apache.commons.lang.StringUtils.equals for that - which is safe regarding null pointers.

As others already pointed out - the toString-method will only work correctly if you implement it correctly (returning the name in your case). By original toString returns a class-name together with an ID. That's probably not what you want (simply try to print it out).

michael_s
  • 2,515
  • 18
  • 24