Skip to content
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

Improve the Style/SelectByRegexp cop #12459

Open
ydakuka opened this issue Dec 3, 2023 · 4 comments
Open

Improve the Style/SelectByRegexp cop #12459

ydakuka opened this issue Dec 3, 2023 · 4 comments

Comments

@ydakuka
Copy link

ydakuka commented Dec 3, 2023

Describe the solution you'd like

I have the following code:

array.select { |x| x.match?(/regexp/) }

If I run the command rubocop -A --only Style/SelectByRegexp I will get:

array.grep(/regexp/)

However, if I run the command rubocop -A --only Performance/StringInclude I will get:

array.select { |x| x.include?('regexp') }

So I conclude that these onelines do the same thing.

I propose (see benchmarks for more information):

# bad
array.select { |x| x.include?('regexp') }
array.reject { |x| x.include?('regexp') }

# good
array.grep(/regexp/)
array.grep_v(/regexp/)
@ydakuka
Copy link
Author

ydakuka commented Dec 3, 2023

I have the Dockerfile:

FROM ruby:3.3.0-preview3
COPY . /var/www/ruby  
WORKDIR /var/www/ruby  
RUN gem install benchmark-ips
RUN ruby -v
CMD ["ruby", "file.rb"]

@ydakuka
Copy link
Author

ydakuka commented Dec 3, 2023

1 case

require "benchmark/ips"

ARRAY = ("a".."zzz").to_a.shuffle.freeze

def initial
  ARRAY.select { |x| x.match?(/regexp/) }
end

def slow
  ARRAY.select { |x| x.include?('regexp') }
end

def fast
  ARRAY.grep(/regexp/)
end

Benchmark.ips do |x|
  x.report('initial') { initial }
  x.report('Array#select#include') { slow }
  x.report('Array#grep') { fast }
  x.compare!
end

Output:

ydakuka@yauhenid:~/ruby-docker-app$  docker run ruby-app  
Warming up --------------------------------------
             initial    60.000  i/100ms
Array#select#include    72.000  i/100ms
          Array#grep    95.000  i/100ms
Calculating -------------------------------------
             initial    582.673  (± 4.8%) i/s -      2.940k in   5.058520s
Array#select#include    777.983  (± 1.4%) i/s -      3.960k in   5.091249s
          Array#grep    966.763  (± 2.5%) i/s -      4.845k in   5.015105s

Comparison:
          Array#grep:      966.8 i/s
Array#select#include:      778.0 i/s - 1.24x  slower
             initial:      582.7 i/s - 1.66x  slower

@ydakuka
Copy link
Author

ydakuka commented Dec 3, 2023

2 case

require "benchmark/ips"

ARRAY = ("a".."zzz").to_a.shuffle.freeze

def initial
  ARRAY.reject { |x| x.match?(/regexp/) }
end

def slow
  ARRAY.reject { |x| x.include?('regexp') }
end

def fast
  ARRAY.grep_v(/regexp/)
end

Benchmark.ips do |x|
  x.report('initial') { initial }
  x.report('Array#reject#include') { slow }
  x.report('Array#grep_v') { fast }
  x.compare!
end

Output:

ydakuka@yauhenid:~/ruby-docker-app$  docker run ruby-app  
Warming up --------------------------------------
             initial    42.000  i/100ms
Array#reject#include    44.000  i/100ms
        Array#grep_v    76.000  i/100ms
Calculating -------------------------------------
             initial    504.497  (± 6.5%) i/s -      2.520k in   5.020388s
Array#reject#include    637.544  (± 8.0%) i/s -      3.168k in   5.025849s
        Array#grep_v    792.533  (± 2.4%) i/s -      4.028k in   5.085471s

Comparison:
        Array#grep_v:      792.5 i/s
Array#reject#include:      637.5 i/s - 1.24x  slower
             initial:      504.5 i/s - 1.57x  slower

@ydakuka ydakuka changed the title [WIP] Improve the Style/SelectByRegexp cop Improve the Style/SelectByRegexp cop Dec 3, 2023
@fatkodima
Copy link
Contributor

I think this should be implemented as a separate cop. ARRAY.grep(/regexp/) should be really ARRAY.grep('regexp'), because grep/grep_v also supports strings, ranges, classes etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants