Cop idea: combine chained `select` / `reject` calls

[vlad-pisanov]

Similar to how Performance/MapMethodChain combines chained map calls, this cop would combine multiple select / reject calls into a single block expression.

# bad
list.select(&:odd?).select(&:positive?)

# good
list.select { |x| x.odd? && x.positive? }

Benchmark:

require 'benchmark/ips'
data = Array.new(1000) { rand(-100..100) }

Benchmark.ips do |x|
  x.report('chain') { data.select(&:odd?).select(&:positive?) }
  x.report('block') { data.select { |x| x.odd? && x.positive? } }
  x.compare!
end

Comparison:
               block:    14184.4 i/s
               chain:    10588.1 i/s - 1.34x  slower

[koic]

It's a difficult decision, but I'm wondering if it could be possible to propose this to the Ruby Style Guide, and if the proposal passes, place it in RuboCop core with a name like Style/CombineFilterConditions. Because unlike Performance/MapMethodChain cop, there don't seem to be any concerns regarding the combination of blocks. What do you think?

[vlad-pisanov]

Hm, I could see that being a style cop, too.

Especially when Ruby3.4's it comes out, something like

select(&:big?).select(&:yellow?).reject(&:sweet?)

could be autocorrected to

select { it.big? && it.yellow? && !it.sweet? }

which is arguably more legible

[koic]

Personally, I find it acceptable, but I’m not sure how widely it will be accepted. So, some people may still prefer to consistently use named block variables. Since style rules can be a source of debate, I think it may not be appropriate to provide autocorrect for this.

[Earlopain]

MapMethodChain also does not provide autocorrect. Personally I'm not a fan of it yet but that may just be because it is so new. I disliked numbered parameters first as well but now I often use them when writing code I know I will change/remove/etc. anyways. Still, both not something I would write in final code. I will just go with select { |x| x.big? && x.yellow? && !x.sweet? }

[koic]

Yep, since Ruby is a scripting language, having simplified syntax available can sometimes be convenient for users who want to write scripts. However, whether it's appropriate to use that in maintainable code should be left to the user’s discretion. Personally, I find it much more acceptable than _1, but even so, evaluating the usage of it will likely take some time.

[vlad-pisanov]

maybe it could be a configuration, something like

AllCops:
  AutocorrectAnonymousBlockVar: it # or _1 or false to disable autocorrect

for cops like MapMethodChain and others that need to introduce a block

[koic]

AFAIK, autocorrection is fundamentally a supplementary tool, but it is common for autocorrected code to be reconized as correct by users. This means that cases where a named block variable is preferable could end up being replaced by it. The issue here is that autocorrection carries the risk of causing users to stop thinking critically. Moreover, if it is already defined as a local variable, forcing the use of the it parameter would not behave as expected. For example, in it = 42; [1, 2, 3].each { p it }, such a case cannot be autocorrected to use it anyway.

For these reasons, I still remain doubt about introducing an option that enforces autocorrection to the it parameter.