Merge pull request #2416 from rrosenblum/conditional_assignment2

Conditional assignment

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## master (unreleased)
 
+### New features
+
+* [#2416](https://github.com/bbatsov/rubocop/pull/2416): New cop `Style/ConditionalAssignment` checks for assignment of the same variable in all branches of conditionals and replaces them with a single assignment to the return of the conditional. ([@rrosenblum][])
+
 ## 0.35.1 (10/11/2015)
 
 ### Bug Fixes

config/enabled.yml

@@ -153,6 +153,13 @@ Style/CommentIndentation:
   Description: 'Indentation of comments.'
   Enabled: true
 
+Style/ConditionalAssignment:
+  Description: >-
+                 Use the return value of `if` and `case` statements for
+                 assignment to a variable instead of assigning that variable
+                 inside of each branch.
+  Enabled: true
+
 Style/ConstantName:
   Description: 'Constants should use SCREAMING_SNAKE_CASE.'
   StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#screaming-snake-case'

lib/rubocop.rb

@@ -168,6 +168,7 @@
 require 'rubocop/cop/style/command_literal'
 require 'rubocop/cop/style/comment_annotation'
 require 'rubocop/cop/style/comment_indentation'
+require 'rubocop/cop/style/conditional_assignment'
 require 'rubocop/cop/style/copyright'
 require 'rubocop/cop/style/constant_name'
 require 'rubocop/cop/style/def_with_parentheses'

lib/rubocop/cop/lint/useless_setter_call.rb

@@ -30,11 +30,11 @@ class UselessSetterCall < Cop
         def on_method_def(_node, _method_name, _args, body)
           return unless body
 
-          if body.type == :begin
-            expression = body.children
-          else
-            expression = body
-          end
+          expression = if body.type == :begin
+                         body.children
+                       else
+                         body
+                       end
 
           last_expr = expression.is_a?(Array) ? expression.last : expression
 

lib/rubocop/cop/style/command_literal.rb

@@ -95,11 +95,11 @@ def preferred_delimiters
         def autocorrect(node)
           return if contains_backtick?(node)
 
-          if backtick_literal?(node)
-            replacement = ['%x', ''].zip(preferred_delimiters).map(&:join)
-          else
-            replacement = %w(` `)
-          end
+          replacement = if backtick_literal?(node)
+                          ['%x', ''].zip(preferred_delimiters).map(&:join)
+                        else
+                          %w(` `)
+                        end
 
           lambda do |corrector|
             corrector.replace(node.loc.begin, replacement.first)

lib/rubocop/cop/style/conditional_assignment.rb

@@ -0,0 +1,247 @@
+# encoding: utf-8
+
+module RuboCop
+  module Cop
+    module Style
+      # Check for `if` and `case` statements where each branch is used for
+      # assignment to the same variable when using the return of the
+      # condition can be used instead.
+      #
+      # @example
+      #   # bad
+      #   if foo
+      #     bar = 1
+      #   else
+      #     bar = 2
+      #   end
+      #
+      #   case foo
+      #   when 'a'
+      #     bar += 1
+      #   else
+      #     bar += 2
+      #   end
+      #
+      #   if foo
+      #     some_method
+      #     bar = 1
+      #   else
+      #     some_other_method
+      #     bar = 2
+      #   end
+      #
+      #   # good
+      #   bar = if foo
+      #           1
+      #         else
+      #           2
+      #         end
+      #
+      #   bar += case foo
+      #          when 'a'
+      #            1
+      #          else
+      #            2
+      #          end
+      #
+      #   bar << if foo
+      #            some_method
+      #            1
+      #          else
+      #            some_other_method
+      #            2
+      #          end
+      class ConditionalAssignment < Cop
+        include IfNode
+
+        MSG = 'Use the return of the conditional for variable assignment.'
+        ASSIGNMENT_TYPES = [:casgn, :cvasgn, :gvasgn, :ivasgn, :lvasgn,
+                            :and_asgn, :or_asgn, :op_asgn].freeze
+        IF = 'if'.freeze
+        ELSIF = 'elsif'.freeze
+        UNLESS = 'unless'.freeze
+
+        def on_if(node)
+          return if ternary_op?(node)
+          return if elsif?(node)
+          _condition, if_branch, else_branch = *node
+          elsif_branches, else_branch = expand_elses(else_branch)
+          return if [if_branch, *elsif_branches, else_branch].any?(&:nil?)
+          *_, if_branch = *if_branch if if_branch.begin_type?
+          return unless assignment_type?(if_branch)
+          return unless types_match?(if_branch, *elsif_branches, else_branch)
+          return unless variables_match?(if_branch, *elsif_branches,
+                                         else_branch)
+          if_variable, = *if_branch
+          operator = operator(if_branch)
+          return if correction_exceeds_line_limit?(node, if_variable, operator)
+
+          add_offense(node, :expression)
+        end
+
+        def on_case(node)
+          return unless node.loc.else
+          _condition, *when_branches, else_branch = *node
+          *_, else_branch = *else_branch if else_branch.begin_type?
+          when_branches = expand_when_branches(when_branches)
+          return unless assignment_type?(else_branch)
+          return unless types_match?(*when_branches, else_branch)
+          return unless variables_match?(*when_branches, else_branch)
+
+          variable, = *else_branch
+          operator = operator(else_branch)
+          return if correction_exceeds_line_limit?(node, variable, operator)
+
+          add_offense(node, :expression)
+        end
+
+        def autocorrect(node)
+          case node.loc.keyword.source
+          when IF
+            if_correction(node)
+          when UNLESS
+            unless_correction(node)
+          else
+            case_correction(node)
+          end
+        end
+
+        private
+
+        def expand_elses(branch)
+          elsif_branches = expand_elsif(branch)
+          else_branch = elsif_branches.any? ? elsif_branches.pop : branch
+          if else_branch && else_branch.begin_type?
+            *_, else_branch = *else_branch
+          end
+          [elsif_branches, else_branch]
+        end
+
+        def expand_elsif(node, elsif_branches = [])
+          return [] if node.nil? || !node.if_type?
+          _condition, elsif_branch, else_branch = *node
+          if elsif_branch && elsif_branch.begin_type?
+            *_, elsif_branch = *elsif_branch
+          end
+          elsif_branches << elsif_branch
+          if else_branch && else_branch.if_type?
+            expand_elsif(else_branch, elsif_branches)
+          else
+            elsif_branches << else_branch
+          end
+          elsif_branches
+        end
+
+        def variables_match?(*branches)
+          first_variable, = *branches.first
+          branches.all? { |branch| branch.children[0] == first_variable }
+        end
+
+        def types_match?(*nodes)
+          first_type = nodes.first.type
+          nodes.all? { |node| node.type == first_type }
+        end
+
+        def assignment_type?(branch)
+          return true if ASSIGNMENT_TYPES.include?(branch.type)
+
+          if branch.send_type?
+            _variable, method, = *branch
+            return true if method == :<<
+          end
+
+          false
+        end
+
+        def expand_when_branches(when_branches)
+          when_branches.map do |branch|
+            when_branch = branch.children[1]
+            *_, when_branch = *when_branch if when_branch.begin_type?
+            when_branch
+          end
+        end
+
+        def correction_exceeds_line_limit?(node, variable, operator)
+          return false unless config.for_cop('Metrics/LineLength')['Enabled']
+          assignment = "#{variable} #{operator} "
+          assignment_regex = /#{variable}\s*#{operator}\s*/
+          max_line_length = config.for_cop('Metrics/LineLength')['Max']
+          lines = node.loc.expression.source.lines.map do |line|
+            line.chomp.sub(assignment_regex, '')
+          end
+          longest_line = lines.max_by(&:length)
+          (longest_line + assignment).length > max_line_length
+        end
+
+        def if_correction(node)
+          _condition, if_branch, else_branch = *node
+          *_, if_branch = *if_branch if if_branch.begin_type?
+          variable, *_operator, if_assignment = *if_branch
+          variable, alternate = *variable
+          variable ||= alternate
+          elsif_branches, else_branch = expand_elses(else_branch)
+          _else_variable, *_operator, else_assignment = *else_branch
+
+          lambda do |corrector|
+            corrector.insert_before(node.loc.expression,
+                                    "#{variable} #{operator(if_branch)} ")
+            corrector.replace(if_branch.loc.expression,
+                              if_assignment.loc.expression.source)
+            correct_branches(corrector, elsif_branches)
+            corrector.replace(else_branch.loc.expression,
+                              else_assignment.loc.expression.source)
+          end
+        end
+
+        def case_correction(node)
+          _condition, *when_branches, else_branch = *node
+          *_, else_branch = *else_branch if else_branch.begin_type?
+          when_branches = expand_when_branches(when_branches)
+
+          variable, *_operator, else_assignment = *else_branch
+          variable, alternate = *variable
+          variable ||= alternate
+
+          lambda do |corrector|
+            corrector.insert_before(node.loc.expression,
+                                    "#{variable} #{operator(else_branch)} ")
+            correct_branches(corrector, when_branches)
+            corrector.replace(else_branch.loc.expression,
+                              else_assignment.loc.expression.source)
+          end
+        end
+
+        def correct_branches(corrector, branches)
+          branches.each do |branch|
+            *_, assignment = *branch
+            corrector.replace(branch.loc.expression,
+                              assignment.loc.expression.source)
+          end
+        end
+
+        def unless_correction(node)
+          _condition, else_branch, if_branch = *node
+          *_, if_branch = *if_branch if if_branch.begin_type?
+          *_, else_branch = *else_branch if else_branch.begin_type?
+          variable, *_operator, if_assignment = *if_branch
+          variable, alternate = *variable
+          variable ||= alternate
+          _else_variable, *_operator, else_assignment = *else_branch
+
+          lambda do |corrector|
+            corrector.insert_before(node.loc.expression,
+                                    "#{variable} #{operator(if_branch)} ")
+            corrector.replace(if_branch.loc.expression,
+                              if_assignment.loc.expression.source)
+            corrector.replace(else_branch.loc.expression,
+                              else_assignment.loc.expression.source)
+          end
+        end
+
+        def operator(node)
+          node.send_type? ? node.loc.selector.source : node.loc.operator.source
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/style/copyright.rb

@@ -75,11 +75,11 @@ def autocorrect(token)
             "match Notice /#{notice}/" unless autocorrect_notice =~ regex
 
           lambda do |corrector|
-            if token.nil?
-              range = Parser::Source::Range.new('', 0, 0)
-            else
-              range = token.pos
-            end
+            range = if token.nil?
+                      Parser::Source::Range.new('', 0, 0)
+                    else
+                      token.pos
+                    end
             corrector.insert_before(range, "#{autocorrect_notice}\n")
           end
         end

lib/rubocop/cop/style/next.rb

@@ -102,13 +102,13 @@ def offense_location(offense_node)
         def autocorrect(node)
           lambda do |corrector|
             cond, if_body, else_body = *node
-            if if_body.nil?
-              opposite_kw = 'if'
-              body = else_body
-            else
-              opposite_kw = 'unless'
-              body = if_body
-            end
+            body = if if_body.nil?
+                     opposite_kw = 'if'
+                     else_body
+                   else
+                     opposite_kw = 'unless'
+                     if_body
+                   end
             next_code = 'next ' << opposite_kw << ' ' <<
                         cond.loc.expression.source << "\n"
             corrector.insert_before(node.loc.expression, next_code)

lib/rubocop/cop/style/regexp_literal.rb

@@ -91,11 +91,11 @@ def preferred_delimiters
         def autocorrect(node)
           return if contains_slash?(node)
 
-          if slash_literal?(node)
-            replacement = ['%r', ''].zip(preferred_delimiters).map(&:join)
-          else
-            replacement = %w(/ /)
-          end
+          replacement = if slash_literal?(node)
+                          ['%r', ''].zip(preferred_delimiters).map(&:join)
+                        else
+                          %w(/ /)
+                        end
 
           lambda do |corrector|
             corrector.replace(node.loc.begin, replacement.first)

lib/rubocop/formatter/formatter_set.rb

@@ -65,13 +65,13 @@ def add_formatter(formatter_type, output_path = nil)
                             builtin_formatter_class(formatter_type)
                           end
 
-        if output_path
-          dir_path = File.dirname(output_path)
-          FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path)
-          output = File.open(output_path, 'w')
-        else
-          output = $stdout
-        end
+        output = if output_path
+                   dir_path = File.dirname(output_path)
+                   FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path)
+                   File.open(output_path, 'w')
+                 else
+                   $stdout
+                 end
 
         self << formatter_class.new(output)
       end