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!
| 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.
There was a problem hiding this comment.
Hi Baiy,
Thanks for the renaming.
About the ordering, you indeed confirm that "the inherent nature of the map data structure means it does not care about ordering...".
I understand. But I do think it creates a design smell. Now, if you ask for the values associated with a certain slice of keys you will receive a slice of values that cannot one by one be associated to the keys you passed. I think the return slice is dubious in how it relates to the the input leading to confusion and possibly code that is difficult to understand.
Per se, not an error, but I would not like to use such an unclear interface.
If you want to continue with the behaviour, that is ok, but I would appreciate if you could evaluate the suggestions given before.
| 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. |
Hi Duke, there are no errors or bugs, but I really think the behavior of some functions (see above) could be less ambiguous. I am not sure that baiy agrees. The suggestions I gave are still open, but I am ok if the decision is not to implement them. If there are good reasons for not implementing the suggestions it would be nice for me to know -- so I can learn the motivations behind. Check if you want to proceed or to ask Baiy for a further evaluation. |
Hi idichekop, thanks for your detailed feedback and patience. After reviewing your suggestions again, I agree that optimizing the function behavior to make the input-output mapping clearer is necessary. It will effectively avoid confusion and improve code readability. |
3 participants
feat: add BiMap for maputil