Removing all instances of Person from a List whose name field matches a given parameter

Question

An exercise demands that all people matching a given name parameter be removed from a List collection. The solution must use a for-each loop.

---

The solution presented below, does not throw any compile-time errors at me, but but it fails to pass one of the Unit tests that are run to verify the correctness of the solution.

Here is what I have tried so far:

public ArrayList<Person> people;
public void remove(String name){
    for(Person i : people){
        if (people.contains(name)){
            people.remove(i);
        }
    }
}

Answer 1

Which test isn't passed?

But let's look at the code:

You get a param 'name'. Then you iterate over a list of Persons. The current person is i. In each step, you ask the list, whether it contains the name (which is a String). Hm.

contains asks the whole list, whether it contains something - this something should match the typ of the elements in the list, shouldn't it?

But while iterating, you could ask each Person (i), whether the persons name is 'name'.

if (i.name.equals (name)) // or 
if (i.getName ().equals (name))

Iterating an meanwhile removing is another issue, to take care of.

Since there is much rumor, and a lot of people seemed to oversee the problem, I have a different solution, than using an interator: Collect the candidates for removing, and substract the whole blacklist as a bunch in the end:

public void remove (String name) 
{
    List <Person> blacklist = new ArrayList <Person> ();
    for (Person p : people) 
    {
        if (p.getName ().equals (name))
        {
            blacklist.add (p);
        }
    }
    people.removeAll (blacklist);
}

Answer 2

Don't use for-each loops if you about to remove from your list:

the for-each loop hides the iterator, so you cannot call remove. 

(from documentation)

Use standard iterator and its remove method instead.

Answer 3

This will require that you person class has a method named getName() of course. Or you could override equals method, however beware of drawbacks as stated in the comments of this answer.

public void remove(String name) {
for (Person person : people) {
    if (person.getName().equals(name)) {
        people.remove(person);
    }
}

Returns true if this collection contains the specified element. More formally, returns true if and only if this collection contains at least one element e such that (o==null ? e==null : o.equals(e)).

http://download.oracle.com/javase/1.5.0/docs/api/java/util/Collection.html#contains(java.lang.Object)

Answer 4

EDIT:

My first answer (below the line) is actually WRONG, just like all the others posted so far. I did-up a little test (below)... We're ALL causing a ConcurrentModificationException in the iterator which underlies the foreach loop. Now there's a gotcha for ya! Sigh.

package forums;

import java.util.List;
import java.util.ArrayList;

class Person
{
  private final String name;
  private final int age;
  Person(String name, int age) {
    this.name=name;
    this.age=age;
  }
  public String getName() { return name; }
  public int getAge() { return age; }
  public String toString() { return name + " " + age; }
}

public class PeopleTest
{
  public List<Person> people;

  public void remove(String name)
  {
    for ( int i=0; i<people.size(); i++ ) {
      Person person = people.get(i);
      if (person.getName().equals(name)) {
        people.remove(person);
        i--;
      }
    }
  }

  public void badRemove(String name)
  {
    for ( Person person : people ) {
      if (person.getName().equals(name)) {
        people.remove(person);
      }
    }
  }

  public void printAll(String message) {
    System.out.println(message);
    for ( Person person : people ) {
      System.out.println("  " + person);
    }
  }


  public void run() 
  {
    // setup
    people = new ArrayList<Person>(5);
    Person dave, kelly, jack, jill, xavier;
    people.add(dave=new Person("Dave", 36));
    people.add(kelly=new Person("Kelly", 25));
    people.add(jack=new Person("Jack", 42));
    people.add(jill=new Person("Jill", 19));
    xavier = new Person("Xavier", 136);

    badRemove("Dave");

    // before image
    assert people.size() == 4;
    printAll("the before list");

    // operation 1
    assert !people.contains(xavier);
    remove("Xavier"); // a no-op.
    assert people.size() == 4;
    assert !people.contains(xavier);

    // operation 2
    assert people.contains(jill);
    remove("Jill"); // she smells!

    // after image
    printAll("the after list");
    assert people.size() == 3;
    assert people.contains(dave);
    assert people.contains(kelly);
    assert people.contains(jack);
    assert !people.contains(jill);
  }

  public static void main(String[] args) {
    try {
      new PeopleTest().run();
    } catch (Exception e) {
      e.printStackTrace();
    }
  }

}

Output

C:\Java\home\src\forums>"C:\Program Files\Java\jdk1.6.0_16\bin\java.exe" -Xms4m -Xmx256m -enableassertions -cp c:\java\home\src;C:\Java\home\classes; forums.PeopleTest
java.util.ConcurrentModificationException
        at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
        at java.util.AbstractList$Itr.next(AbstractList.java:343)
        at forums.PeopleTest.badRemove(PeopleTest.java:36)
        at forums.PeopleTest.run(PeopleTest.java:62)
        at forums.PeopleTest.main(PeopleTest.java:89)

---

Dude,

THIS ANSWER IS WRONG, PLEASE IGNORE IT!!!

I suggest you use http://download.oracle.com/javase/6/docs/api/java/util/ArrayList.html#remove%28java.lang.Object%29 instead.

Something like:

public ArrayList<Person> people;

public void remove(String name) {
  for ( Person person : people ) {
    if (person.getName().equals(name)) {
      people.remove(person);
    }
  }
}

... presuming the Person has a getName mothod, which returns a string.

Cheers. Keith.

Answer 5

I dont know how do you store the name on the Person class but maybe you could do something like this. Remember that i stores ith element on the people arraylist so each iteration you need to check for the name of the current person to check if you need to remove it

public ArrayList<Person> people;

public void remove(String name){
        for(Person i : people){
            if (i.getName().equals(name)){
                people.remove(i);
            }
        }
    }