rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
667 stars 78 forks source link

Improve `Performance/StringReplacement` cop #379

Open ydakuka opened 9 months ago

ydakuka commented 9 months ago

Description

Don’t use String#gsub in scenarios in which you can use a faster and more specialized alternative.

https://rubystyle.guide/#dont-abuse-gsub

https://docs.rubocop.org/rubocop-performance/cops_performance.html#performancestringreplacement

https://github.com/fastruby/fast-ruby/blob/main/code/string/gsub-vs-sub.rb

Behavior

url = 'http://example.com'

# bad
url.gsub('http://', 'https://')

# good
url.sub('http://', 'https://')
ydakuka commented 9 months ago

And the second case.

https://github.com/shopify/ruby-style-guide#strings

https://github.com/fastruby/fast-ruby/blob/main/code/string/gsub-vs-tr-vs-delete.rb (similar to this)

Behavior

str = 'lisp-case-rules'

# bad
str.gsub(/[aeiou]/, '')

# good
str.delete('aeiou')

I've benchmarked it with file.rb:

require 'benchmark/ips'

URL = 'lisp-case-rules'

def slow
  URL.gsub(/[aeiou]/, '')
end

def fast
  URL.delete('aeiou')
end

Benchmark.ips do |x|
  x.report('String#gsub')   { slow }
  x.report('String#delete') { fast }
  x.compare!
end

Ruby 3.3.0

ydakuka@yauhenid:~/ruby-docker-app$ docker run ruby-app  
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-linux]
Warming up --------------------------------------
         String#gsub    48.726k i/100ms
       String#delete   344.436k i/100ms
Calculating -------------------------------------
         String#gsub    462.242k (± 2.7%) i/s -      2.339M in   5.063481s
       String#delete      3.200M (± 2.7%) i/s -     16.188M in   5.062611s

Comparison:
       String#delete:  3200076.3 i/s
         String#gsub:   462242.4 i/s - 6.92x  slower
vlad-pisanov commented 9 months ago

sub and gsub aren't interchangeable; one can't be safely recommended over the other

'aaa'.sub('a', 'x')  # "xaa"
'aaa'.gsub('a', 'x') # "xxx"
ydakuka commented 9 months ago

I know, that's why my examples are given for unambiguous cases.

fatkodima commented 9 months ago

How rubocop can guess if you need to replace all occurrences or just one?

ydakuka commented 9 months ago

How rubocop can guess if you need to replace all occurrences or just one?

I meant that the args passed to #gsub are always 'http://' and 'https://', or vice versa. I agree now that I chose the very edge case for the first case.