Java: Why Doesn’t My Equals Method Work?

Question

I implemented the equal’s method in my Person object with the @annotation, however, when I create an object in my main class and check it against my List of Person objects, it doesn’t return the correct index. It says they are both at 1, but it should be 1 and 3.

Objective:

Check to see if “Andy Bernard” exists in my list and if so, display the index of the objects.

import java.util.ArrayList;
import java.util.List;

public class Person {
    private String firstName;
    private String lastName;

    public Person(String firstName,String lastName) {
        this.firstName = firstName;
        this.lastName = lastName;
    }

    public String getFirstName() {
        return firstName;
    }
    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }
    public String getLastName() {
        return lastName;
    }
    public void setLastName(String lastName) {
        this.lastName = lastName;
    }

    @Override 
    public String toString() {
        return String.format(this.firstName + " " + this.lastName);  
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result
                + ((firstName == null) ? 0 : firstName.hashCode());
        result = prime * result
                + ((lastName == null) ? 0 : lastName.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        Person other = (Person) obj;
        if (firstName == null) {
            if (other.firstName != null)
                return false;
        } else if (!firstName.equals(other.firstName))
            return false;
        if (lastName == null) {
            if (other.lastName != null)
                return false;
        } else if (!lastName.equals(other.lastName))
            return false;
        return true;
    }

    private static List<Person> deletePeople = new ArrayList<>();

    public static void main(String[] args) {
         addPerson("Micheal","Scott");
         addPerson("Andy","Bernard");
         addPerson("Micheal","Scott");
         addPerson("Andy","Bernard");
         display();

    }

    public static void addPerson(String firstname, String lastname) {
        Person createPerson = new Person(firstname,lastname);
        deletePeople.add(createPerson);
    }

    public static void display() {
        Person checkPerson = new Person("Andy","Bernard");

        for (Person display : deletePeople) {
            if(display.equals(checkPerson)) {
                System.out.println((display.getFirstName() + " " +
                display.getLastName() + " " + deletePeople.indexOf(display)));
            }
        }
    }
}

Answer 1

indexOf returns the index of the first occurence which is the same (according to the equals method) in the List. That's why you see both 1 printed.

Returns the index of the first occurrence of the specified element in this list, or -1 if this list does not contain the element. More formally, returns the lowest index i such that (o==null ? get(i)==null : o.equals(get(i))), or -1 if there is no such index.

And you can see the implementation (for ArrayList, but that's the same idea for a LinkedList except that you iterate other the nodes):

229  public int indexOf(Object o) {
230      if (o == null) {
231          for (int i = 0; i < size; i++)
232              if (elementData[i]==null)
233                  return i;
234      } else {
235          for (int i = 0; i < size; i++)
236              if (o.equals(elementData[i]))
237                  return i;
238      }
239      return -1;
240  }

If you want to print the index associated, you can use a counter that you increment at each iteration.

int index = 0;
for (Person display : deletePeople) {
    if(display.equals(checkPerson)){
        System.out.println((display.getFirstName() + " " + display.getLastName() + " " + index));
    }
    index++;
}

It would also be possible with a standard for loop.

for(int i = 0; i < deletePeople.size(); i++){
    Person display = deletePeople.get(i);
    if(display.equals(checkPerson)){
        System.out.println((display.getFirstName() + " " + display.getLastName() + " " + i));
    }
}

However be careful with the latter implementation because if the underlying representation of the list is a LinkedList the complexity of the above loop would be O(n^2) instead of O(n).

Answer 2

By calling indexOf() you print the first index that matches, rather than the current index. If you're interested in tracking your position in the list, the best thing is to do that manually, so instead of a for-each loop:

for(Object o : ls) {
  // the syntax is nice, but we don't have an easy way to
  // tell how far into the list we are
  System.out.println(o);
}

Instead, use a simple for loop:

for(int i = 0; i < ls.size(); i++) {
  Object o = ls.get(i);
  // Now we have access to the object *and* its index
  System.out.println(i+": "+o);
}

---

Additionally, there's quite a few simple cleanups you could make in your code, if you'll forgive me, here's some suggestions:

1. Avoid allowing null in your objects - rarely is null actually meaningful, and allowing it forces all calling code to do null checks everywhere.

   public Person(String firstName,String lastName) {
     if(firstName == null || lastName == null) {
       throw new NullPointerException("First/Last name cannot be null");
     }
     this.firstName = firstName;
     this.lastName = lastName;
   }
   

2. Use String.format() correctly, or just avoid it. Your current toString() method calls String.format(this.firstName + " " + this.lastName), which doesn't actually do anything; you might as well just return the concatenated string without String.format(). Alternatively, you can properly use the format functionality like so:

   String.format("%s %s", firstName, lastName);
   

3. Take advantage of Java 7's Objects class for your hashCode() and equals() methods:

   @Override
   public int hashCode() {
       return Objects.hash(firstName, lastName);
   }
   
   @Override
   public boolean equals(Object obj) {
       if (this == obj)
           return true;
       if (obj == null)
           return false;
       if (getClass() != obj.getClass())
           return false;
       Person other = (Person) obj;
       return Objects.equals(firstName, other.firstName) &&
              Objects.equals(lastName, other.lastName);
   }