DaveAKing / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Revist deprecation of Objects.equal #1732

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago

Commit 2b3b26449f7c recommends using JDK 7 Object.equals in preference to 
Guava's Objects.equal.  This Google+ post is related: 
https://plus.google.com/118010414872916542489/posts/TtFEKhU8qnG.

java.util.Objects.equals cannot be statically imported, due to Java language 
spec limitations on static imports combined with a poorly chosen name of 
'equals' (it's obviously already used, causing a conflict for static imports).

Without the static import, code size for this construct will increase by 180% 
(Guava equal - 5 characters; j.u.Objects.equals - 14 characters -> 9 extra 
characters).  These extra characters are uninteresting, adding no semantic or 
syntactical value.

An argument could be made to 'roll your own' (the code is trivial), however 
that has a hidden downside: many code analysis tools recognize Guava's 
Objects.equals and java.util.Objects.equals to be equivalent to x.equals(y) for 
the purposes of validating the correctness of the code.  

For example:

Integer integer = 5;
String string = "guava";
integer.equals(string); // caught as a likely bug
com.google.common.base.Objects.equal(integer,string); // caught as a likely bug
java.util.Objects.equals(integer,string); // caught as a likely bug
my.package.Objects.equals(integer,string); // not flagged as likely bug

Proposing that Objects.equal remain for reasons of code brevity/cleanliness, 
broad adoption (including by code analysis tools) and inertia.

Original issue reported on code.google.com by ch...@digitalascent.com on 19 Apr 2014 at 3:54

GoogleCodeExporter commented 9 years ago

Original comment by kak@google.com on 20 Apr 2014 at 5:00

GoogleCodeExporter commented 9 years ago
(Note that Guava's Objects.equal() is not deprecated, just discouraged.)

In general, I don't think that "exactly like a JDK method but can be 
static-imported" is enough justification for a Guava method to exist. When it 
*eventually* is deprecated, the best thing to do is just grumble about the JDK 
mistake and write out the "Objects.equal()".

Original comment by kevinb@google.com on 21 Apr 2014 at 6:32

GoogleCodeExporter commented 9 years ago
I'd agree if this was a *new* method - it wouldn't be justifiable to *add* it.  

However, the bar should be different for discouraging/deprecating established 
methods.

Guava's Objects.equal(), while (now) providing the same functionality as the 
JDK, is statically importable, which is valuable in reducing the noise in a 
code base (especially given the high frequency of use for this specific method).

Having to read ~2x the code for no additional value is a step backward.  Not 
concerned about typing it out, as IDE templates address this regardless of 
length - we spend considerably more time *reading/reviewing* code than writing 
it.

Original comment by ch...@digitalascent.com on 21 Apr 2014 at 6:44

GoogleCodeExporter commented 9 years ago
Applying a markedly different standard for whether something should be *added* 
compared to whether something should be *kept* is a recipe for accumulating a 
lot of cruft over the years. It isn't the Guava way.

As well, having two different library methods in existence that do the exact 
same thing is not a good thing. At best it leads to wasted time in fruitless 
debates between developers over minor concerns like static-importability. This 
also isn't the Guava way. We want to add real value on top of JDK libraries, 
and be seen as adding real value on top of JDK libraries, not as simply 
creating our own dialect.

Original comment by kevinb@google.com on 21 Apr 2014 at 8:51

GoogleCodeExporter commented 9 years ago
Code readability/brevity add real value on top of the JDK.

With Guava - 213 characters:
return equal( id, bean.id ) && equal( data, bean.data ) && equal( isActive, 
bean.isActive ) && equal( searchSynonym, bean.searchSynonym ) && equal( 
sequence, bean.sequence ) && equal( userAdded, bean.userAdded );

With j.u.Objects - 261 characters:
return Object.equals( id, bean.id ) && Object.equals( data, bean.data ) && 
Object.equals( isActive, bean.isActive ) && Object.equals( searchSynonym, 
bean.searchSynonym ) && Object.equals( sequence, bean.sequence ) && 
Object.equals( userAdded, bean.userAdded );

That's a ~22% increase in code size; the added code ('Objects.') is a 
distraction from reading/comprehending the executable logic.

Original comment by ch...@digitalascent.com on 21 Apr 2014 at 9:11

GoogleCodeExporter commented 9 years ago
Classes where all fields are nullable are rare (and *should* be even more rare 
than they are) yet you've zeroed in on that rare case for your example.

Moreover, you're talking about snipping mere *dozens* of characters from 
hand-written value-type classes -- which are *enormous* sources of mind-numbing 
boilerplate in Java. Why be content with such small savings; why not save way 
more with AutoValue (https://github.com/google/auto/tree/master/value), or 
something else like it?

We've already adequately explained that this character savings is NOT enough 
justification for us to change our plans for this method.

Original comment by kevinb@google.com on 21 Apr 2014 at 9:25

GoogleCodeExporter commented 9 years ago
> Classes where all fields are nullable are rare (and *should* be even more 
rare than they are) yet you've zeroed in on that rare case for your example.

Kevin, do you suggest to use equal for nullable fields only, i.e., to write 
something like

return equal(id, bean.id) && data.equals(bean.data) && equal(isActive, 
bean.isActive) && searchSynonym.equals(bean.searchSynonym) && ...

if some field were nullable and some not?

Original comment by Maaarti...@gmail.com on 23 Apr 2014 at 12:37