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

Add new Performance/DeletePrefix and Performance/DeleteSuffix cops #105

Merged

Conversation

koic
Copy link
Member

@koic koic commented May 6, 2020

This PR adds new Performance/DeletePrefix and Performance/DeleteSuffix cops.

In Ruby 2.5, String#delete_prefix and String#delete_suffix have been added.

Performance/DeletePrefix cop identifies places where gsub(/\Aprefix/, '') can be replaced by delete_prefix('prefix').

# bad
str.gsub(/\Aprefix/, '')
str.gsub!(/\Aprefix/, '')
str.gsub(/^prefix/, '')
str.gsub!(/^prefix/, '')

# good
str.delete_prefix('prefix')
str.delete_prefix!('prefix')

The delete_prefix('prefix') method is faster than gsub(/\Aprefix/, '').

% ruby -v
ruby 2.5.8p224 (2020-03-31 revision 67882) [x86_64-darwin17]

% cat bench.rb
require 'benchmark/ips'

Benchmark.ips do |x|
  str = 'foobar'

  x.report('gsub')           { str.gsub(/bar\z/, '') }
  x.report('gsub!')          { str.gsub(/bar\z/, '') }
  x.report('delete_suffix')  { str.delete_suffix('bar') }
  x.report('delete_suffix!') { str.delete_suffix('bar') }
  x.compare!
end

% ruby bench.rb
Warming up --------------------------------------
                gsub    46.814k i/100ms
               gsub!    46.896k i/100ms
       delete_suffix   211.337k i/100ms
      delete_suffix!   208.332k i/100ms
Calculating -------------------------------------
                gsub    546.500k (± 1.3%) i/s -      2.762M in 5.054918s
               gsub!    551.054k (± 1.2%) i/s -      2.767M in 5.021747s
       delete_suffix      4.780M (± 1.1%) i/s -     24.092M in 5.040850s
      delete_suffix!      4.770M (± 1.1%) i/s -     23.958M in 5.022823s

Comparison:
       delete_suffix:  4780060.8 i/s
      delete_suffix!:  4770419.3 i/s - same-ish: difference falls within error
               gsub!:   551054.2 i/s - 8.67x  slower
                gsub:   546500.1 i/s - 8.75x  slower

Performance/DeleteSuffix cop is the same.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@koic koic force-pushed the add_new_delete_prefix_and_delete_suffix_cops branch from 137a66c to c84965c Compare May 6, 2020 18:28
This PR adds new `Performance/DeletePrefix` and `Performance/DeleteSuffix` cops.

In Ruby 2.5, `String#delete_prefix` and `String#delete_suffix` have been added.

- https://ruby-doc.org/core-2.5.8/String.html#method-i-delete_prefix
- https://ruby-doc.org/core-2.5.8/String.html#method-i-delete_suffix

`Performance/DeletePrefix` cop identifies places where `gsub(/\Aprefix/, '')`
can be replaced by `delete_prefix('prefix')`.

```ruby
# bad
str.gsub(/\Aprefix/, '')
str.gsub!(/\Aprefix/, '')
str.gsub(/^prefix/, '')
str.gsub!(/^prefix/, '')

# good
str.delete_prefix('prefix')
str.delete_prefix!('prefix')
```

The `delete_prefix('prefix')` method is faster than `gsub(/\Aprefix/, '')`.

```console
% ruby -v
ruby 2.5.8p224 (2020-03-31 revision 67882) [x86_64-darwin17]

% cat bench.rb
require 'benchmark/ips'

Benchmark.ips do |x|
  str = 'foobar'

  x.report('gsub')           { str.gsub(/bar\z/, '') }
  x.report('gsub!')          { str.gsub(/bar\z/, '') }
  x.report('delete_suffix')  { str.delete_suffix('bar') }
  x.report('delete_suffix!') { str.delete_suffix('bar') }
  x.compare!
end

% ruby bench.rb
Warming up --------------------------------------
                gsub    46.814k i/100ms
               gsub!    46.896k i/100ms
       delete_suffix   211.337k i/100ms
      delete_suffix!   208.332k i/100ms
Calculating -------------------------------------
                gsub    546.500k (± 1.3%) i/s -      2.762M in 5.054918s
               gsub!    551.054k (± 1.2%) i/s -      2.767M in 5.021747s
       delete_suffix      4.780M (± 1.1%) i/s -     24.092M in 5.040850s
      delete_suffix!      4.770M (± 1.1%) i/s -     23.958M in 5.022823s

Comparison:
       delete_suffix:  4780060.8 i/s
      delete_suffix!:  4770419.3 i/s - same-ish: difference falls within error
               gsub!:   551054.2 i/s - 8.67x  slower
                gsub:   546500.1 i/s - 8.75x  slower
```

`Performance/DeleteSuffix` cop is the same.
@koic koic force-pushed the add_new_delete_prefix_and_delete_suffix_cops branch from c84965c to 78047b0 Compare May 6, 2020 18:54
@koic koic merged commit 0f14f80 into rubocop:master May 10, 2020
@koic koic deleted the add_new_delete_prefix_and_delete_suffix_cops branch May 10, 2020 23:34
@vishalzambre
Copy link

@koic delete_prefix doesn't support to regex but rubocop gives error if gsub is used with regex

@koic
Copy link
Member Author

koic commented Jun 19, 2020

@vishalzambre Can you open the issue with the following template?
https://github.com/rubocop-hq/rubocop-performance/issues/new?template=bug_report.md

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

Successfully merging this pull request may close these issues.

None yet

3 participants