[BEAM-389] Provide equals and hashCode in DelegateCoder #559
Conversation
peihe
force-pushed
the
fix-delegate-coder-equals
branch
from
c46f3bb
to
b2211c0
Compare
|
R: @kennknowles |
| return Objects.hashCode(this.coder, this.toFn, this.fromFn); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "DelegateCoder(" + coder + ")"; |
There was a problem hiding this comment.
Should toFn/fromFn be used here?
peihe
force-pushed
the
fix-delegate-coder-equals
branch
from
ad51960
to
677d821
Compare
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hashCode(this.coder, this.toFn.getClass(), this.fromFn.getClass()); |
There was a problem hiding this comment.
Just use toFn and fromFn. Their class may not be sufficient.
peihe
force-pushed
the
fix-delegate-coder-equals
branch
from
cb214e2
to
1232506
Compare
|
I also have to provide equals and hashCode in StringDelegateCoder and tests. |
| @@ -101,8 +104,28 @@ public Object structuralValue(T value) throws Exception { | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
| if (o == null || this.getClass() != o.getClass()) { | |||
There was a problem hiding this comment.
instanceof automatically removes the need for the null check
if (!(o instanceof DelegateCoder)) {
return false;
}
There was a problem hiding this comment.
Use instanceof will allow DelegateCoder equal to its subclass, which would break the symmetric property when its subclass override equals().
There was a problem hiding this comment.
Cool article.
I suggest that DelegateCoder is not intended for subclassing, so we could make it final. Then I think what Pei has is the same as what Luke suggests here. I think that we should actually move more towards the "different types cannot be equal" perspective throughout the SDK, unless those two types are both defined to be just implementations of a common interface, and then the interface should own the definition of equality.
There was a problem hiding this comment.
+1 @kennknowles. Relatedly, I think most classes should either be abstract or final.
The only extender of this class is StringDelegateCoder, and it's only used in Tfidf (and clones of Tfidf). We could replace it with a proper implementation using DelegateCoder instead, or StringDelegateCoder could wrap it instead.
peihe
force-pushed
the
fix-delegate-coder-equals
branch
2 times, most recently
from
4c4022a
to
634f4e8
Compare
|
PTAL |
| @@ -26,6 +27,8 @@ | |||
| import java.util.Collection; | |||
| import java.util.List; | |||
|
|
|||
| import autovalue.shaded.com.google.common.common.base.MoreObjects; | |||
peihe
force-pushed
the
fix-delegate-coder-equals
branch
from
634f4e8
to
7700250
Compare
PiperOrigin-RevId: 503475285
No description provided.