feat: add BiMap for maputil #329
Conversation
idichekop
left a comment
idichekop
left a comment
There was a problem hiding this comment.
Thank you Baiy for the nice feature! Some suggestions for more intuitive behaviour of Keys() and Values() in the review!
maputil/bimap.go
Outdated
| return m.normal[k] | ||
| } | ||
|
|
||
| // Keys returns a slice of query keys |
There was a problem hiding this comment.
Hi Bayi,
First of all, thank you for the nice and clean code. Also with the good thought on safety (locks).
Here a kind of reflection on your design and implementation for the Keys() and Values() function.
I wonder that these functions do NOT preserve the order of the returned keys corresponding to the order to which the values are given in the input. Is that true?
I see that the tests for the function for example, only test for the existence of each key in the returned set, but not to their order.
I suggest to have the order of the returned slice corresponding to the order of the input slice. This is much more intuitive and allows the caller to know for each key returned whose value it matches to.
Moreover: I also guess that, for this implementation, if you pass as input some value(s) (or key in the Values func) that are not in the BiMap, the return slice will contain only keys(values) for the found elements. This makes it impossible to know the correspondence between input and output values. That is, for which value(key) in the input, the returned key(value) is associated at the output. This would be true even if the order would be preserved -- because there is no indication of which elements are in the BiMap or not.
That all explained, I have two suggestions:
- Change the implementation, so that the order of the returned elements correspond to the order of the given elements
- Return an error if one of the input elements cannot be found in the BiMap as a value(in the case of the Keys func) or as a key (in the case of the Values func).
There was a problem hiding this comment.
These two methods have been renamed. Fundamentally, they are wrappers for maputil.FilterByKeys and maputil.FilterByValues, so it is inappropriate to claim that there are errors. Regarding the sorting issue, the inherent nature of the map data structure means it does not care about ordering, so there is no need to support sorted output.
maputil/bimap_test.go
Outdated
| assert.Equal(false, biMap.ContainsValue(4)) | ||
|
|
||
| assert.Equal(2, len(biMap.Keys(1, 2))) | ||
| assert.Equal(true, slices.Contains(biMap.Keys(1, 2), "one")) |
There was a problem hiding this comment.
These tests do not include checking if order of returned slice corresponds to order of input slice.
The test in line 33 (Equal(2, len(biMap.Keys(1,2)) only covers the case when the given elements exist. Tests should also include what should be the behaviour when some of the input parameters are not in the BiMap -- should that issue an error (preferred), or return a subset (less optimal).
|
@idichekop , Hi idichekop. I noticed PR #328 and #329 are pending to open status. Do these two PRs meet the requirements for merging? I plan to release version v2.3.8 this Thursday (CST, October 16), which will include all changes made in the past three months. |
3 participants
feat: add BiMap for maputil