Problem with 2D array of lists

Question

In short, I have an object called PersonList that has a List of Person objects. I have created a 2D array of PersonLists to simulate a map and randomly place and move Person objects within that map. However, I'm having a problem removing Persons from the 2D array of PersonLists. At the same time, it seems that the same Person will never be added to the same spot in the 2D array even if it moves there.

I have tried my best to create a small version of what I have below. I have a feeling I'm missing something glaring. I'm also open to ways I can implement this system better as this is probably the most inefficient way to do it.

Person:

public class Person {
    private int id;
    private int[] pos = new int[2];

    public Person(int id) { this.id = id; }

    public int getId() { return id; }
    public int[] getPos() { return pos; }
    public void setPos(int x, int y) { pos[0] = x; pos[1] = y; }

    public boolean equals(Object obj) {
        if (obj == null) { return false; }
        if (obj == this) { return true; }
        if (obj.getClass() != getClass()) { return false; }

        Person rhs = (Person) obj;
        return id == rhs.getId();
    }

    public int hashCode() { return id; }
}

PersonList:

import java.util.*;

public class PersonList {
    List<Person> listPersons = new ArrayList<Person>();

    PersonList() {}

    public void add(Person p) { listPersons.add(p); }
    public void remove(Person p) { listPersons.remove(p); }
    public int getSize() { return listPersons.size(); }
}

Now, I have one more class that controls some Persons and contains the 2D array of PersonLists. It looks something like this:

import java.util.*;

public class Control {
    List<Person> persons = new ArrayList<Person>();
    PersonList[][] pmap;

    //numPeople is the number of Person objects to use
    //size is the size of the pmap (used as both length and width)    
    public Control(int numPeople, int size) {
        pmap = new PersonList[size][size];
        //Initialize all PersonList objects within array
        for(int y = 0; y < size; y ++) {
            for(int x = 0; x < size; x ++) {
                pmap[x][y] = new PersonList();
            }
        }

        for(int i = 0; i < numPeople; i ++) {
            persons.add(new Person(i));
        }
        // Defined below    
        placePersons();
    }

    // Randomly place Person on the pmap    
    private void placePersons() {
        Iterator<Person> i = persons.listIterator();
        while(i.hasNext()) {
            Person p = i.next();
            int x, y;
            // Random method to obtain valid position within pmap stored in x and y
            p.setPos(x, y);
            pmap[x][y].add(p);
         }
    }

    //Move person from "src" on pmap to "dest" on pmap    
    private void movePerson(Person p, int[] src, int[] dest) {
        pmap[src[0]][src[1]].remove(p);
        pmap[dest[0]][dest[1]].add(p);
        p.setPos(dest[0], dest[1]);
    }

    //Makes every person wander around the pmap
    public void wander() {
        for(Person p : persons) {
            int[] xy = new int[2];
            // Random method to obtain valid position within pmap stored in xy
            movePerson(p, p.getPos(), xy);
        }
    }                
}

Imagine I kick off "Control.wander()" in a loop. I'm having a problem where Persons are not being removed from the pmap correctly. I've printed off a grid with the size of each PersonList within pmap and the numbers will never decrease. However, they will not increase above the number of Persons in the "persons" list.

That is, it seems like they cannot be removed, but no Person will be added to the same position in pmap more than once, even if they move there.

Is my problem immediately obvious, or should this theoretically work?

Feel free to ask any questions. Thanks for any help, I really appreciate it.

EDIT: I've added some changes I made - I now use an iterator instead of a for-each loop and I have defined a simple hashCode method for Person. As a test, I've added a method (getFirst()) in PlayerPos that returns the 0'th object in the list, or null if there isn't any.

Within movePerson, I have something like:

System.out.println(p + " and " + pmap[src[0]][src[1]].getFirst());

And it expectantly prints two matching id's (I only use 1 person in my tests). I also added the same line within the remove(Person p) function itself in PersonList. However, more often than not, this one will show null for the "getFirst()" result. remove(Person p) is called immediately after my test in movePerson, so why would it return something there but nothing within PersonList itself? Does this shed more light on my problem? Thanks for all the help so far.

EDIT 2: I've also tried making PersonList's listPersons public and manipulating it directly, but I still seem to have the same problem.

EDIT 3: With more testing, in movePerson, I've printed out the size of the src and dest pmap between every line, and it seems like it's being removed from the src pmap correctly, but after the add to the dest, the size of the src pmap increases again. I've triple checked my add function (it's a simple one-liner) and ensured that the array indices are correct. How can this be happening?

Answer 1

private void movePerson(Person p, int[] src, int[] dest) {
    pmap[src[0]][src[1]].remove(p);
    pmap[dest[0]][src[1]].add(p);
    p.setPos(dest[0], dest[1]);
}

should be

private void movePerson(Person p, int[] src, int[] dest) {
    pmap[src[0]][src[1]].remove(p);
    pmap[dest[0]][dest[1]].add(p); //fix on this line
    p.setPos(dest[0], dest[1]);
}

So it looks like Persons were being added to a place other than where they thought they were.

Also note that you shouldn't need to pass in src to the method, since Person already keeps track of where it is:

private void movePerson(Person p, int[] dest) {
    int[] pos = p.getPos();
    pmap[pos[0]][pos[1]].remove(p);
    pmap[dest[0]][dest[1]].add(p);
    p.setPos(dest[0], dest[1]);
}

Edit: Make sure to override hashCode() whenever you override equals(). The absence of this may be interfering with the List method remove(). Somebody else mentioned this but the answer was deleted.

For your purpose hashCode() should just return id assuming this is unique. Don't factor location into it for example - you want a Person to be one identity regardless of where they are. Test this and then update your question if it's still not working.

Answer 2

Is your random method in PlacePersons intentionally commented out, or do you really want to place all Persons in [0, 0].
I don't think that the PersonList class is adding any value, I would have a structure as follows:
Map<Integer, Set<Person>>

The Integer key of the map would represent the y co-ordinate, and the position of Persons in the Set would represent their x co-ordinate.
You could choose whether to store each persons coordinate internally within the Person class whenever they are moved, or you could (at a cost) change getPos to iterate through the Map/Set until they locate themselves and return those co-ordinates.

Or even better Map<Person, Coord> where Coord is a class hold the x-y co-ordinate position.

Remember to implement equals and hashCode properly for Person.

Answer 3

When using:

for(Person p : persons) 
{      
  int[] xy = new int[2];            
  // Random method to obtain valid position within pmap stored in xy             
  movePerson(p, p.getPos(), xy);         
} 

You're obtaining a reference to Person p from your arrayList of persons and then using the symbolic reference generated by the foreach loop to move them. If you actually want to use the remove operation on your collection you need to set up an iterator.

See this for a good explaination