fix: Minor QoL clean up #175
Conversation
Also reflects the equals() change to hashCode() and adds tests for both implementations.
|
|
||
| Ref<?> valueRef = (Ref<?>) o; | ||
| if (val.getClass() != o.getClass()) return false; | ||
|
|
||
| return id.equals(valueRef.id) && val.equals(valueRef.val); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return id.hashCode(); | ||
| return Objects.hash(id, val); |
There was a problem hiding this comment.
I am not sure we want this change. The value here can be a string or a container type (we seem to be using maps as T), and if it is a container type we'll end up doing a deep traversal for equals() and hashCode() as opposed to a simple integer comparison / hashing which can become bad for performance.
Semantically, ids should be unique and generated based on a similar (and somewhat expensive) map lookup, so we should be fine with using the id only.
@korniltsev can you confirm this?
There was a problem hiding this comment.
In 32f2f77 I reverted this change. I did this in the interest of keeping the changes in this PR non-controversial. I mostly wanted this PR to clean up a few untidy spots in the code base.
There was a problem hiding this comment.
@korniltsev can you confirm this?
I confirm the id already represents the contents of the value which is uniquely stored in a mp Aleks pointed. It will not hurt to hash/equals the value, but it is also not needed.
| @@ -16,7 +16,8 @@ public Ref(T val, Long id) { | |||
| public boolean equals(Object o) { | |||
| if (this == o) return true; | |||
| if (o == null || getClass() != o.getClass()) return false; | |||
| Ref<T> valueRef = (Ref<T>) o; | |||
|
|
|||
| Ref<?> valueRef = (Ref<?>) o; | |||
There was a problem hiding this comment.
Note this uses ? instead of T. I gather that ? is somewhat safer when casting generic classes.
This PR performs some housekeeping tasks like removing unused imports and removing unused return values.