Alexey Voinov

Posted on Sep 2, 2017

Did you ever try to write Java equals() with clean code style?

#discuss #cleancode

I've tried several ways of splitting it. All of them seem to be equally unreadable.

Top comments (19)

My personal strategy has two methods: One for comparing against any Object, and one more to compare against objects of the same Type.
This will also get rid of the noisy boilerplate null checks and boilerplate class comparisons and boilerplate runtime cast (when used with Cat objects at compile time).

// inside class Cat

// contains verbose boilerplate code
@Override 
public boolean equals(@Nullable Object object){
    return object == this || (
        other != null 
        && other.getClass() == this.getClass() 
        && this.equals((Cat)object) // call the other method
    );
}

// contains core equality logic
public boolean equals(@NonNull Cat cat){
    return cat == this || (
        cat.name.equals(this.name)
        && cat.age == this.age
    );
}

Alexey Voinov • Nov 15 '17

Yeah, this approach looks nice. But I see two problems here:

this nice and shiny @NonNull annotation does nothing. You still can call the second method even with literal null argument and compiler won't even issue a warning. Yeah, IDE may warn you about it, but imagine, that someone on your team is still using vim or emacs. :)
instanceof is not exactly what you need to use if your Cat class has any descendants.

Johannes Vollmer • Nov 15 '17 • Edited

That's right. That instanceof call is problematic. I've editet that to compare classes.

To be honest, I personally think, the best approach to handling null in Java is simply to avoid it. Nullability is, in my opinion, very rarely really needed. Java 8 has the Optional<T>, which pretty much renders null useless (unless you're in that 0.5% bottleneck).

That's why I think @NonNull should be the implicit default for Parameters or basically any part of the API, and Optional<T> should be used instead, in your team. Exclusively in implementation you sometimes cannot avoid null, but you'll have to live with that.

The compiler may not warn you when you pass null as an argument, regardless of using @NonNull. Your App will crash either way. The only difference is a slightly more precise Exception when it crashes.

I've come to a point where just seeing null as an argument is suspicious. Even without Optional<T> you could often write an overloaded method that simply omits that argument.

So that's a convention you'd have to follow, just like so many conventions in Java. It's not that much work to check for any null literals, and it saves you a hundred of noisy and verbose null-checks.

If you want to use a language that can be used without tons of conventions, don't use Java. I recently started learning Rust, which seems to solve some of these Problems very well. If you need JVM-Bytecode, try Kotlin. It handles null more elegant than Java.

Alexey Voinov • Nov 16 '17

Yes! Yes! Yes! I agree. Well, almost. :) It's still easy to get null without actually writing literal null. And you should probably check for nulls in your public API. But otherwise, yes, I prefer to trust my own (or my team's) code in private scope.

But all of that (including the edited version of your example), leads us back to the original problem: how to write equals() with clean code rules in mind. :) I like your answer: use Kotlin, but unfortunately, I'm working on a rather large project and I cannot just change the language.

Thank you. :)

Johannes Vollmer • Nov 16 '17

Thanks, I agree. :)

Did you try to speak with your team about Kotlin? I heard it can be used virtually side by side with Java source code. You can exchange maybe just a single file with Kotlin.
Of course, the developers would have to learn Kotlin (which is said to be not that hard if you already know Java).

In hindsight, I realize that I just like the separation of logic and boilerplate in my example.

Cheers! :)

Alexey Voinov • Nov 17 '17

Well, actually I am the one, who's against adding new languages to the code base. It just doesn't work with the team of about a thousand engineers working on a monolithic java project with size over 10GB (source code only). :) I know how to deal with such projects and turning it into something manageable, but it seems like our management don't. :)

Johannes Vollmer • Nov 17 '17

I see, that sounds difficult. :)

Alexey Voinov • Sep 5 '17

I thought everyone with a bit of experience with Java knows the standard form of equals() method:

class SomeClass {
    public boolean equals(Object o) {
        if (this == o) return true;
        if (that == null || getClass() != o.getClass()) return false;
        SomeClass that = (SomeClass) o;
        return this.field == that.field && ...
    }
}

And the moment you start extracting methods, the IDE (IntelliJ IDEA in my case) starts complaining about this method not being of expected form. And the extracted methods mostly have nothing to do with SomeClass, it works with and needs only Object class, so they do not really belong there. The natural thing in that case is to use mixin, but you cannot override equals() in an interface. Etc. Literally, every combination of split I've tried led to some kind of unreadability. :)

Daniel Schmitt • Sep 9 '17

I'm not 100% sure if I really understand your problem, but, that equals() method looks complicated (and buggy, too). equals() is really just a longish one-liner:

import java.util.Objects;

public class NeedsEquals {
    private int primitive;
    private Object someRef;
    private Double[] someArray;

    @Override
    public boolean equals(Object rhs) {
        return rhs instanceof NeedsEquals
               && (this == rhs
                   || primitive == ((NeedsEquals) rhs).primitive
                      && Objects.equals(someRef, ((NeedsEquals) rhs).someRef)
                      && Objects.deepEquals(someArray, ((NeedsEquals) rhs).someArray));
    }

    // don't forget to override hashCode()
}

The instanceof operator takes care of the null check on rhs, and the helpers from Objects handle nullable properties. In a base class, you might want to use getClass() instead of instanceof or make the method final, depending on the use case.

But. George Marr already hinted at the deeper issue: why do you end up needing a full object comparison to establish identity? Unless your instance came over the network or from some other external source, referential identity (i.e. == semantics) should work. Otherwise, how about an explicit ID property? If an equals()-implementation makes you think about method extraction, that's a design smell to me.

Alexey Voinov • Sep 12 '17

Yes, I perfectly understand, that equals() could be written as a one-liner. That doesn't make it clean or even more readable. :) So, the problem is only in the way we express it. I've failed to express it with Clean Code in mind. Partially because of IDE, which requires equals() to have certain form, partially because Java doesn't really support multiple inheritance. :)

As for using identity comparison vs full object comparison is a completely different question. And it won't always work. For example, I'd say I would be very surprised if new Integer(1000) won't be equal to another new Integer(1000). :)

Daniel Schmitt • Sep 12 '17

I guess I just fail to see what problems either of our equals() implementations have with regards to Uncle Bob's cleanliness standards. It doesn't get more concise than that, and basically by definition equals() does exactly one thing. It's pure, and it concerns itself with one level of abstraction (the one the object is on). So I - wrongly - assumed that it would be a design question.

On that note, I realise that there's a difference between value types and entity types; what I was getting at is that if your equals()method is becoming unwieldy, you're likely either dealing with a massively complicated value type (and I have honestly never seen such a thing), or an entity that could benefit from a more explicit approach to identity. If I'm wrong on both counts, I'm all the more curious about what is actually going on in your specific case.

Alexey Voinov • Sep 13 '17

Let me put it like this. Do you remember, that clean code should be readable (almost) like plain English? If I start to read your version, for example, it will be something like "if rhs is instance of NeedEquals and this is the same as rhs or primitive is the same as field primitive in rhs cast to NeedEquals ..." I feel like this is just too low level, and part with Object.equals(...) is hardly readable (in that style) at all.

What I would love to read is something like "if this is the same instance as that or if that is of the same class as this and every field of this is equal to corresponding field of that", or even "this is the same instance as that or has the same content". Do you see it?

There some problems with this approach though, mainly IDE warnings, which could be ignored, but again, it reduces readability and most of the time requires comments, and placement of those methods, because they are very generic and not specific to my class, except for fields comparison.

BTW, thank you for helping me expressing all of this explicitly. :) I love to find things that are obvious to me and not so obvious to someone else.

Daniel Schmitt • Sep 13 '17 • Edited

The only thing I know that comes close to what I think you are looking for has been mentioned already: EqualsBuilder from Apache Commons. Or you could go for a reflection-based approach like in Unitils, but this is probably unacceptable performance-wise outside of unit tests.

Funnily enough, I read both your code and my code exactly like you say you'd want it to read, but I agree that it's not "in the code" as such. It's an idiom - OK, an equals() method, now then: the questions I need the code to answer are: which fields?, shallow or deep comparisons?, nulls OK?, subclasses OK? Both implementations give me those answers at a glance. This can bite you in code reviews, but otherwise it helps to not overthink things. ;)

Neil Madden • Sep 12 '17

My favourite approach is to implement Comparable<T> then make equals just do:

public boolean equals(Object that) {
    return this == that
        || that instanceof T && this.compareTo((T) that) == 0;
}

Isn't always applicable but ensures compareTo is consistent with equals when you want to implement both anyway. Otherwise I write an isEqualTo(T that) method and do similar.

Alexey Voinov • Sep 12 '17

Yeah, I tried to implement isEqualTo(), but I end up with a bunch of nice short methods, that do not belong to my class, and that doesn't look nice. Where should I put it? It would be nice to have some mixin, but I cannot override equals() there, and in that case there will be equals() that calls some methods that I do not see, and by some magic, it later calls isEqualTo().

I like idea with Comparable. It makes sense, but in some situations it is an overkill, I agree.

Lluís Josep Martínez • Sep 12 '17

I use Apache EqualsBuilder and HashcodeBuilder

Alexey Voinov • Sep 12 '17

Good idea, but it still doesn't solve the mess with null-check, identity-check, and class-check. :)

George • Sep 5 '17

Got an example of what you're referring to here?

I tend to only use equals() when comparing strings, when it comes to objects I tend to use == .

Alexey Voinov • Sep 5 '17

I've posted it on the thread above. It is all about the standard Java equals() that is, for example, generated by IntelliJ IDEA.

I'm not sure, that comparing, that what I have is actually two references to the same instance is enough for any more or less complex piece of software. But that could be an interesting challenge. :) Thanks for the idea.

View full discussion (19 comments)

DEV Community

Did you ever try to write Java equals() with clean code style?

Top comments (19)

Read next

My Everyday Tools

What was your win this week?

Thursday Quiz

Believe in what you know already