Remove method is not working

[Naziru Gelajo]

Greetings ladies and gentlemen, for some reason, in my Pseudo database, my remove method seems to be completely ineffective and isn't working. The source code is below:

import java.util.ArrayList;
import java.util.Scanner;

public class Lab2 {
	static ArrayList<Person> peopleDirectory = new ArrayList<Person>();
	
	public static void main(String[] args) {
		// TODO Auto-generated method stub
		int choice;
		
		Scanner userInput = new Scanner(System.in);
		
		do {
			System.out.println("Welcome to the people directory please make a choice from the list below:");
			System.out.println("-------------------------------------------------------------------------");
			System.out.println("1. Add a person to the directory.");
			System.out.println("2. Remove a Person from the directory.");
			System.out.println("3. View the User Directory.");
			System.out.println("4. Exit the directory.");
			choice = userInput.nextInt();
			switch(choice) {
			case 1: 
				    addPerson(new Person());
					break;
			case 2: removePerson(new Person());
					break;
			case 3: displayPeople();
					break;
			case 4: System.out.println("Thanks for using the people diretory!");
				    System.exit(0);
				    break;
			default: System.out.println("Invalid choice! Please select a valid choice!");
				 	break;
			
			}
		} while (choice != 4);
	}
	
	public static void addPerson(Person thePerson) {
		String firstName;
		String lastName;
		String phoneNumber;
		int age;
		if (peopleDirectory.size() >= 10) {
			System.out.println("Sorry the list can not be larger than 10 people");
		} else {
			int i = 0;
			Scanner input = new Scanner(System.in);
			System.out.println("Enter the first name of the Person you would like to add: ");
			firstName = input.nextLine();
			thePerson.setFirstName(firstName);
			System.out.println("Enter the last name of the Person you would like to add: ");
			lastName = input.nextLine();
			thePerson.setLastName(lastName);
			System.out.println("Enter the phone number of the Person you would like to add: ");
			phoneNumber = input.nextLine();
			thePerson.setPhoneNumber(phoneNumber);
			System.out.println("Enter the age of the Person you would like to add: ");
			age = input.nextInt();
			thePerson.setAge(age);
			peopleDirectory.add(i, thePerson);
			
			i++;
		}
	
		
		
	}
	
	public static void removePerson(Person thePerson) {
		if (peopleDirectory.size() < 1) {
			System.out.println("There is absolutely nothing to remove from the Directory");
		} 
		
		
		else {
			Scanner input = new Scanner(System.in);
			System.out.println("Please enter the first name of the person you would like to delete: ");
			String firstName = input.nextLine();
			thePerson.setFirstName(firstName);
			System.out.println("Enter the last name of the Person you would like to add: ");
			String lastName = input.nextLine();
			thePerson.setLastName(lastName);
			System.out.println("Enter the phone number of the Person you would like to add: ");
			String phoneNumber = input.nextLine();
			thePerson.setPhoneNumber(phoneNumber);
			System.out.println("Enter the age of the Person you would like to add: ");
			int age = input.nextInt();
			thePerson.setAge(age);
			for (int i = 0; i < peopleDirectory.size(); i++) {
				if (peopleDirectory.contains(thePerson)) {
					peopleDirectory.remove(thePerson);
				}
			}
			
		}
	}
	
	public static void displayPeople() {
		for (Person person : peopleDirectory) {
			System.out.println("First Name: " + person.getFirstName() + " Last name: " + 
							person.getLastName() + " Phone number: " + person.getPhoneNumber() + 
							" Age: " + person.getAge());
		}
	}
	
	

}

class Person {
	private String firstName;
	private String lastName;
	private int age;
	private String phoneNumber;
	
	public Person (String firstName, String lastName, int personAge, String phoneNumber) {
		this.firstName = firstName;
		this.lastName = lastName;
		this.age = personAge;
		this.phoneNumber = phoneNumber;
	}
	
	public Person() {
		this.firstName = "";
		this.lastName = "";
		this.age = 0;
		this.phoneNumber = "";
	}
	
	public int getAge() {
		return this.age;
	}
	
	public String getFirstName() {
		return this.firstName;
	}
	
	public String getLastName() {
		return this.lastName;
	}
	
	public String getPhoneNumber() {
		return this.phoneNumber;
	}
	
	
	public void setFirstName(String firstName) {
		this.firstName = firstName;
	}
	
	public void setLastName(String lastName) {
		this.lastName = lastName;
	}
	
	public void setAge(int age) {
		this.age = age;
	}
	
	public void setPhoneNumber(String phoneNumber) {
		this.phoneNumber = phoneNumber;
	}
}

When I attempt to remove an element from the ArrayList, it still remains in the arrayList. I have no idea why, but I feel as if my remove method is a bit clunky.

For instance I add an element and attempt to remove it (see output below):

Welcome to the people directory please make a choice from the list below:
-------------------------------------------------------------------------
1. Add a person to the directory.
2. Remove a Person from the directory.
3. View the User Directory.
4. Exit the directory.
1
Enter the first name of the Person you would like to add:
Tom
Enter the last name of the Person you would like to add:
Jones
Enter the phone number of the Person you would like to add:
6073388152
Enter the age of the Person you would like to add:
24
Welcome to the people directory please make a choice from the list below:
-------------------------------------------------------------------------
1. Add a person to the directory.
2. Remove a Person from the directory.
3. View the User Directory.
4. Exit the directory.
3
First Name: Tom Last name: Jones Phone number: 6073388152 Age: 24
Welcome to the people directory please make a choice from the list below:
-------------------------------------------------------------------------
1. Add a person to the directory.
2. Remove a Person from the directory.
3. View the User Directory.
4. Exit the directory.
2
Please enter the first name of the person you would like to delete:
Tom
Enter the last name of the Person you would like to remove:
Jones
Enter the phone number of the Person you would like to remove:
6073388152
Enter the age of the Person you would like to remove:
24
Welcome to the people directory please make a choice from the list below:
-------------------------------------------------------------------------
1. Add a person to the directory.
2. Remove a Person from the directory.
3. View the User Directory.
4. Exit the directory.
3
First Name: Tom Last name: Jones Phone number: 6073388152 Age: 24
Welcome to the people directory please make a choice from the list below:
-------------------------------------------------------------------------
1. Add a person to the directory.
2. Remove a Person from the directory.
3. View the User Directory.
4. Exit the directory.

What could I be doing wrong here?

[Junilu Lacar]

Refer to the API documentation https://docs.oracle.com/javase/8/docs/api/java/util/ArrayList.html#remove-java.lang.Object-

It says it uses the equals() method to find the first object in the list to remove. Since you don't override the equals() method in your Person class, the one inherited from Object will be used, which is basically reference equality. Because of the way you add Person objects to the list, no other object will be equal to any Person object added to the list. Ever. You need to override equals() and hashCode().

There's also a problem with the for-loop on lines 90-93. Please explain what you think this code is doing, particularly, why you think a for-loop is needed.

[Naziru Gelajo]

Junilu Lacar wrote:
There's also a problem with the for-loop on lines 90-93. Please explain what you think this code is doing, particularly, why you think a for-loop is needed.

In my remove method, what I'm doing, is prompting a user for a first name, last name, phone number ,age. If the ArrayList contains a Person within it with that information, then I'm telling the remove method to remove it from the ArrayList.

[Moderator edit] please limit quoting to the parts of the post that you are replying to

[Junilu Lacar]

I was referring specifically to lines 90-93 of your code. You didn't answer the question I asked. Why do you think you need a for-loop here? I know you are trying to remove a Person object from your list but explain what you think each line does. Hint: that code doesn't do what you think it does.

[Naziru Gelajo]

Junilu Lacar wrote:I was referring specifically to lines 90-93 of your code. You didn't answer the question I asked. Why do you think you need a for-loop here? I know you are trying to remove a Person object from your list but explain what you think each line does. Hint: that code doesn't do what you think it does.

for (int i = 0; i < peopleDirectory.size(); i++) {
                if (peopleDirectory.contains(thePerson)) {
                    peopleDirectory.remove(thePerson);
                }
            }

Line per line,

At line 90, I iterate through the entire arrayList (based on the size of the arrayList)

In the second line, I'm saying if the directory contains the particular Person in question, then remove it.

However, I see what you mean about the for loop because the contains method already has it built-in (.contains() is a method that runs in O(n) time thus it is linear like my for loop, so I'm adding inefficient code). So I removed the For loop, but I'm still getting the same results.

[Junilu Lacar]

Please read ALL of my first reply carefully. Pay particular attention to the first two paragraphs.

[praveen kumaar]

Hello naziru,
when you are calling a "contains(E e)" method of any collection API,it just iterates through the collection implicitly and checks for the "e" in the collection using a equals() method of the class E.but in your case you have not override the equals() in Person class so it will use the inherited one(by Object),which in turn will check for the reference equality(never going to be true in your case) because the reference of e in list and one you are comparing with(local refrence in your remove method) are different.so override it,also always override hashcode() when you are overriding equals.because 2 equal object should have same hashcode.

lets see:

public class Person {
 
 {/* your code....*/}
 
// override equals to compare 2 person object.
 @Override
 public boolean equals(Object o) {
   // If the object is compared with itself then return true  
 if(o == this) return true;
  
  /* Check if o is an instance of Person or not
          "null instanceof [type]" also returns false */
if (! ( o instanceof Person ) ) return false;
   
   // typecast o to Person so that we can compare data members
        Person p = (Person) o;
 
  // compare the data members and return accordingly.
return this.firstname.equals(p.fistname) && this.lastname.equals(p.lastname) && this.age == p.age && this.phonenumbe.equals(p.phonenumber);

   // you can also just compare the phone number as 2 person cannot have same phonenumber.
                                  }

@Overide
 public int hashCode(){
  int  hash = 11; // random number(but integer)
       hash = 15*hash + firstname.hashCode() ;
       hash = 15*hash + lastname.hashCode() ;
       hash = 15*hash + age ;
       hash = 15*hash+phonenumber.hashCode() ;
        return hash;
                       }
                      
                     }  // class ends...

Hope it helps!
kind regards,
praveen

[praveen kumaar]

This is the basic idea to override equals() and hashcode() method which i learnt through Effective java (must read book) by joshua Bloch.

[Liutauras Vilda]

Beside what has been mentioned, your Person class looks quite good. Maybe one thing you could improv, is to remove code duplication within no arguments constructor by calling overloaded constructor and supplying appropriate default values. Example:

Person(parameter1, parameter2, parameter3) {
    ...
}

Person() {
    this("Unknown", "", 20);
}

[edit] actually one more thing. When you write getters and setters, keep them grouped.
setName
getName
setSurname
getSurname

rather than

setName
setSurname
getName
getSurname

[Liutauras Vilda]

It appears the are more things about the same Person class.

1. Consider overriding toString() method in Person class, so you wouldn't need your displayPeople() method look as such.

2. Also, don't name variable as "thePerson", that is unnecessary and adds its own bit towards the mess, just call it "person". Think about the variable names always, along with writing code. Whenever you find better name, change it to better one.

3. if (peopleDirectory.size() < 1). For that there is nice method for the lists isEmpty(). (Read API Junilu showed you more often, there you could find things which have been done already, you just need to know about their existence and re-use it).

[Liutauras Vilda]

Also, your method addPerson() for me looks totally wrong. It does different thing than its name suggest. Per my understanding it should add person to a persons list and that is it. This is at least what name says.

If you want to create person, create another method which sets all its member variables and returns fully constructed object and only then pass it to method addPerson().

[Campbell Ritchie]

PK: Don't use 15 when calculating hash codes; if you look up Bloch again, you will find he uses prime numbers throughout. A lot of people used to write methods to simplify hash codes, and in Java7, the standard installation caught up with them, with the Objects class. You would probably write a hash code method like this:-

/**
 * blahblahblah
 * @return a hash code
 */
@Override
public int hashCode()
{
   return Objects.hash(firstName, lastName, age, phoneNumber);
}

If age is an int, it will be boxed into an Integer.

I don't like the idea of no‑arguments constructors. It tells the world, “It is all right to create a Person with no name or phone number and to guess their age.” I would suggest you only have constructors which initialise every field. I think every class shou‍ld have a constructor, and the ideal is to have only one.

[praveen kumaar]

Hello! campbell,
i have just take a look at how bloch does and adviced others to override hash...
thanks for reminding me...