Reduce Metrics/CyclomaticComplexity Max to 9 (rubocop#3038)

* Reduce complexity in AstNode Split `RuboCop::Node#value_used?` into smaller methods. * Reduce complexity in Detect Split `on_send` into smaller methods and refactor existing methods slightly to improve consistency. * Reduce complexity in GuardClause Split `on_if` into smaller methods. * Reduce complexity in Options Split `RuboCop::Options#validate_compatibility` into smaller methods. * Reduce complexity in StringReplacement Split `on_send` into smaller methods. * Update .rubocop_todo.yml
Neodelf · Oct 15, 2016 · e8ad9e4 · e8ad9e4
1 parent 54bd043
commit e8ad9e4
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 74 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -1,25 +1,25 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2016-03-29 18:50:49 +0300 using RuboCop version 0.39.0.
+# on 2016-04-12 21:23:03 +0300 using RuboCop version 0.39.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 212
+# Offense count: 206
 Metrics/AbcSize:
   Max: 27
 
-# Offense count: 20
+# Offense count: 21
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
   Max: 160
 
-# Offense count: 80
+# Offense count: 77
 Metrics/CyclomaticComplexity:
-  Max: 10
+  Max: 9
 
-# Offense count: 237
+# Offense count: 235
 # Configuration parameters: CountComments.
 Metrics/MethodLength:
   Max: 20
@@ -29,6 +29,6 @@ Metrics/MethodLength:
 Metrics/ModuleLength:
   Max: 154
 
-# Offense count: 59
+# Offense count: 55
 Metrics/PerceivedComplexity:
   Max: 10
diff --git a/lib/rubocop/ast_node.rb b/lib/rubocop/ast_node.rb
@@ -503,27 +503,20 @@ def chained?
     def value_used?
       # Be conservative and return true if we're not sure
       return false if parent.nil?
-      index = parent.children.index { |child| child.equal?(self) }
 
       case parent.type
       when :array, :defined?, :dstr, :dsym, :eflipflop, :erange, :float,
            :hash, :iflipflop, :irange, :not, :pair, :regexp, :str, :sym, :when,
            :xstr
         parent.value_used?
       when :begin, :kwbegin
-        # the last child node determines the value of the parent
-        index == parent.children.size - 1 ? parent.value_used? : false
+        begin_value_used?
       when :for
-        # `for var in enum; body; end`
-        # (for <var> <enum> <body>)
-        index == 2 ? parent.value_used? : true
+        for_value_used?
       when :case, :if
-        # (case <condition> <when...>)
-        # (if <condition> <truebranch> <falsebranch>)
-        index == 0 ? true : parent.value_used?
+        case_if_value_used?
       when :while, :until, :while_post, :until_post
-        # (while <condition> <body>) -> always evaluates to `nil`
-        index == 0
+        while_until_value_used?
       else
         true
       end
@@ -589,5 +582,27 @@ def visit_ancestors_with_types(types)
         last_node = current_node
       end
     end
+
+    def begin_value_used?
+      # the last child node determines the value of the parent
+      sibling_index == parent.children.size - 1 ? parent.value_used? : false
+    end
+
+    def for_value_used?
+      # `for var in enum; body; end`
+      # (for <var> <enum> <body>)
+      sibling_index == 2 ? parent.value_used? : true
+    end
+
+    def case_if_value_used?
+      # (case <condition> <when...>)
+      # (if <condition> <truebranch> <falsebranch>)
+      sibling_index == 0 ? true : parent.value_used?
+    end
+
+    def while_until_value_used?
+      # (while <condition> <body>) -> always evaluates to `nil`
+      sibling_index == 0
+    end
   end
 end
diff --git a/lib/rubocop/cop/performance/detect.rb b/lib/rubocop/cop/performance/detect.rb
@@ -33,23 +33,12 @@ class Detect < Cop
         def on_send(node)
           return unless should_run?
           receiver, second_method, *args = *node
-          return unless check_second_call(receiver, second_method, args)
+          return if accept_second_call?(receiver, second_method, args)
 
           receiver, _args, body = *receiver if receiver.block_type?
-          caller, first_method, args = *receiver
-
-          # check that we have usual block or block pass
-          return if body.nil? && (args.nil? || !args.block_pass_type?)
-          return unless SELECT_METHODS.include?(first_method)
-          return if lazy?(caller)
+          return if accept_first_call?(receiver, body)
 
-          range = receiver.loc.selector.join(node.loc.selector)
-
-          message = second_method == :last ? REVERSE_MSG : MSG
-          add_offense(node, range, format(message,
-                                          preferred_method,
-                                          first_method,
-                                          second_method))
+          offense(node, receiver, second_method)
         end
 
         def autocorrect(node)
@@ -79,10 +68,31 @@ def should_run?
             config['Rails'.freeze]['Enabled'.freeze])
         end
 
-        def check_second_call(receiver, method, args)
-          receiver &&
-            DANGEROUS_METHODS.include?(method) &&
-            args.empty?
+        def accept_second_call?(receiver, method, args)
+          !receiver ||
+            !DANGEROUS_METHODS.include?(method) ||
+            !args.empty?
+        end
+
+        def accept_first_call?(receiver, body)
+          caller, first_method, args = *receiver
+
+          # check that we have usual block or block pass
+          return true if body.nil? && (args.nil? || !args.block_pass_type?)
+          return true unless SELECT_METHODS.include?(first_method)
+
+          lazy?(caller)
+        end
+
+        def offense(node, receiver, second_method)
+          _caller, first_method, _args = *receiver
+          range = receiver.loc.selector.join(node.loc.selector)
+
+          message = second_method == :last ? REVERSE_MSG : MSG
+          add_offense(node, range, format(message,
+                                          preferred_method,
+                                          first_method,
+                                          second_method))
         end
 
         def preferred_method

diff --git a/lib/rubocop/cop/performance/string_replacement.rb b/lib/rubocop/cop/performance/string_replacement.rb
@@ -32,28 +32,12 @@ class StringReplacement < Cop
 
         def on_send(node)
           _string, method, first_param, second_param = *node
-          return unless GSUB_METHODS.include?(method)
-          return unless string?(second_param)
-          return unless DETERMINISTIC_TYPES.include?(first_param.type)
-
-          first_source, options = first_source(first_param)
-          second_source, = *second_param
-
-          return if first_source.nil?
-
-          if regex?(first_param)
-            return unless first_source =~ DETERMINISTIC_REGEX
-            return if options
-            # This must be done after checking DETERMINISTIC_REGEX
-            # Otherwise things like \s will trip us up
-            first_source = interpret_string_escapes(first_source)
-          end
 
-          return if first_source.length != 1
-          return unless second_source.length <= 1
+          return unless GSUB_METHODS.include?(method)
+          return if accept_second_param?(second_param)
+          return if accept_first_param?(first_param)
 
-          message = message(method, first_source, second_source)
-          add_offense(node, range(node), message)
+          offense(node, method, first_param, second_param)
         end
 
         def autocorrect(node)
@@ -84,6 +68,41 @@ def autocorrect(node)
 
         private
 
+        def accept_second_param?(second_param)
+          return true unless string?(second_param)
+          second_source, = *second_param
+
+          second_source.length > 1
+        end
+
+        def accept_first_param?(first_param)
+          return true unless DETERMINISTIC_TYPES.include?(first_param.type)
+
+          first_source, options = first_source(first_param)
+          return true if first_source.nil?
+
+          if regex?(first_param)
+            return true if options
+            return true unless first_source =~ DETERMINISTIC_REGEX
+            # This must be done after checking DETERMINISTIC_REGEX
+            # Otherwise things like \s will trip us up
+            first_source = interpret_string_escapes(first_source)
+          end
+
+          first_source.length != 1
+        end
+
+        def offense(node, method, first_param, second_param)
+          first_source, = first_source(first_param)
+          if regex?(first_param)
+            first_source = interpret_string_escapes(first_source)
+          end
+          second_source, = *second_param
+          message = message(method, first_source, second_source)
+
+          add_offense(node, range(node), message)
+        end
+
         def string?(node)
           node && node.str_type?
         end

diff --git a/lib/rubocop/cop/style/guard_clause.rb b/lib/rubocop/cop/style/guard_clause.rb
@@ -61,22 +61,30 @@ def on_def(node)
         end
 
         def on_if(node)
-          cond, body, else_body = *node
-
-          return unless body && else_body
-          # discard modifier ifs and ternary_ops
-          return if modifier_if?(node) || ternary?(node) || elsif?(node)
-
-          return unless single_line_control_flow_exit?(body) ||
-                        single_line_control_flow_exit?(else_body)
-          return if cond.multiline?
+          return if accept_form?(node)
+          return unless any_single_line_control_flow_exit?(node)
           return if line_too_long_when_corrected?(node)
 
           add_offense(node, :keyword, MSG)
         end
 
         private
 
+        def accept_form?(node)
+          cond, body, else_body = *node
+          return true unless body && else_body
+          return true if modifier_if?(node) || ternary?(node) || elsif?(node)
+
+          cond.multiline?
+        end
+
+        def any_single_line_control_flow_exit?(node)
+          _cond, body, else_body = *node
+
+          single_line_control_flow_exit?(body) ||
+            single_line_control_flow_exit?(else_body)
+        end
+
         def if?(node)
           node && node.if_type?
         end

diff --git a/lib/rubocop/options.rb b/lib/rubocop/options.rb
@@ -179,23 +179,44 @@ def initialize(options)
     end
 
     def validate_compatibility
-      if @options.key?(:only) &&
-         (@options[:only] & %w(Lint/UnneededDisable UnneededDisable)).any?
+      if only_includes_unneeded_disable?
         raise ArgumentError, 'Lint/UnneededDisable can not be used with --only.'
       end
-      if @options.key?(:except) &&
-         (@options[:except] & %w(Lint/Syntax Syntax)).any?
+      if except_syntax?
         raise ArgumentError, 'Syntax checking can not be turned off.'
       end
-      if @options.key?(:cache) && !%w(true false).include?(@options[:cache])
+      unless boolean_or_empty_cache?
         raise ArgumentError, '-C/--cache argument must be true or false'
       end
-      if @options.key?(:no_offense_counts) && !@options.key?(:auto_gen_config)
+      if no_offense_counts_without_auto_gen_config?
         raise ArgumentError, '--no-offense-counts can only be used together ' \
-                            'with --auto-gen-config.'
+                             'with --auto-gen-config.'
       end
-      return if (incompat = @options.keys & Options::EXITING_OPTIONS).size <= 1
-      raise ArgumentError, "Incompatible cli options: #{incompat.inspect}"
+      return if incompatible_options.size <= 1
+      raise ArgumentError, 'Incompatible cli options: ' \
+                           "#{incompatible_options.inspect}"
+    end
+
+    def only_includes_unneeded_disable?
+      @options.key?(:only) &&
+        (@options[:only] & %w(Lint/UnneededDisable UnneededDisable)).any?
+    end
+
+    def except_syntax?
+      @options.key?(:except) &&
+        (@options[:except] & %w(Lint/Syntax Syntax)).any?
+    end
+
+    def boolean_or_empty_cache?
+      !@options.key?(:cache) || %w(true false).include?(@options[:cache])
+    end
+
+    def no_offense_counts_without_auto_gen_config?
+      @options.key?(:no_offense_counts) && !@options.key?(:auto_gen_config)
+    end
+
+    def incompatible_options
+      @incompatible_options ||= @options.keys & Options::EXITING_OPTIONS
     end
 
     def validate_exclude_limit_option(args)