I've tried several ways of splitting it. All of them seem to be equally unreadable.
I've tried several ways of splitting it. All of them seem to be equally unreadable.
For further actions, you may consider blocking this person and/or reporting abuse
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).
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.
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 theOptional<T>
, which pretty much rendersnull
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, andOptional<T>
should be used instead, in your team. Exclusively in implementation you sometimes cannot avoidnull
, 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 withoutOptional<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.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. :)
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! :)
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. :)
I see, that sounds difficult. :)
I thought everyone with a bit of experience with Java knows the standard form of equals() method:
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 onlyObject
class, so they do not really belong there. The natural thing in that case is to use mixin, but you cannot overrideequals()
in an interface. Etc. Literally, every combination of split I've tried led to some kind of unreadability. :)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:The
instanceof
operator takes care of the null check onrhs
, and the helpers fromObjects
handle nullable properties. In a base class, you might want to usegetClass()
instead ofinstanceof
or make the methodfinal
, 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 anequals()
-implementation makes you think about method extraction, that's a design smell to me.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 anothernew Integer(1000)
. :)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 definitionequals()
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.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.
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. ;)My favourite approach is to implement
Comparable<T>
then make equals just do: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.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 overrideequals()
there, and in that case there will beequals()
that calls some methods that I do not see, and by some magic, it later callsisEqualTo()
.I like idea with
Comparable
. It makes sense, but in some situations it is an overkill, I agree.I use Apache EqualsBuilder and HashcodeBuilder
Good idea, but it still doesn't solve the mess with null-check, identity-check, and class-check. :)
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 == .
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.