equals implemented without hashCode

[dclements]

There are several classes where we implement equals without implementing hashCode, this is almost always a bad practice since it violates the Object contract (Effective Java, Item 8). In some cases we use these objects in hash-based objects, which makes them especially critical to add hashCode methods to if they override equals.

Override equals but not hashCode and are used in a hashed collection:

• com.readytalk.revori.Aggregate
• com.readytalk.revori.ColumnReference
• com.readytalk.revori.Parameter

Override equals but not hashCode:

• com.readytalk.revori.BinaryOperation
• com.readytalk.revori.Constant
• com.readytalk.revori.imp.MyRevision
• com.readytalk.revori.Join
• com.readytalk.revori.QueryTemplate
• com.readytalk.revori.TableReference
• com.readytalk.revori.UnaryOperation

[mgodave]

Use Guava's Objects class to help with these implementations where appropriate.

[joshuawarner32]

For these, we should use the Guava implementation with care - unless we can be sure that hotspot will optimize it away (I don't know either way). Some of these will be pretty frequently used.

[dclements]

I'm generally of the mind that you do it the clearest way first, worry about minor gains after profiling indicates it is a problem. So let's use Guava since it is already integrated, then try alternatives if and only if we see a problem.

[mgodave]

I am not sure I get what you are talking about. What exactly would be optimized away?

It's a matter of correctness. If you're going to implement one of: {hashCode, equals} then you need to make sure you also implement the other. I also agree with David, implement the obviously correct implementation and once you can prove a meaningful performance hit then optimize.

There may be obvious implementations, in that case invoke the "where appropriate" clause of my comment.

[joshuawarner32]

mgodave: basically, I was wondering whether hotspot would end up properly inlining the Guava Objects calls.
dclements: Normally, I'd be a fan of using a simpler API - but it's really not hard to write a good hashCode method. I'd rather make good performance decisions now (unless it's a significant imposition), than be cleaning up a thousand little performance problems later - that alone, don't show up in a profile, but together drag the system down.

[plytro]

HashCode and equals are pretty boilerplate type functionality. Maybe look into using this lombok for most cases and roll your own when what it does isn't good enough.

http://projectlombok.org/features/EqualsAndHashCode.html

[mgodave]

I'm not sure why it would be any less likely to inline a call to Guava.

Good hashcode implementations are actually non-trivial. Along the lines of performance decisions, I would argue that architecture and design at this point would make the biggest difference. If your object is immutable, cache the hashcode. If you object maybe should be immutable, then refactor and make the aforementioned change. Again, go the obviously correct route then optimize when you can prove a performance penalty! equals and hashcode should be the least of your performance worries at this point, especially since there is no empirical data to show they affect the critical path.

[dclements]

@plytro I'd generally agree, but in this case it also has to play nice with compareTo, which has some nuance (e.g., they compare method results off of fields, rather than comparing the fields directly). This may be a design problem in itself, but it makes a drop-in like that not particular trivial. Based on the current code base and how compareTo is being implemented, we'd end up implementing our own in almost all cases.