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:
load() method work like that?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?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:
What's wrong? The bug in delete that removes the wrong Person.
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.
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
}
}