feat: add BiMap for maputil #329
Conversation
| 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).
| 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).
feat: add BiMap for maputil