Rubocop: New cop to suggest replacing case/when with hash lookup

[fatkodima]

When case/when represents a simple 1:1 mapping, suggest to replace it with a hash lookup.

# bad
case country
when "europe"
  "http://eu.example.com"
when "america"
  "http://us.example.com"
end

# good
SITES = {
  "europe"  => "http://eu.example.com",
  "america" => "http://us.example.com"
}
SITES[country]

I'm propose to name it Style/CaseHashMapping (is there a better name?). Maybe it would be wise to add a Min config option, specifying for how many branches should be to consider an offense.

@bbatsov Interested in your thoughts on this.

[bbatsov]

I love the idea! As for the name -> something like HashLikeCase or StaticMappingViaCase sound reasonably good to me, although not great. Naming is hard! :laughing:

[fatkodima]

Cool, will implement it then 😄

[marcandre]

Should this be in rubocop-performance? Or is there another goal here?
Will that be limited to string values?

[fatkodima]

Keys can be strings and symbols, values can be any [recursive]_basic_literal.
Yes, it will be faster, but the main goal is to make the code easier. So I believe this should go to the Style department here.

[koic]

Hm. Intuitively, this rule seems to belong to Performance department. I think Style can provide the opposite option, but it doesn't seem to be the case.

Also, I think that conversion to Hash constant is limited only when Hash values have the same type.

[tejasbubane]

The correction can be without variable:

{
  "europe"  => "http://eu.example.com",
  "america" => "http://us.example.com"
}[country]

[rrosenblum]

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 performance rather than style. Do we have any metrics to back this up?

[marcandre]

The correction can be without variable:

{
  "europe"  => "http://eu.example.com",
  "america" => "http://us.example.com"
}[country]

Building a hash is expensive, this is a bad idea

[fatkodima]

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

Comparison:
       constant hash:  4367232.1 i/s
           case-when:  4263872.0 i/s - same-ish: difference falls within error
         inline hash:  3369430.3 i/s - 1.30x  (± 0.00) slower

Still looks like a stylistic cop to me.

[mame]

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.