fix: equals and hashcode of several classes #1364
Conversation
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
chrfwow
requested review from
askpt,
gruebel,
liran2000,
alexandraoberaigner and
guidobrei
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
|
| @@ -46,4 +46,46 @@ public Map<String, Object> asObjectMap() { | |||
| (accumulated, entry) -> accumulated.put(entry.getKey(), convertValue(entry.getValue())), | |||
| HashMap::putAll); | |||
| } | |||
|
|
|||
| public boolean equals(final Object o) { | |||
There was a problem hiding this comment.
why implement it instead of adding EqualsAndHashCode and adding inheritors with EqualsAndHashCode**(callSuper = true)** ?
Then it seems to call it on attributes map entries.
similar for others.
There was a problem hiding this comment.
Lombok's implementation has a bit more overhead, and as we can see from the issue, it is not always an implementation one would expect
There was a problem hiding this comment.
I read the issue. By solution of this, it may be suggested that Lombok's implementation is not useful here?
If their implementation works, widely used, and makes the methods redunadant, why not use it?
One thing I see misleading is that callSuper value ia false. But if you add the value as true, isn't it enough to make all the tests pass without explicit implementations?
There was a problem hiding this comment.
Agree with @liran2000 here, if we can configure lombok to work "as expected" then there is less code to maintain :)
There was a problem hiding this comment.
Ok. Do we want different inheritors of AbstractStructure and classes that delegate to it to equal each other if they have the same underlying AbstractStructure?
There was a problem hiding this comment.
I don't think they should be equal, else what's the point of the different classes?
| * @param o the other object | ||
| * @return true iff both objects are equal or represent the same data | ||
| */ | ||
| public boolean equals(final Object o) { |
There was a problem hiding this comment.
Here too, similar implementation to the EqualsAndHashCode by delombok.
There was a problem hiding this comment.
Do we want a Value object to have the same hashCode as its innerObject?
|
|
||
| @Test | ||
| void unequalMutableStructuresAreNotEqual() { | ||
| MutableStructure m1 = new MutableStructure(); |
There was a problem hiding this comment.
minor comment: the MutableStructure tests are in ImmutableStructureTest. Should they be moved into a separate class?
| @@ -46,4 +46,46 @@ public Map<String, Object> asObjectMap() { | |||
| (accumulated, entry) -> accumulated.put(entry.getKey(), convertValue(entry.getValue())), | |||
| HashMap::putAll); | |||
| } | |||
|
|
|||
| public boolean equals(final Object o) { | |||
There was a problem hiding this comment.
Agree with @liran2000 here, if we can configure lombok to work "as expected" then there is less code to maintain :)
|
Perhaps in a separate PR could we address the @tostring(callSuper = true) generates what we want |
This PR
Fixes some of the
equalsandhashcodemethods in the sdk.Related Issues
Fixes #1362
Notes
I assumed that for the classes I modified, two objects should be equal iff their underlying data is equal. E.g. the class
Value. Two distinctValueobjects should be equal if they represent the same data:Please comment if you think there are more such cases in the sdk, or if I added this behavior somewhere it is not appropriate.