Add 'Hash#each_key_for' methods #5450

marksiemers · 2017-12-24T19:21:48Z

larubujo · 2017-12-24T19:51:48Z

src/hash.cr

+  # key # => "dos"
+  # ```
+  def each_key_for(value)
+    self.select { |k, v| v == value }.each_key


each.select { |(k, v)| v == value }.map &.[0]

raw select create copy of hash, slow and not accurate if hash change

Thanks, I'll make that change.

Any danger to using &.[0]?

RX14 · 2017-12-24T20:45:42Z

src/hash.cr

+  # key # => "dos"
+  # ```
+  def each_key_for(value)
+    each.select { |(k, v)| v == value }.map &.first


You should build a custom Iterator class for this. For example look at the valueiterator below.

i thought this first, but what need? this functionality not very needed, so if not super optimized fine too

@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.

@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.

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

marksiemers · 2017-12-25T02:17:06Z

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> .

marksiemers · 2017-12-25T05:16:25Z

@RX14 - LMK if this is what you had in mind.

asterite · 2018-01-02T13:39:26Z

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.

marksiemers · 2018-01-02T23:17:42Z

@asterite - It came from a discussion with @drosehn @bcardiff and a few others.
See here: #5248 (comment)

From @drosehn:

I certainly write a lot of hashes were the same value appears for multiple keys, and sometimes I do want to find all keys which match a given value. In those cases I loop through all keys to see if it matches the value I care about.

drosehn · 2018-01-02T23:41:27Z

For me, the main point of issue 5248 was that Hash#key? was a confusing name for what that method did. In one of the early responses to issue, @asterite wrote:

Ooh... Now I see the difference. Maybe key_for_value is a good name, or we could also leave it like that for now. It's a difference with Ruby but it doesn't matter

asterite · 2018-01-03T12:34:27Z

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.

# each_for_key
hash.each do |key, value|
  next unless value == target_value
end

# each_for_key (iterator)
each.select { |k, v| v == target_value }

I don't feel we need to include this custom logic in the standard library, and much less a custom iterator just for this.

RX14 · 2018-01-03T13:44:22Z

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.

marksiemers · 2018-01-26T18:55:53Z

@RX14 @asterite @bcardiff @drosehn
Do we want to make a decision on this and either merge or close?

drosehn · 2018-01-27T01:20:46Z

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.

marksiemers · 2018-01-30T04:38:10Z

@bcardiff - Any thoughts on this. each_key_for is a replacement for your originally suggested all_keys_for.

As I worked through it, each_key_for seemed to make more sense:

I've been working through this, and I think just having each_key_for will be a cleaner solution (and dropping all_keys_for):

Hash#each_key_for(value) : Iterator(K)
Hash#each_key_for(value, &block) : Nil
The second will yield to the block, and the first, you can call to_a to get an Array(K)

One less method and consistently singular with key_for

bcardiff · 2018-01-30T05:04:06Z

@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 each.select { |k, v| v == target_value }. I don't think there will be any performance difference.

Another alternative would be to take advantage of BaseIterator as the other hash iterators, but I believe the former should be enough.

WDYT?

marksiemers · 2018-01-30T07:56:01Z

@bcardiff - something similar was implemented in an earlier iteration of the PR.

It was changed to each.select { |(k, v)| v == value }.map &.first based on this comment from @larubujo :

raw select create copy of hash, slow and not accurate if hash change

See: dc82c69#r158608841

The iterator was added based on this comment from @RX14 :

You should build a custom Iterator class for this...more performance and more consistent with how the other iterators are implemented.

See: bb2904c#r158609466

It does use BaseIterator to an extent as it inherits from KeyIterator which includes BaseIterator. However, I don't think it is possible to use base_next to efficiently iterate for this functionality (since it is being filtered).

Add 'Hash#each_key_for' methods

dc82c69

larubujo reviewed Dec 24, 2017

View reviewed changes

Optimize 'Hash#each_key_for' based on review

bb2904c

RX14 requested changes Dec 24, 2017

View reviewed changes

marksiemers added 2 commits December 24, 2017 19:53

Switch 'Hash#each_key_for' to use a custom Iterator

82087c3

Fix whitespace formatting

8038f6d

RX14 approved these changes Dec 25, 2017

View reviewed changes

drosehn mentioned this pull request Jan 3, 2018

Rename 'Hash#key?' to make clear it's not an alias to Hash#has_key? #5248

Closed

bcardiff closed this Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Hash#each_key_for' methods #5450

Add 'Hash#each_key_for' methods #5450

marksiemers commented Dec 24, 2017

larubujo Dec 24, 2017

marksiemers Dec 24, 2017

RX14 Dec 24, 2017

larubujo Dec 24, 2017

marksiemers Dec 24, 2017

RX14 Dec 24, 2017

RX14 Dec 24, 2017

marksiemers commented Dec 25, 2017 via email

marksiemers commented Dec 25, 2017

asterite commented Jan 2, 2018

marksiemers commented Jan 2, 2018

drosehn commented Jan 2, 2018

asterite commented Jan 3, 2018

RX14 commented Jan 3, 2018

marksiemers commented Jan 26, 2018

drosehn commented Jan 27, 2018

marksiemers commented Jan 30, 2018

bcardiff commented Jan 30, 2018

marksiemers commented Jan 30, 2018 •

edited

Add 'Hash#each_key_for' methods #5450

Add 'Hash#each_key_for' methods #5450

Conversation

marksiemers commented Dec 24, 2017

larubujo Dec 24, 2017

Choose a reason for hiding this comment

marksiemers Dec 24, 2017

Choose a reason for hiding this comment

RX14 Dec 24, 2017

Choose a reason for hiding this comment

larubujo Dec 24, 2017

Choose a reason for hiding this comment

marksiemers Dec 24, 2017

Choose a reason for hiding this comment

RX14 Dec 24, 2017

Choose a reason for hiding this comment

RX14 Dec 24, 2017

Choose a reason for hiding this comment

marksiemers commented Dec 25, 2017 via email

marksiemers commented Dec 25, 2017

asterite commented Jan 2, 2018

marksiemers commented Jan 2, 2018

drosehn commented Jan 2, 2018

asterite commented Jan 3, 2018

RX14 commented Jan 3, 2018

marksiemers commented Jan 26, 2018

drosehn commented Jan 27, 2018

marksiemers commented Jan 30, 2018

bcardiff commented Jan 30, 2018

marksiemers commented Jan 30, 2018 • edited

marksiemers commented Jan 30, 2018 •

edited