Merge pull request #357 from vlad-pisanov/vp_sort

Add `sort!` and `minmax` to `Performance/CompareWithBlock`

changelog/change_compare_with_block_methods.md

@@ -0,0 +1 @@
+* [#357](https://github.com/rubocop/rubocop-performance/pull/357): Add `sort!` and `minmax` to `Performance/CompareWithBlock`. ([@vlad-pisanov][])

lib/rubocop/cop/performance/compare_with_block.rb

@@ -5,35 +5,42 @@ module Cop
     module Performance
       # Identifies places where `sort { |a, b| a.foo <=> b.foo }`
       # can be replaced by `sort_by(&:foo)`.
-      # This cop also checks `max` and `min` methods.
+      # This cop also checks `sort!`, `min`, `max` and `minmax` methods.
       #
       # @example
       #   # bad
-      #   array.sort { |a, b| a.foo <=> b.foo }
-      #   array.max { |a, b| a.foo <=> b.foo }
-      #   array.min { |a, b| a.foo <=> b.foo }
-      #   array.sort { |a, b| a[:foo] <=> b[:foo] }
+      #   array.sort   { |a, b| a.foo <=> b.foo }
+      #   array.sort!  { |a, b| a.foo <=> b.foo }
+      #   array.max    { |a, b| a.foo <=> b.foo }
+      #   array.min    { |a, b| a.foo <=> b.foo }
+      #   array.minmax { |a, b| a.foo <=> b.foo }
+      #   array.sort   { |a, b| a[:foo] <=> b[:foo] }
       #
       #   # good
       #   array.sort_by(&:foo)
+      #   array.sort_by!(&:foo)
       #   array.sort_by { |v| v.foo }
       #   array.sort_by do |var|
       #     var.foo
       #   end
       #   array.max_by(&:foo)
       #   array.min_by(&:foo)
+      #   array.minmax_by(&:foo)
       #   array.sort_by { |a| a[:foo] }
       class CompareWithBlock < Base
         include RangeHelp
         extend AutoCorrector
 
-        MSG = 'Use `%<compare_method>s_by%<instead>s` instead of ' \
+        MSG = 'Use `%<replacement_method>s%<instead>s` instead of ' \
               '`%<compare_method>s { |%<var_a>s, %<var_b>s| %<str_a>s ' \
               '<=> %<str_b>s }`.'
 
+        REPLACEMENT = { sort: :sort_by, sort!: :sort_by!, min: :min_by, max: :max_by, minmax: :minmax_by }.freeze
+        private_constant :REPLACEMENT
+
         def_node_matcher :compare?, <<~PATTERN
           (block
-            $(send _ {:sort :min :max})
+            $(send _ {:sort :sort! :min :max :minmax})
             (args (arg $_a) (arg $_b))
             $send)
         PATTERN
@@ -54,9 +61,9 @@ def on_block(node)
 
               add_offense(range, message: message(send, method, var_a, var_b, args_a)) do |corrector|
                 replacement = if method == :[]
-                                "#{send.method_name}_by { |a| a[#{args_a.first.source}] }"
+                                "#{REPLACEMENT[send.method_name]} { |a| a[#{args_a.first.source}] }"
                               else
-                                "#{send.method_name}_by(&:#{method})"
+                                "#{REPLACEMENT[send.method_name]}(&:#{method})"
                               end
                 corrector.replace(range, replacement)
               end
@@ -82,7 +89,8 @@ def slow_compare?(method, args_a, args_b)
 
         # rubocop:disable Metrics/MethodLength
         def message(send, method, var_a, var_b, args)
-          compare_method = send.method_name
+          compare_method     = send.method_name
+          replacement_method = REPLACEMENT[compare_method]
           if method == :[]
             key = args.first
             instead = " { |a| a[#{key.source}] }"
@@ -94,6 +102,7 @@ def message(send, method, var_a, var_b, args)
             str_b = "#{var_b}.#{method}"
           end
           format(MSG, compare_method: compare_method,
+                      replacement_method: replacement_method,
                       instead: instead,
                       var_a: var_a,
                       var_b: var_b,

spec/rubocop/cop/performance/compare_with_block_spec.rb

@@ -1,61 +1,63 @@
 # frozen_string_literal: true
 
 RSpec.describe RuboCop::Cop::Performance::CompareWithBlock, :config do
-  shared_examples 'compare with block' do |method|
+  shared_examples 'compare with block' do |method, replacement|
     it "registers an offense and corrects for #{method}" do
       expect_offense(<<~RUBY, method: method)
         array.#{method} { |a, b| a.foo <=> b.foo }
-              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by(&:foo)` instead of `#{method} { |a, b| a.foo <=> b.foo }`.
+              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement}(&:foo)` instead of `#{method} { |a, b| a.foo <=> b.foo }`.
       RUBY
 
       expect_correction(<<~RUBY)
-        array.#{method}_by(&:foo)
+        array.#{replacement}(&:foo)
       RUBY
     end
 
     it "registers an offense and corrects for #{method} with [:foo]" do
       expect_offense(<<~RUBY, method: method)
         array.#{method} { |a, b| a[:foo] <=> b[:foo] }
-              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a[:foo] }` instead of `#{method} { |a, b| a[:foo] <=> b[:foo] }`.
+              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a[:foo] }` instead of `#{method} { |a, b| a[:foo] <=> b[:foo] }`.
       RUBY
 
       expect_correction(<<~RUBY)
-        array.#{method}_by { |a| a[:foo] }
+        array.#{replacement} { |a| a[:foo] }
       RUBY
     end
 
     it "registers an offense and corrects for #{method} with ['foo']" do
       expect_offense(<<~RUBY, method: method)
         array.#{method} { |a, b| a['foo'] <=> b['foo'] }
-              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a['foo'] }` instead of `#{method} { |a, b| a['foo'] <=> b['foo'] }`.
+              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a['foo'] }` instead of `#{method} { |a, b| a['foo'] <=> b['foo'] }`.
       RUBY
 
       expect_correction(<<~RUBY)
-        array.#{method}_by { |a| a['foo'] }
+        array.#{replacement} { |a| a['foo'] }
       RUBY
     end
 
     it "registers an offense and corrects for #{method} with [1]" do
       expect_offense(<<~RUBY, method: method)
         array.#{method} { |a, b| a[1] <=> b[1] }
-              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a[1] }` instead of `#{method} { |a, b| a[1] <=> b[1] }`.
+              ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a[1] }` instead of `#{method} { |a, b| a[1] <=> b[1] }`.
       RUBY
 
       expect_correction(<<~RUBY)
-        array.#{method}_by { |a| a[1] }
+        array.#{replacement} { |a| a[1] }
       RUBY
     end
 
     it "accepts valid #{method} usage" do
       expect_no_offenses("array.#{method} { |a, b| b <=> a }")
     end
 
-    it "accepts #{method}_by" do
-      expect_no_offenses("array.#{method}_by { |a| a.baz }")
+    it "accepts #{replacement}" do
+      expect_no_offenses("array.#{replacement} { |a| a.baz }")
     end
   end
 
-  include_examples 'compare with block', 'sort'
-  include_examples 'compare with block', 'max'
-  include_examples 'compare with block', 'min'
+  include_examples 'compare with block', 'sort',   'sort_by'
+  include_examples 'compare with block', 'sort!',  'sort_by!'
+  include_examples 'compare with block', 'max',    'max_by'
+  include_examples 'compare with block', 'min',    'min_by'
+  include_examples 'compare with block', 'minmax', 'minmax_by'
 end