Win a copy of Kotlin in Action this week in the Kotlin forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Remove method is not working  RSS feed

 
Naziru Gelajo
Ranch Hand
Posts: 175
1
Java Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:



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
    Sheriff
    Posts: 11125
    160
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 175
    1
    Java Netbeans IDE Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Sheriff
    Posts: 11125
    160
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Ranch Hand
    Posts: 175
    1
    Java Netbeans IDE Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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.




    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
    Sheriff
    Posts: 11125
    160
    Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Please read ALL of my first reply carefully. Pay particular attention to the first two paragraphs.
     
    praveen kumaar
    Ranch Hand
    Posts: 461
    22
    Android Chrome Eclipse IDE Google App Engine Java Notepad Oracle Ubuntu Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:



    Hope it helps!
    kind regards,
    praveen

     
    praveen kumaar
    Ranch Hand
    Posts: 461
    22
    Android Chrome Eclipse IDE Google App Engine Java Notepad Oracle Ubuntu Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Marshal
    Posts: 4624
    316
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:


    [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
    Marshal
    Posts: 4624
    316
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Marshal
    Posts: 4624
    316
    BSD
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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
    Marshal
    Posts: 55672
    161
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    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:-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
    Ranch Hand
    Posts: 461
    22
    Android Chrome Eclipse IDE Google App Engine Java Notepad Oracle Ubuntu Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hello! campbell,
    i have just take a look at how bloch does and adviced others to override hash...
    thanks for reminding me...
     
    Consider Paul's rocket mass heater.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!