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
New cop to suggest replacing case/when with hash lookup #8247
Comments
I love the idea! As for the name -> something like |
Cool, will implement it then 😄 |
Should this be in |
Keys can be strings and symbols, values can be any [recursive]_basic_literal. |
Hm. Intuitively, this rule seems to belong to Also, I think that conversion to Hash constant is limited only when Hash values have the same type. |
The correction can be without variable: {
"europe" => "http://eu.example.com",
"america" => "http://us.example.com"
}[country] |
The inline version that @tejasbubane suggested would be the easiest to implement with auto-correct. I think extracting a constant to the correct location within the file will prove to be tricky. However, I do think using a hash would be a better solution. I also agree with others that this sounds like a good candidate for |
Building a hash is expensive, this is a bad idea |
Benchmark# frozen_string_literal: true
require 'bundler/inline'
gemfile(true) do
gem 'benchmark-ips'
end
SITES = {
"europe" => "http://eu.example.com",
"america" => "http://us.example.com",
"australia" => "http://au.example.com"
}
def case_when(country)
case country
when "europe"
"http://eu.example.com"
when "america"
"http://us.example.com"
when "australia"
"http://au.example.com"
end
end
def constant_hash(country)
SITES[country]
end
def inline_hash(country)
{
"europe" => "http://eu.example.com",
"america" => "http://us.example.com",
"australia" => "http://au.example.com"
}[country]
end
Benchmark.ips do |x|
x.report('case-when') { case_when(SITES.keys.sample) }
x.report('constant hash') { constant_hash(SITES.keys.sample) }
x.report('inline hash') { inline_hash(SITES.keys.sample) }
x.compare!
end Results
Still looks like a stylistic cop to me. |
Just FYI: As @fatkodima's benchmark suggests, MRI applies the similar optimization internally. For every trivial case/when statement, the interpreter builds an internal hash from literal keys to bytecode addresses at the compilation, and lookups the hash and just jumps during execution. So, I think this is just a matter of coding styles. |
But why is a hash considered better style than a case? I personally find the case style much more readable, but what am I missing? |
Well, there's always subjectivity but if something is essentially a dictionary-structure it's usually a good idea to represent it as such (meaning a hash in Ruby). For me this reduces the cognitive overhead. |
That's a little bit tautological, but it seems you aren't alone in feeling that way :-) luckily I can always turn it off. |
When
case/when
represents a simple 1:1 mapping, suggest to replace it with a hash lookup.I'm propose to name it
Style/CaseHashMapping
(is there a better name?). Maybe it would be wise to add aMin
config option, specifying for how many branches should be to consider an offense.@bbatsov Interested in your thoughts on this.
The text was updated successfully, but these errors were encountered: