2

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?

Improve this question
2
  • No, you can't do this. If the collection is unmodifiable, how would you expect that to work? Commented Apr 19, 2016 at 18:48
  • yes, it couldn't work, because foreach is a read-only loop, you cannot change a reference to phone Commented Apr 19, 2016 at 18:53

5 Answers 5

Reset to default
5

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).

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

9 Comments

Would be slightly cleaner IMHO if creating the replacement was done outside the loop in your 2nd if. Maybe renaming original to found?
Would also be simpler using an iterator to delete inside the iteration whilst avoiding a ConcurrentModificationException.
you may earn UnsupportedOperationException because not all subclasses implement the remove method (which is default and throws this exception)
@user949300 - Agreed. I made that change.
@AndrewTobilko - True. But OP's code indicates that the collection is actually an ArrayList, which does support remove.
|
1

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.

Improve this answer

12 Comments

This would only help if he were trying to negate duplicates in his collection.
Well using a HashMap is essentially the same thing. No duplicates allowed.
But what advantages would a set have over a list in this case unless the goal was to eliminate duplicate elements?
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)
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.
|
-1

If order matters to you, you can change Collection to List (Since you're always using an ArrayList anyway) and then:

int index = col.indexOf(phone);
col.remove(phone);
col.add(p, index);
Improve this answer

6 Comments

Trouble is, he doest have a Phone to remove, just an ID of the phone.
He's using an enhanced for loop, which gives him a Phone. It's also possible to use col.remove(index);
But col.indexOf(theNewPhone) will always return -1. Unless he overrides equals for Phone.
@Zircon, what happens if there are duplicates? It is a List after all.
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.
|
-1

If you take the object out from the collection and update its properties, it will get reflected in the same object in collection too.. Hence, you dont have to technically replace object after updating it. As "Mike M." pointed out, you can use hashmap to retrieve the object quickly without iteration and update the object values.

Improve this answer

1 Comment

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

Instead of a list, use a map with the Phone's id as the key. Then your code looks like this:

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");

        Map<Integer, Phone> phoneMap = c.getPhoneMap();
        phoneMap.put(p.getId(), p);

        System.out.println(c);
}
Improve this answer

4 Comments

I love the idea, but why not simply phoneMap.put(p)?
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.
The phone map should be Map<Integer, Phone> rather than Map<String, Phone> since phone IDs are int values, not strings.
@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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.