Did I implement equals and hashCode correctly using Google Guava?

Question

I am using hibernate and need to override equals and hashCode(). I chose to use google-guava's equals and hashCode helpers.

I wanted to know if I am missing something here.

I have get/set methods for idImage and filePath.

@Entity
@Table(name = "IMAGE")
public class ImageEntity {
    private Integer idImage;
    private String filePath;

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

    @Override
    public boolean equals(final Object obj) {
        if(obj == this) return true;
        if(obj == null) return false;

        if(obj instanceof ImageEntity){
            final ImageEntity otherImage = (ImageEntity) obj;
            return Objects.equal(getFilePath(), otherImage.getFilePath());
        }
        return false;
    }
}

EDIT:

Ran into inheritance and have a sample here

It looks right... are you getting any errors/unwanted behavior with it? — Marcelo, Feb 13 '12 at 08:28
@NimChimpsky idImage is the id mapping to auto increment Primary Key in database (using hibernate). — brainydexter, Feb 13 '12 at 08:59
@Marcelo No, not as of now. everything works great. I just wanted to know if I missed on anything. — brainydexter, Feb 13 '12 at 08:59
For anyone else, who comes here wandering: http://www.angelikalanger.com/Articles/JavaSolutions/SecretsOfEquals/Equals.html — brainydexter, Mar 27 '12 at 11:11

Radu Murzea · Accepted Answer · 2012-08-14T13:16:44.113

18

The problem with the instanceof operator is that it works taking into account polymorphism, if I may say so.

Let's say, for example, that you do this:

public class AdvancedImageEntity extends ImageEntity
{
    //some code here
}

and then you do this:

ImageEntity ie = new ImageEntity ();
AdvancedImageEntity advanced_ie = new AdvancedImageEntity ();

boolean this_will_be_true = ie.equals (advanced_ie);

As the name suggests, that equals call will return true, because of the instanceof operator.

I know this sounds like basic stuff and most people know it, but it's SOOOO damn easy to forget it. Now, if you want that behaviour, then it's fine, you implemented equals correctly. But if you consider that an ImageEntity object must not be equal to an (hypothetical) AdvancedImageEntity object, then either declare ImageEntity to be final OR forget about instanceof and implement your equals method like this:

@Override public boolean equals(final Object obj)
{
    if(obj == this) return true;
    if(obj == null) return false;

    if (getClass ().equals (obj.getClass ()))
    {
        final ImageEntity otherImage = (ImageEntity) obj;

        return Object.equals (getFilePath(), otherImage.getFilePath());
    }

    return false;
}

This will check the object's true type, no matter what type the reference is. If the obj parameter is an instance of a subclass, it would "slip" by instanceof. But getClass is a lot more strict and won't allow it.

PS: I'm not saying that instanceof is bad and should not be used. I'm just saying that you must be aware of this particular situation and decide whether to use it taking this into account.

edited Aug 14 '12 at 13:16

answered Feb 13 '12 at 10:33

Radu Murzea

10,724
10
47
69

1

Better yet--don't subclass DTOs – Ray Feb 13 '12 at 14:57
1

http://en.wikipedia.org/wiki/Data_transfer_object; roughly equivalent to value objects, if you're more familiar with that term. In this case, you'd want to favor composition over inheritance. – Ray Feb 16 '12 at 19:31
I think first two `if` statements in `equals` are redundant because the Guava's `Object#equals` do those. – Sanghyun Lee Aug 02 '12 at 09:45
@Sangdol The `equals` method is overriden. There is no call anywhere to `super.equals()`. This means that whatever is going on in `Object#equals` is irrelevant (because it's never executed). – Radu Murzea Aug 02 '12 at 09:52
Oh, I misread your code. I thought you used Guava's `Objects#equal`. Check [this code](http://grepcode.com/file/repo1.maven.org/maven2/com.google.guava/guava/r09/com/google/common/base/Objects.java#Objects.equal%28java.lang.Object%2Cjava.lang.Object%29) – Sanghyun Lee Aug 02 '12 at 10:02
@Sangdol I understand now what you meant. I think I should edit my answer though... I totally forgot about `obj instanceof A` where `A` is an interface, not a class. Maybe later tonight :) . – Radu Murzea Aug 02 '12 at 10:06
What's the purpose of comparing class names rather than classes? – Dave Newton Aug 14 '12 at 12:33
@DaveNewton Hmmm, I guess I thought that either works. But actually... what if both classes have the same name but are in different packages ? You probably have a point here, let me look into it :) . – Radu Murzea Aug 14 '12 at 13:05
`getName` returns the FQN, AFAIK. It just seems redundant. – Dave Newton Aug 14 '12 at 13:08
"Don't subclass DTOs" may not be possible with tools like Hibernate, which subclass your model objects with proxies to implement features like lazy loading. – Marvo Nov 25 '12 at 00:10

Thomas Jung · Answer 2 · 2012-02-13T15:04:26.753

13

You could actually use the Guava EqualsTester to test your equals and hashCode implementation:

new EqualsTester()
     .addEqualityGroup("hello", "h" + "ello")
     .addEqualityGroup("world", "wor" + "ld")
     .addEqualityGroup(2, 1 + 1)
     .testEquals();

It's in the guava testlib.

<dependency>
    <groupId>com.google.guava</groupId>
    <artifactId>guava-testlib</artifactId>
    <scope>test</scope>
</dependency>

Minor change to your implementation:

@Override public boolean equals(final Object obj) {
    if(obj == this) return true;
    return obj instanceof ImageEntity &&
        Objects.equal(getFilePath(), ((ImageEntity) obj).getFilePath());
}

edited Feb 13 '12 at 15:04

answered Feb 13 '12 at 14:58

Thomas Jung

32,428
9
84
114

@David Don't understand your argument. – Thomas Jung Jun 30 '12 at 08:30
@David I'd expect that this fails, which is obviously correct in this example. – Thomas Jung Jul 01 '12 at 08:58

score 3 · Answer 3 · edited May 23 '17 at 10:26

3

Its fine as is. Although instanceof makes the null check unnessary.

Also, I think its fine to use auto generated id's, although this is very debatable.

edited May 23 '17 at 10:26

Community

1
1

answered Feb 13 '12 at 09:13

NimChimpsky

46,453
60
198
311

I agree on the redundancy of the null check. Just remove it, and it'll be ok. – Olivier Grégoire Feb 13 '12 at 10:19
Also, you could just go ahead and use `getFilePath().hashCode()` instead of `Objects.hashCode(getFilePath())`. – Louis Wasserman Feb 13 '12 at 16:09
@LouisWasserman - That will fail if `getFilePath()` can return `null`. It's not mentioned in the docs, but I suspect that `Objects.hashCode()` which calls `Arrays.hashCode()` handles `null` values safely. – David Harkness Jun 29 '12 at 20:20
Perhaps, although `getFilePath()` returning null _probably_ reflects a bug. ;) – Louis Wasserman Jun 30 '12 at 10:01

Did I implement equals and hashCode correctly using Google Guava?

3 Answers3

Linked