KEMBAR78
Add Hash#key? vs. Hash#keys.include? and Hash#value? vs. Hash#values.include? by JacobEvelyn · Pull Request #154 · fastruby/fast-ruby · GitHub
Skip to content

Conversation

@JacobEvelyn
Copy link
Contributor

No description provided.

@nirvdrum
Copy link
Collaborator

Thanks for the contribution. But is this something that people do in practice? I don't think I've ever encountered someone calling keys.include?. It's usually a matter of Hash#key? vs Hash#[].

@utilum
Copy link

utilum commented Apr 28, 2018

https://github.com/search?utf8=%E2%9C%93&q=%22keys%5C.include%5C%3F%22+extension%3Arb+language%3ARuby&type=Code&ref=advsearch&l=Ruby&l=

@JacobEvelyn
Copy link
Contributor Author

Our team is a group of pretty experienced Ruby developers with a strict code review policy, and I noticed both of these patterns appear in our codebases. I suspect we're not alone.

I actually made this PR because I caught myself writing values.include? which isn't obviously slower than value?, which is why I looked at the Ruby implementation and then tested it to confirm my suspicion that it was.

@kbrock
Copy link
Contributor

kbrock commented May 1, 2018

There sure are a lot of ips blocks there.
I get the whole hit and miss use cases. (I did this recently while testing) - I just wonder if this is overkill for this project?

Also, I'm amused that Hash[key] vs Hash.key?(key) is not there. That is how I usually check for presence.

I too am surprised people use Hash.keys.include?(key) - so many allocations and all.

@ixti
Copy link
Collaborator

ixti commented May 1, 2018

Can you please split that into 2 sections: one for keys and the other for values - IMO it will look much more readable.

@JacobEvelyn
Copy link
Contributor Author

@ixti thanks for the suggestion; done.

I also added tests for !!Hash[key] as @kbrock suggested, though I go back and forth on whether the !! should be in the test or not. I'm also not totally sure that Hash[key] should be in here at all, since it's not really a presence check—if the value is falsey the presence check will fail.

Thoughts?

@ixti
Copy link
Collaborator

ixti commented May 1, 2018

I recommend avoid having Hash#[](key) test in here to avoid confusion. In some cases, Hash#[] is fine for such tests like:

return unless RANGES[key]

But it may lead to confusion:

data = { :foo => nil, :bar => false, :baz => 1 }

%i[foo bar baz].map { |k| data.key? k } # => [true, true, true]
%i[foo bar baz].map { |k| data[k] ? true : false } # [false, false, true]

@ixti
Copy link
Collaborator

ixti commented May 1, 2018

And after all Hash#[] returns value, so !!data[k] testing value truthiness rather than presence of key. :D

@JacobEvelyn
Copy link
Contributor Author

Oof, you're right @ixti—can't believe I missed that !! issue. I've removed Hash#[] from this PR.

Copy link
Collaborator

@ixti ixti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ixti ixti merged commit db5274c into fastruby:master May 2, 2018
@ixti
Copy link
Collaborator

ixti commented May 2, 2018

Thank you!

@JacobEvelyn JacobEvelyn deleted the inefficient-hash-search branch May 2, 2018 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants