New Performance/TimesMap cop

[DNNX]

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.

[bbatsov]

This should be a performance cop, not a lint cop IMO.

[DNNX]

@bbatsov yep, I agree. Will move it.

[alexdowad]

It would be better to squash these 3 commits into 1. There is no need to have a separate commit just to add a line to the changelog.

[DNNX]

@alexdowad sure. I'll squash them when everyone is happy with the changes. Any suggestions on phrasing, tests and everything are welcome :)

[DNNX]

@bbatsov I have relocated the cop to Performance

[alexdowad]

Use backticks to highlight the Ruby code (Array.new and .times.map).

(When we print out the offense messages, the backticks are removed and the highlighted area is shown in yellow.)

[alexdowad]

Aside from the minor point mentioned above, this looks good!

[DNNX]

@alexdowad I added backticks and squashed the commits together.

[DNNX]

...and rebased to resolve merge conflicts

[bbatsov]

Seems to me you could have generated all 3 tests for both collect and map, just to be on the safe side (and to avoid a bit of redundancy).

[DNNX]

@bbatsov I added more tests and also fixed the offense message (see my last commit). I leave the commit separate for now so that it is clear what I changed. If everything looks good, let me know - I'll go ahead and squash it into the previous commit.

[DNNX]

@alexdowad @bbatsov This PR is ready to merge too.

[kytrinyx]

Not all .times.map can be replaced with Array.new with a block.

E.g. this uses a starting date to create a series of date ranges going back in time:

r = new(d.year, d.month)                                                                                                          
n.times.map { r = prev(r) }  

I don't particularly want to discuss how good the above code is—I had my reasons for writing it. The main thing is that Rubocop probably shouldn't suggest that Array.new is more correct.

[bbatsov]

The main thing is that Rubocop probably shouldn't suggest that Array.new is more correct.

The point of the performance cops is speed, but your point is valid as well. Seems, however, this is the first problem related to the cop in 5 months. We can limit it just to numeric literals as receivers, but I guess this would reduce its overall usefulness significantly.

[kytrinyx]

We can limit it just to numeric literals as receivers

The problem isn't with n being a variable, but with the map taking the input from a prior iteration and using it to generate the next one.

I've disabled the cop in my project. I understand that I'm an outlier here.

[ixti]

@kytrinyx Sorry for necroposting, but can you explain how the snippet you've posted can't be replaced with Array.new?

r = new(d.year, d.month)
n.times.map { r = prev(r) }

# it's will result into the same as:

r = new(d.year, d.month)
Array.new(n) { r = prev(r) }

[kytrinyx]

I don't recall, @ixti. It might be fine like that, I can't remember which project I ran into this was in.

[ixti]

@kytrinyx thanks! :D Was just pretty curious.