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 Performance/StringReplacement cop #379

Open
ydakuka opened this issue Nov 4, 2023 · 5 comments
Open

Improve Performance/StringReplacement cop #379

ydakuka opened this issue Nov 4, 2023 · 5 comments

Comments

@ydakuka
Copy link
Contributor

ydakuka commented Nov 4, 2023

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
Copy link
Contributor Author

ydakuka commented Nov 8, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

ydakuka commented Nov 10, 2023

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

@fatkodima
Copy link
Contributor

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

@ydakuka
Copy link
Contributor Author

ydakuka commented Nov 25, 2023

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.

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

3 participants