Support equals() for IntOrString#283
Conversation
| return this == o || (o instanceof IntOrString && equals((IntOrString) o)); | ||
| } | ||
|
|
||
| private boolean equals(IntOrString o) { |
There was a problem hiding this comment.
minor nit, but is there value in making this a separate function?
Why not:
if (this == o) {
return true;
}
if (! o instanceof IntOrString) {
return false;
}
IntOrString other = (IntOrString) o;
...
}There was a problem hiding this comment.
I use this approach because I find it easier to read. The approach you show uses seven lines of code to accomplish what the submitted change does in one. Extra functions can act as a very simple way of documenting functionality (see Robert Martin's "Clean Code"), and get optimized away at runtime.
But my goal is just getting this fixed so that my project doesn't need a workaround. if using my approach would hold up the pull request, I am perfectly happy to see it changed.
There was a problem hiding this comment.
I don't feel strongly enough about this to have you correct it (though overally I think my suggestion is shorter once you consider the lines to define the new function..)
I'm merging as-is.
|
Thanks for the PR, one minor nit... |
Implemented equals() and hash() for the IntOrString class. Unit test included