Add a new cop Performance/UnneededSort #5753

parkerfinch · 2018-04-07T23:04:41Z

Usage of sort.last and sort.first can be replaced with max and
min, respectively. Similarly, sort_by(&:something).first and
sort_by(&:something).last can be replaced with the more efficient min_by(&:something)andmax_by(&:something)`.

This cop identifies and autocorrects these unneeded sorts.

This cop does not register offenses if the first or last call has an
argument, because sort.last(n) is not equivalent to max(n). This
cop does not register offenses for the bang sorts (sort! and
sort_by!) because their side-effects may be used.

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 rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

Drenmi · 2018-04-08T10:47:10Z

Code looks good to me. One comment on the cop name. There's already a small eco system of names for different cases of "unneeded", "redundant", etc. Would prefer using one of those instead of introducing additional "superfluous". 🙂

parkerfinch · 2018-04-08T11:40:27Z

Thanks @Drenmi , makes sense, I'll switch it over to UnneededSort!

parkerfinch · 2018-04-08T12:03:34Z

I changed the cop name, although the branch still references superfluous. Should I open a new PR with a renamed branch?

koic · 2018-04-08T12:15:56Z

No, the branch name should remain old. It is not important when investigating history. Also, it is more convenient to see conversations with the same PR in this case. Thanks for your concern.

bdewater · 2018-04-08T17:21:07Z

For #5643 I discovered a few different ways of accessing the first element which you might also want to cover in this cop.

Like suggested in that PR, please consider adding benchmarks to https://github.com/JuanitoFatas/fast-ruby as well for the community to learn from 😄

rrosenblum · 2018-04-09T13:13:16Z

lib/rubocop/cop/performance/unneeded_sort.rb

+        include RangeHelp
+
+        MSG = 'Use `%<suggestion>s` instead of '\
+              '`%<sorter>s...%<accessor>s`.'.freeze


What do you think of using the actual code snippet here instead of ...?

Sounds reasonable, but I'm not completely sold! I'm a little worried about the case when the actual code snippet is nontrivial:

Use `min_by` instead of `sort_by...first`

is clear about what both the offense and preferred approach are, while

Use `min_by do |x| a = x.first b = x.last [a.length, b.length].max end` instead of `sort_by do |x| a = x.first b = x.last [a.length, b.length].max end.first`

is more difficult to interpret. Any thoughts there?

If you do still want to do the the full code snippet approach, what's the best way to generate the preferred code outside of the autocorrect method? Is there a way to access a similar Corrector that doesn't actually affect the source code?

Good point. I wasn't really thinking about multi-line examples. The shorthand using sort_by...first will be fine.

rrosenblum · 2018-04-09T13:16:50Z

spec/rubocop/cop/performance/unneeded_sort_spec.rb

+  subject(:cop) { described_class.new }
+
+  it 'registers an offense when first is called with sort' do
+    inspect_source('[1, 2, 3].sort.first')


Please use the expect_offense and expect_no_offenses helpers.

expect_offense(<<-RUBY.strip_indent) [1, 2, 3].sort.first ^^^^^^^^^^ Use `min` instead of `sort...first`. RUBY

parkerfinch · 2018-04-10T12:49:33Z

Added more ways of accessing the first/last element suggested by @bdewater and used the spec helpers suggested by @rrosenblum!

parkerfinch · 2018-04-11T00:16:38Z

Squashed commits and edited the commit message to reflect the new scope of this cop.

Let me know if there's anything else I can do to help this across the finish line!

bdewater · 2018-04-11T13:20:50Z

CHANGELOG.md

@@ -20,6 +20,7 @@

 * [#5597](https://github.com/bbatsov/rubocop/pull/5597): Add new `Rails/HttpStatus` cop. ([@anthony-robin][])
 * [#5643](https://github.com/bbatsov/rubocop/pull/5643): Add new `Style/UnpackFirst` cop. ([@bdewater][])
+* Add new `Performance/UnneededSort` cop. ([@parkerfinch][])


You can add the PR # and link :)

Ah nice will do, thanks!

bdewater · 2018-04-11T14:03:46Z

CHANGELOG.md

@@ -20,6 +20,7 @@

 * [#5597](https://github.com/bbatsov/rubocop/pull/5597): Add new `Rails/HttpStatus` cop. ([@anthony-robin][])
 * [#5643](https://github.com/bbatsov/rubocop/pull/5643): Add new `Style/UnpackFirst` cop. ([@bdewater][])
+* [#5753](https://github.com/bbatsov/rubocop/pull/5753): Add new `Performance/UnneededSort` cop. ([@parkerfinch][])


I didn't expand the preview on GH until now - sorry to be a bother - noticed this is under the '0.54.0' header instead of 'master' probably due to the rebase.

Whew, good catch! Fixed.

A `sort` followed by retrieving only the first or last element of the resulting array can be replaced with a `min` or `max` call. For example, `sort.first` can be replaced with `min` while `sort[-1]` can be replaced with `max`. Similarly, `sort_by(&:something).first` and `sort_by(&:something).last` can be replaced with the more efficient `min_by(&:something)` and `max_by(&:something)`. This cop identifies and autocorrects these unneeded sorts. This cop does *not* register offenses if the `first` or `last` call has an argument, because `sort.last(n)` is not equivalent to `max(n)`. This cop does *not* register offenses for the bang sorts (`sort!` and `sort_by!`) because their side-effects may be used.

parkerfinch · 2018-04-11T17:30:50Z

Rebased onto master to resolve CHANGELOG.md issues.

bbatsov · 2018-04-16T07:08:26Z

Nice job!

Just one small feedback - I think that a lot of the general explanations you've mentioned in the PR description would be useful addition to the cop's description. It also seems that you haven't added examples there for for everything the cop actually covers.

parkerfinch · 2018-04-16T12:49:24Z

Thanks @bbatsov! Per your feedback, opened #5787 with a more general overview in the cop description and a more complete set of examples.

parkerfinch force-pushed the superfluous-sort-cop branch from 18ea6ba to 6460edc Compare April 8, 2018 11:51

parkerfinch changed the title ~~Add a new cop Performance/SuperfluousSort~~ Add a new cop Performance/UnneededSort Apr 8, 2018

parkerfinch force-pushed the superfluous-sort-cop branch 3 times, most recently from f2b1fc9 to 8c0ad4b Compare April 8, 2018 12:00

parkerfinch mentioned this pull request Apr 8, 2018

Add Enumerable#sort_by.first vs Enumerable#min_by fastruby/fast-ruby#153

Merged

rrosenblum suggested changes Apr 9, 2018

View reviewed changes

rrosenblum approved these changes Apr 10, 2018

View reviewed changes

parkerfinch force-pushed the superfluous-sort-cop branch from b24534d to 85870f8 Compare April 11, 2018 00:15

bdewater approved these changes Apr 11, 2018

View reviewed changes

parkerfinch force-pushed the superfluous-sort-cop branch from 85870f8 to 0548822 Compare April 11, 2018 13:47

bdewater reviewed Apr 11, 2018

View reviewed changes

parkerfinch force-pushed the superfluous-sort-cop branch from 0548822 to 5671a13 Compare April 11, 2018 14:14

parkerfinch force-pushed the superfluous-sort-cop branch from 5671a13 to 372b44d Compare April 11, 2018 17:29

Merge branch 'master' into superfluous-sort-cop

88e851e

bbatsov merged commit ec3bdbf into rubocop:master Apr 16, 2018

parkerfinch deleted the superfluous-sort-cop branch April 16, 2018 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new cop Performance/UnneededSort #5753

Add a new cop Performance/UnneededSort #5753

parkerfinch commented Apr 7, 2018

Drenmi commented Apr 8, 2018

parkerfinch commented Apr 8, 2018

parkerfinch commented Apr 8, 2018

koic commented Apr 8, 2018 •

edited

bdewater commented Apr 8, 2018

rrosenblum Apr 9, 2018

parkerfinch Apr 10, 2018

rrosenblum Apr 10, 2018

rrosenblum Apr 9, 2018

parkerfinch commented Apr 10, 2018

parkerfinch commented Apr 11, 2018

bdewater Apr 11, 2018

parkerfinch Apr 11, 2018

bdewater Apr 11, 2018

parkerfinch Apr 11, 2018

parkerfinch commented Apr 11, 2018

bbatsov commented Apr 16, 2018

parkerfinch commented Apr 16, 2018

Add a new cop Performance/UnneededSort #5753

Add a new cop Performance/UnneededSort #5753

Conversation

parkerfinch commented Apr 7, 2018

Drenmi commented Apr 8, 2018

parkerfinch commented Apr 8, 2018

parkerfinch commented Apr 8, 2018

koic commented Apr 8, 2018 • edited

bdewater commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerfinch commented Apr 10, 2018

parkerfinch commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerfinch commented Apr 11, 2018

bbatsov commented Apr 16, 2018

parkerfinch commented Apr 16, 2018

koic commented Apr 8, 2018 •

edited