Modified LinkedList (2 integers per node) with ListIterator

Question

I'm writing a code to save, delete and load a person's height and weight data. I have created 2 classes:

class Person {
    private int height;
    private int weight;

    public Person(int h, int w) {
        height = h;
        weight = w;
    }

    public int getHeight() {
        return height;
    }

    public int getWeight() {
        return weight;
    }

    public String getValues() {
        return ("Height "+height+" and weight "+weight);
    }
}


class DataModified {        //Problem in this class
    private LinkedList<Person> lList;
    private ListIterator<Person> lIter;

    public DataModified() {
        lList = new LinkedList<Person>();
        lIter = lList.listIterator();
    }

    public void save(Person p) {        
        Person p1, p2;                                  //p1: Data needed to be saved
        p1 = new Person(p.getHeight(), p.getWeight());  //p2: Data already on the list
        boolean alreadyExist = false;
        lIter = lList.listIterator();
        while(lIter.hasNext()) {        
            p2 = lIter.next();      
            if ((p2.getHeight() == p1.getHeight()) && (p2.getWeight() == p1.getWeight())) {
                alreadyExist = true;        
            }
        }
        if(alreadyExist) {
            System.out.println("Person: " + p1.getValues() + " had already been on the list.");
        }
        else {
            lIter.add(p1);
            System.out.println("Person: " + p1.getValues() + " is added to the list.");
        }
    }

    public void delete(Person p) {
        Person p3, p2;                                  //p3: Data needed to be deleted
        p3 = new Person(p.getHeight(), p.getWeight());  
        boolean alreadyExist = false;
        lIter = lList.listIterator();
        while(lIter.hasNext()) {        
            p2 = lIter.next();      
            if ((p2.getHeight() == p3.getHeight()) && (p2.getWeight() == p3.getWeight())) {
                alreadyExist = true;        
            }
        }
        if(alreadyExist) {
            lIter.remove();
            System.out.println("Person: " + p3.getValues() + " is deleted from the list.");
        }
        else {
            System.out.println("Person: " + p3.getValues() + " is not on the list.");
        }
    }

    public void load() {            //Problem
        lIter = lList.listIterator();
        Person p2;
        for(int i = 1; lIter.hasNext(); i++){
            p2 = lIter.next();
            System.out.println("Person "+i+" has "+p2.getValues());
        }
    }
}

I had tested these 3 methods from class DataModified: I first save data from 3 persons, then delete 1 person and load the rest. However, the last method prints not the 2 persons on the list, but the person I have deleted before.

My question are:

1. What is wrong with my code? Why did the load() method work like that?
2. I noticed that after iterating, I can only modified the lIter. So are lList and lIter the same list or 2 separated lists? If they are not the same, how can I give lList the data from lIter?
3. Is there a way to stop the iteration of a list?

Answer 1

As others are pointing out, you definitely have a bug in your delete method. As written, if the target Person is found in the list, then the last Person in the list will be removed, not the target. You really need to be able to cut out of the while loop when you find the person and remove it immediately instead of continuing the loop.

To answer your questions:

1. What's wrong? The bug in delete that removes the wrong Person.

2. Are lList and lIter the same list? Conceptually yes, technically no. lList is the list itself, and lIter is the Iterator acting on that list. It's not a list itself. It's an Iterator. But the data it's working on is definitely the same list.

3. How to stop iteration? You have a few options. The easiest for how your code is currently written is a break statement. It stops execution of the current loop and resumes executing outside the block. In both your add and delete methods, it would make sense to break right after alreadyExist is set to true. Another option, suggested first by jiveturkey, is to add alreadyExist as a condition to the while loops. Then you'll only continue iterating if both there are more items to iterate over and alreadyExist hasn't yet been set to true. A third option would be to do the real work (i.e delete) as soon as you find the Person and then return from the method entirely.

Beyond that, some unsolicited general advice :)

• You're comparing Person objects in multiple methods. Over time, this will get hard to maintain, so it would be better to define the comparison in one place. Java comes with an equals method for this. It's in every Object, but the default implementation won't help you, so you want to override it. In your case, you consider two distinct Person objects to be conceptually equal if their heights and weights are equal. So override the equals() method in Person to return true iff the height and weight are equal. See How to override equals method in java or http://users.csc.calpoly.edu/~gfisher/classes/102/info/howToOverrideEquals.html for some tips. If you override equals, you also need to override hashCode.

• You're making copies of the Person parameters. The exact object being passed in is not the actual object being added or deleted from the list; the copy is. You could just use the paremeter. In the best case, you currently have an unnecessary performance hit (creating extra objects). In the worst case you'll hit bugs.

• lIter is set both in the constructor and in every method. If you don't need to store its current state across method calls, then it should probably be just a local variable, used for a method and then discarded.

• getValues() is currently just being used to make the object human-readable. That's a common problem, and the task given to toString(), also defined in Object and overridable in any class you write. All you'd need to do to take advantage of it is rename getValues to toString. Then you could just use it in a log message directly. Example below.

Here's how I would then rewrite delete, assuming a good equals method in Person, and getValues renamed to toString:

public void delete(Person p) {
    boolean alreadyExist = false;
    ListIterator definitelyNotLIter = lList.listIterator();
    while(definitelyNotLIter.hasNext()) {        
        Person current = definitelyNotLIter.next();      
        if (p.equals(current)) {
            alreadyExist = true;
            definitelyNotLIter.remove();

            // Option 1:
            break;  // next line to execute will be the if(alreadyExist) block

            // Option 2:
            // put your successful delete logging here
            // return;
            // and leave the failed delete logging outside the loop

            // Option 3:
            // Do nothing. The looping will continue, and you'd have a deleteAll method, where multiple items would get deleted if you managed to get duplicates in the list.
            // You actually wouldn't need alreadyExist any more.

            // I'd go with option 1, myself
        }
    }
    if(alreadyExist) {
        System.out.println("Person: " + p + " is deleted from the list."); // p.toString() will get called
    }
    else {
        System.out.println("Person: " + p + " is not on the list."); // p.toString() will get called
    }
}

Answer 2

In delete(), once you find your Person you set alreadyExist to true the first time you find someone and keep going. So once it's set, it's set for EVERYONE. If you find your Person in the list in position 1, you'll delete him, but then you'll delete the next person and so on. All of your while loops need to say.

while (!alreadyExists && lIter.hasNext()) {        
    p2 = lIter.next();      
    if ((p2.getHeight() == p1.getHeight()) && (p2.getWeight() == p1.getWeight())) {
            alreadyExist = true;
            lIter.remove();
        }
    }

    // Delete the if/else code

You should probably get rid of the member variable:

private ListIterator<Person> lIter;

Answer 3

The problem is in your delete method:

You loop through the entire list before removing anything. ListIterator.remove only deletes the last item returned by next(). You need to break when you find the item that already exists:

while(lIter.hasNext()) {        
    p2 = lIter.next();      
    if ((p2.getHeight() == p3.getHeight()) && (p2.getWeight() == p3.getWeight())) {
        alreadyExist = true;   
        break;     
    }
}

---

Additionally, the way you are comparing Person objects should really be done in a equals() method, as described here.

---

Finally, your creation/use of an extra Person that is a duplicate of parameter p is entirely unnecessary here. Just use p instead of p1 (in save) and p3 (in delete).

Answer 4

From Documentation:

void remove() Removes from the list the LAST element that was returned by next() or previous()

But when you reach the required Person Object, you continue iteration the next Person Object.

What you need is to break the while loop once it finds the target object.

if ((p2.getHeight()==p3.getHeight())&&(p2.getWeight()==p3.getWeight())){
      alreadyExist = true;
      break;        
}

Now the last element is the required Object, you can now use remove().

if(alreadyExist) {lIter.remove();}