-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'Hash#each_key_for' methods #5450
Add 'Hash#each_key_for' methods #5450
Conversation
src/hash.cr
Outdated
# key # => "dos" | ||
# ``` | ||
def each_key_for(value) | ||
self.select { |k, v| v == value }.each_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each.select { |(k, v)| v == value }.map &.[0]
raw select
create copy of hash, slow and not accurate if hash change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll make that change.
Any danger to using &.[0]
?
src/hash.cr
Outdated
# key # => "dos" | ||
# ``` | ||
def each_key_for(value) | ||
each.select { |(k, v)| v == value }.map &.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should build a custom Iterator class for this. For example look at the valueiterator below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought this first, but what need? this functionality not very needed, so if not super optimized fine too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RX14 and @larubujo
For moving to a custom Iterator class, what advantages does it provide?
- Better performance
- More reliable
- More readable
- More reusable
It will be very similar to KeyIterator(K, V)
, from what I can tell.
Entry#fore
may need to be overridden, unless there is a way to write KeyForValueIterator#next
that I haven't figured out yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marksiemers more performance and more consistent with how the other iterators are implemented.
It wouldn't be hard, given the BaseIterator
module and how the other iterators are implemented. It should in fact be only 3-4 lines in the def next
impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually my bad, it can't be done with base_next
.
def next
current = @current
while current
if current.value == @value
@current = current.fore
return current.key
else
current = current.fore
end
end
@current = current
stop
end
Looks good. I can move that into the implementation unless you want your
name on the commit.
I'm glad to see we don't have to touch fore.
Not sure why I didn't work with a while loop before. I had in my head that
linear time could be avoided for some reason.
Thanks for the solution.
…On Dec 24, 2017 3:35 PM, "Chris Hobbs" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/hash.cr
<#5450 (comment)>:
> +
+ # Returns an iterator over the keys with a value matching the input value.
+ # Which behaves like an `Iterator` consisting of the key's types.
+ #
+ # ```
+ # h = {"one" => 1, "two" => 2, "dos" => 2}
+ # iterator = h.each_key_for(2)
+ #
+ # key = iterator.next
+ # key # => "two"
+ #
+ # key = iterator.next
+ # key # => "dos"
+ # ```
+ def each_key_for(value)
+ each.select { |(k, v)| v == value }.map &.first
Actually my bad, it can't be done with base_next.
def next
current = @current
while current
if current.value == @value
@current = current.fore
return current.key
else
current = current.fore
end
end
@current = current
stopend
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5450 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABPrX7liwfcmZRuQwoKwzZaNs4aHtpy5ks5tDt-mgaJpZM4RL_01>
.
|
@RX14 - LMK if this is what you had in mind. |
Do we really need this method? What's the use case? There's even an iterator here! It seems super specific... I'm not sure such code belongs here. It's not even in Ruby's std. |
@asterite - It came from a discussion with @drosehn @bcardiff and a few others. From @drosehn:
|
For me, the main point of issue 5248 was that
|
I personally can't approve this. It's not general enough to include it in the standard library, and it's so easy to implement outside of it.
I don't feel we need to include this custom logic in the standard library, and much less a custom iterator just for this. |
I disagree. I think it's a nice addition for completeness's sake, and there's going to be little if any change to the implementation of it, ever. If there's a usecase, I don't see why not. |
I like the idea, but I'm not one to say whether it is useful enough to make it part of the stdlib. But at the very least, I suspect I'll refer back to the code in this pull request whenever I want a feature like this in my own code. |
@bcardiff - Any thoughts on this. As I worked through it,
|
@marksiemers sorry for the delay. I like the api . From @asterite last comment I think we could get rid of the custom iterator implementation though, but keep both methods. So the implementation can be along Another alternative would be to take advantage of WDYT? |
@bcardiff - something similar was implemented in an earlier iteration of the PR. It was changed to
See: dc82c69#r158608841 The iterator was added based on this comment from @RX14 :
See: bb2904c#r158609466 It does use |
#5248