Comparing two objects names in an equals method

Question

I'm a little lost on how to correctly compare objects that I'm testing. My issue is that the tests themselves always come out as true due to the code, but any other way I think of doesn't work correctly either.

public class Element {

    private String atomLetter;
    private String name;

    public Element(String atomLetter, String name) {
        this.atomLetter = atomLetter.toUpperCase();
        this.name = name.toLowerCase();
    }

    public Element(String atomLetter) {
        this(atomLetter, "");
    }

    public String getAtomLetter() {
        return atomLetter;
    }

    public String getName() {
        return name;
    }

    // TODO: two elements are considered to be equal if they have the same atom letter.
    @Override
    public boolean equals(final Object obj) {



        if (atomLetter == this.atomLetter){

            return true;
        }

        return false;

    }

    @Override
    public String toString() {
        return "Element{" +
                "'" + atomLetter + "'" +
                ", name='" + name + '\'' +
                '}';
    }
}

In this case, the outcome comes out exactly the same, but the issue is the equals method.

@Test
public void testSimpleMolecules() {
    // simple carbon
    Molecule m1 = new Molecule("");
    assertTrue(m1.isEmpty());
    assertEquals(0, m1.size());
    m1.add(new Element("C"));
    assertFalse(m1.isEmpty());
    assertEquals(1, m1.size());
    assertEquals(new Element("C"), m1.get(0));
    // simple hydrogen
    Molecule m2 = new Molecule("");
    m2.add(new Element("H"));
    assertFalse(m2.isEmpty());
    assertEquals(1, m2.size());
    assertEquals(new Element("H"), m2.get(0));
    // simple nitrogen
    Molecule m3 = new Molecule("");
    m3.add(new Element("N"));
    assertFalse(m3.isEmpty());
    assertEquals(1, m3.size());
    assertEquals(new Element("N"), m3.get(0));
    // simple oxygen
    Molecule m4 = new Molecule("");
    m4.add(new Element("O"));
    assertFalse(m4.isEmpty());
    assertEquals(1, m4.size());
    assertEquals(new Element("O"), m4.get(0));
}

Answer 1

You can implement the equals method this way. However, you also have to implement hashCode for correctness.

For equals

@Override
public boolean equals(Object obj)
{
   // check the instance of obj
   if (!(obj instanceof Element)) return false;
   
   // check if obj is itself
   if (obj == this) return true;
   
   // cast obj as Element
   Element e = (Element) obj;
   
   // compare fields
   return this.atomLetter.equals(e.atomLetter) &&
   this.name.equals(e.name);
 }

For hashcode, you can implement it a number of ways, but usually this is the quickest and easiest way.

@Override
public int hashCode()
{
  return Objects.hash(atomLetter, name);
}

Answer 2

The Answer by DarkSigma is correct about your comparison of this.atomLetter to itself. You have a few other issues with your equals.

Compare content, not references

Your code … == this.atomLetter is comparing object references (pointers) rather than the textual content of those String objects. In other words, you are asking if the two variables both refer to the same object, that is, the same chunk of memory.

Always compare String content by calling String::equals or String::equalsIgnoreCase.

For implementing equals, you can test for references being the same, as a quick first part of the equality testing. But this alone is not enough.

if ( this == other ) return true;

Test for null

You should test for null. If the other object reference is null, there is no object, so there can be no equality.

 if ( other == null ) return false;

Test for class

You can also make sure the class of the two objects match.

 if ( other == null || getClass() != other.getClass() ) return false;

Cast

As the other Answer mentioned, you should cast the passed Object, having gotten past the class-matching test shown above.

Element element = ( Element ) other;

Check for matching content

As the last test, check for matching content.

In this particular case, I suspect you do care about case matching. So we call String::equals rather than String::equalsIgnoreCase.

return getAtomLetter().equals( element.getAtomLetter() );

Example equals method

Let's pull that all together into a single equals implementation.

@Override
public boolean equals ( Object other )
{
    if ( this == other ) return true;
    if ( other == null || getClass() != other.getClass() ) return false;
    Element element = ( Element ) other;
    return getAtomLetter().equals( element.getAtomLetter() );
}

Tip: Your IDE will generate this code for you. No need to write this yourself. For example, in IntelliJ, choose: Code > Generate > equals() and hashCode.

Always implement hashCode when implementing equals

As discussed many times on Stack Overflow, such as here, when writing an equals method, always write a hashCode method using the same logic.

@Override
public int hashCode ( )
{
    return Objects.hash( getAtomLetter() );
}

Example class

So we end up with a Element class that looks like this.

package work.basil.example;

import java.util.Objects;

public class Element
{
    // Member fields
    private String atomLetter, name;

    // Constructor
    public Element ( String atomLetter , String name )
    {
        this.atomLetter = Objects.requireNonNull( atomLetter ).toUpperCase();
        if ( this.atomLetter.isBlank() ) { throw new IllegalArgumentException();}
        this.name = Objects.requireNonNull( name ).toLowerCase();
    }

    // Getters (read-only).
    public String getAtomLetter ( ) {return atomLetter;}

    public String getName ( ) {return name;}

    // `Object` overrides

    @Override
    public boolean equals ( Object other )
    {
        if ( this == other ) return true;
        if ( other == null || getClass() != other.getClass() ) return false;
        Element element = ( Element ) other;
        return getAtomLetter().equals( element.getAtomLetter() );
    }

    @Override
    public int hashCode ( )
    {
        return Objects.hash( getAtomLetter() );
    }

    @Override
    public String toString ( )
    {
        return "Element{ " +
                "atomLetter='" + atomLetter + '\'' +
                " | name='" + name + '\'' +
                " }";
    }
}

Answer 3

In your equals method, you are comparing this object's atomLetter to itself.

if (atomLetter == this.atomLetter){ 

Instead, you need to cast the obj argument to the Element class and compare its atomLetter to this.atomLetter

Element other = (Element) obj;
return this.atomLettet == other.atomLettet;

Of course, you'll likely want to test that the cast is possible before actually doing it, and say that the objects are not equal if they are of different classes. Also test for null. The javadoc for object.Equals() explains all if the requirements for a proper equals method.