Why does RuboCop suggest replacing .times.map with Array.new?

RuboCop suggests:

Use Array.new with a block instead of .times.map.

In the docs for the cop:

This cop checks for .times.map calls. In most cases such calls can be replaced with an explicit array creation.

Examples:

# bad
9.times.map do |i|
  i.to_s
end

# good
Array.new(9) do |i|
  i.to_s
end

I know it can be replaced, but I feel 9.times.map is closer to English grammar, and it's easier to understand what the code does.

Why should it be replaced?

Solution 1:

The latter is more performant; here is an explanation: Pull request where this cop was added

It checks for calls like this:

9.times.map { |i| f(i) }
9.times.collect(&foo)

and suggests using this instead:

Array.new(9) { |i| f(i) }
Array.new(9, &foo)

The new code has approximately the same size, but uses fewer method calls, consumes less memory, works a tiny bit faster and in my opinion is more readable.

I've seen many occurrences of times.{map,collect} in different well-known projects: Rails, GitLab, Rubocop and several closed-source apps.

Benchmarks:

Benchmark.ips do |x|
  x.report('times.map') { 5.times.map{} }
  x.report('Array.new') { Array.new(5){} }
  x.compare!
end
__END__
Calculating -------------------------------------
           times.map    21.188k i/100ms
           Array.new    30.449k i/100ms
-------------------------------------------------
           times.map    311.613k (± 3.5%) i/s -      1.568M
           Array.new    590.374k (± 1.2%) i/s -      2.954M

Comparison:
           Array.new:   590373.6 i/s
           times.map:   311612.8 i/s - 1.89x slower

I'm not sure now that Lint is the correct namespace for the cop. Let me know if I should move it to Performance.

Also I didn't implement autocorrection because it can potentially break existing code, e.g. if someone has Fixnum#times method redefined to do something fancy. Applying autocorrection would break their code.

Solution 2:

If you feel it is more readable, go with it.

This is a performance rule and most codepaths in your application are probably not performance critical. Personally, I am always open to favor readability over premature optimization.

That said

100.times.map { ... }

• times creates an Enumerator object
• map enumerates over that object without being able to optimize, for example the size of the array is not known upfront and it might have to reallocate more space dynamically and it has to enumerate over the values by calling Enumerable#each since map is implemented that way

Whereas

Array.new(100) { ... }

• new allocates an array of size N
• And then uses a native loop to fill in the values

Solution 3:

When you need to map the result of a block invoked a fixed amount of times, you have an option between:

Array.new(n) { ... }

and:

n.times.map { ... }

The latter one is about 60% slower for n = 10, which goes down to around 40% for n > 1_000.

Note: logarithmic scale!