How can I replace object in java collection?

Question

I am trying to replace element in collection with new modified version. Below is short code that aims to demonstrate what I'd like to achieve.

The whole idea is that I have one object that consists of collections of other objects. At some point in time I am expecting that this objects in collections (in my example phones) might require some modifications and I'd like to modify the code in one place only.

I know that in order to update the object's attributes I can use setters while iterating through the collection as demonstrated below. But maybe there is better, more general way to achieve that.

public class Customer {
    private int id;
    private Collection<Phone> phoneCollection;
    public Customer() {
        phoneCollection = new ArrayList<>();
    }
//getters and setters    

}

and Phone class

public class Phone {
    private int id;
    private String number;
    private String name;
//getters and setters    
}

and

public static void main(String[] args) {
        Customer c = new Customer();

        c.addPhone(new Phone(1, "12345", "aaa"));
        c.addPhone(new Phone(2, "34567", "bbb"));
        System.out.println(c);

        Phone p = new Phone(2, "9999999", "new name");

        Collection<Phone> col = c.getPhoneCollection();
        for (Phone phone : col) {
            if (phone.getId() == p.getId()) {
//             This is working fine
//                phone.setNumber(p.getNumber());
//                phone.setName(p.getName());

//              But I'd like to replace whole object if possible and this is not working, at least not that way
                  phone = p;
            }
        }
        System.out.println(c);
    }
}

Is this possible to achieve what I want? I tried copy constructor idea and other methods I found searching the net but none of them was working like I would expect.

EDIT 1

After reading some comments I got an idea

I added the following method to my Phone class

public static void replace(Phone org, Phone dst){
    org.setName(dst.getName());
    org.setNumber(dst.getNumber());
}

and now my foreach part looks like that

    for (Phone phone : col) {
        if (phone.getId() == p.getId()) {
            Phone.replace(phone, p);
        }
    }

And it does the job. Now if I change the Phone class attributes I only need to change that method. Do you think it is OK solving the issue that way?

Solution 1

You should not modify the collection while you're iterating through it; that's likely to earn you a ConcurrentModificationException. You can scan the collection for the first object that matches your search criterion. Then you can exit the loop, remove the old object, and add the new one.

Collection<Phone> col = c.getPhoneCollection();
Phone original = null;
for (Phone phone : col) {
    if (phone.getId() == p.getId()) {
        original = phone;
        break;
    }
}
if (original != null) {
    Phone replacement = new Phone(original);
    replacement.setNumber(p.getNumber());
    replacement.setName(p.getName());
    col.remove(original);
    col.add(replacement);
}

Alternatively, you could declare a more specific type of collection, such as a List, that would allow you to work with indexes, which would make the replacement step much more efficient.

If your phone IDs are unique to each phone, you should consider using a Map<Integer, Phone> that maps each phone ID to the corresponding phone. (Alternatively, you could use some sort of third-party sparse array structure that doesn't involve boxing each ID into an Integer.) Of course, if your IDs aren't unique, then you might want to modify the above to gather a secondary collection of all matching phones (and reconsider the logic of your existing code as well).

Solution 2

You can also use a Set (HashSet), this is only when you don't want to do the way Mike suggested.

Use the Phone as an item in the set. Don't forget to implement hashCode() and equals() in Phone. hashCode() should return the id, as it is supposed to be unique.

Since you are concerned about replacing the item, here's how HashSet will help you :

1. Create an instance of your object.
2. Remove the object you want to replace from the set.
3. Add the new object (you created in step 1) back to the set.

Both these operations 2 & 3 are guaranteed in O(1) / constant time.

You don't need to maintain a map for this problem, that's redundant.

If you want to get the object from the collection itself and then modify it, then HashMap would be better, search is guaranteed in O(1) time.

Comments

[user949300]

Trouble is, he doest have a Phone to remove, just an ID of the phone.

[Zircon]

He's using an enhanced for loop, which gives him a Phone. It's also possible to use col.remove(index);

[user949300]

I love the idea, but why not simply phoneMap.put(p)?

[user949300]

But col.indexOf(theNewPhone) will always return -1. Unless he overrides equals for Phone.

[MeetTitan]

@Zircon, what happens if there are duplicates? It is a List after all.

[user949300]

Would be slightly cleaner IMHO if creating the replacement was done outside the loop in your 2nd if. Maybe renaming original to found?

[MeetTitan]

Would also be simpler using an iterator to delete inside the iteration whilst avoiding a ConcurrentModificationException.

[Andrew Tobilko]

you may earn UnsupportedOperationException because not all subclasses implement the remove method (which is default and throws this exception)

[Ted Hopp]

@user949300 - Agreed. I made that change.

[Ted Hopp]

@AndrewTobilko - True. But OP's code indicates that the collection is actually an ArrayList, which does support remove.

[Zircon]

He's iterating over the whole List, so they would all eventually be replaced anyway. In any case, my solution won't work because it causes a ConcurrentModificationException I think.

[Ted Hopp]

@MeetTitan - yes, that would be more efficient. It would need an explicit iterator rather than an enhanced for loop. Also, as a matter of personal style, I'd prefer to keep the remove/add pair together in the code. If efficiency is an issue, a Map is the way to go.

[norbi771]

I have lists (or collections) in my entities and would like to stick with that. I am looking for simplest repeatable method to update object composed of some basic attributes and many collections / lists. These lists are bound to JSF components.

[Ted Hopp]

That's what OP is already doing. The question is specifically about not mutating the existing object in the collection.

[Ted Hopp]

@Zircon - The Collection interface does not have a remove(int) method. That only exists for ordered collections (such as List).

[MeetTitan]

This would only help if he were trying to negate duplicates in his collection.

[Ted Hopp]

The phone map should be Map<Integer, Phone> rather than Map<String, Phone> since phone IDs are int values, not strings.

[norbi771]

Could you see my EDIT 1 and eventually comment on that please?

[MeetTitan]

@TedHopp, I made that edit for him. I made the assumption that the ID was a String based on stackoverflow.com/a/2853253/4307644, however scrolling up I can see I've made the decision in error. I've edited his answer to reflect this.

[Partha]

Well using a HashMap is essentially the same thing. No duplicates allowed.

[Ted Hopp]

@norbi771 - Re your edit 1: that will work perfectly well if you don't mind changing the original Phone object already in the collection. My understanding was that you wanted to avoid that. (Perhaps because the objects in the collection are also used elsewhere?) Of course, your original code is doing pretty much the same thing. (BTW, I'd rename the method from replace() to something closer to what's going on--perhaps set() or even setNameAndNumber().)

[MeetTitan]

But what advantages would a set have over a list in this case unless the goal was to eliminate duplicate elements?

[Partha]

In order to use a HashMap, a key-value pair has to be maintained. This mean's the Phone object is now exposed with having its Id/phone number as a Key and the object itself as a value. This problem doesn't need a key-value pair implementation, that's redundant and unnecessary. Instead, the developer just needs to find the object and replace it with a new/updated one. Since Set internally uses HashMap as a backing structure, this can be achieved in O(1) time.When using list, unless the specific index the object is present at is known beforehand(in this case it isn't), the cost of search is O(n)

[norbi771]

@Ted Hopp - thanks for your comment. I couldn't find good name but you're right I definitely will rename that method. What I wanted to express in my problem description is that what the code is supposed to do is not limited to update the name and the number. Actually, since I have many collections of objects in my customer object, I was looking for something opposite, the general solution that could be simply applied to any collection I have. That's way the controller code would be simple and would not change if for some reason I would have to change attributes of object in collection.

[MeetTitan]

All of what you say is correct, except for your last couple sentences. HashMaps do indeed have O(1) (amortized) access time, and by extension HashSets do too; however since there is no way to access an element at any given position in a set, you still need to perform the O(n) search to receive said object from your set to do anything with it. In other words you wouldn't be able to pull any object from the set without either a linear search, or having a reference to the object beforehand, which negates our problem entirely.

[Partha]

So, what I was getting at is the "behind-the scenes" part here. The developer needn't find an object here, which is obviously O(n) for a set, instead, he can just call remove and then add - both operations happen in O(1) time. So, the headache of finding the object and modifying it is gone. The problem statement is not about modifying an existing object in the collection. Its about replacing it. If it was just about modifying, then I agree HashMap is a better choice.

[MeetTitan]

I'm well aware of the "behind-the-scenes" action here. You also must be aware of the Set interface contract that HashSet is bound to. How do you propose calling Set.remove(Object o) without a reference to the object to be removed? A O(n) search is needed to retrieve the reference to even begin the O(1) removal process, as an object reference is needed to remove from a set.

[Partha]

That object is the one you want to replace an existing object with. So, you create that and pass on that object. This is only in the replace scenario (not the modify scenario) I am talking about, since set doesn't really provide a replaceIfFound()

[MeetTitan]

I'd like to see the hashCode() method you have in mind to make this work properly.

[Partha]

It will return the id of the Phone object, as per the design, the id is supposed to be unique.

[MeetTitan]

So you're never explicitly calling remove, simply relying on the duplicate removal of the set? That would work, but that did not seem like what you were getting at when you said removal from a HashSet is constant time. If you update your answer to reflect our conversation, it'll score an upvote from me. :)

[Partha]

Thanks :) If you see my comment above - where I was explaining "behind-the-scenes" I did mention about remove and then add. Also, removal from HashSet is O(1) anyway. In our case, this is constant time when we dont have to look up for that object, just replace it. Replacing = remove +add in the set.