Add node extraction and extra two-fer tests

Author: iHiD

lib/analyzer.rb

@@ -16,6 +16,7 @@
 require_relative "generic/extract_module_method"
 require_relative "generic/extract_instance_method"
 require_relative "generic/extract_module_or_class"
+require_relative "generic/extract_nodes"
 
 require_relative 'analyze_solution'
 

lib/analyzers/exercise_analyzer.rb

@@ -1,4 +1,5 @@
 class ExerciseAnalyzer
+  include SA::InlineHelpers
 
   # This is just flow-control for quickly exiting the
   # analysis. We probably don't want to do things this

lib/analyzers/two_fer/analyze.rb

@@ -4,9 +4,11 @@ module TwoFer
     no_module:               "ruby.general.no_target_module",
     no_method:               "ruby.general.no_target_method",
     incorrect_indentation:   "ruby.general.incorrect_indentation",
+    explicit_return:         "ruby.general.explicit_return",         #"The last line automatically gets returned"
     splat_args:              "ruby.two_fer.splat_args",              #Rather than using *%s, how about actually setting a parameter called 'name'?",
     missing_default_param:   "ruby.two_fer.missing_default_param",   #"There is no correct default param - the tests will fail",
     incorrect_default_param: "ruby.two_fer.incorrect_default_param", #You could set the default value to 'you' to avoid conditionals",
+    reassigning_param:       "ruby.two_fer.reassigning_param",    # You don't need to reassign - use the default param
     string_building:         "ruby.two_fer.avoid_string_building",   # "Rather than using string building, use interpolation",
     kernel_format:           "ruby.two_fer.avoid_kernel_format",     #"Rather than using the format method, use interpolation",
     string_format:           "ruby.two_fer.avoid_string_format",     #"Rather than using string's format/percentage method, use interpolation"
@@ -16,6 +18,8 @@ class Analyze < ExerciseAnalyzer
     include Mandate
 
     def analyze!
+      #target_method.pry
+
       # Note that all "check_...!" methods exit this method if the solution
       # is approved or disapproved, so each step is only called if the
       # previous one has not resolved what to do.
@@ -46,6 +50,10 @@ def analyze!
       # default parameter if we see one.
       check_for_conditional_on_default_argument!
 
+      # Sometimes, rather than setting a variable, people reassign the input param e.g.
+      #   use name ||= "you"
+      check_for_reassigned_parameter!
+
       # Sometimes people specify the names (if name == "Alice" ...). If we
       # do this, suggest using string interpolation to make us of the
       # parameter, rather than using a conditional on it.
@@ -90,13 +98,33 @@ def check_for_optimal_solution!
       # statement using interpolation. Other solutions might be approved
       # but this is the only one that we would approve without comment.
 
+
       return unless default_argument_is_optimal?
       return unless one_line_solution?
-      return unless using_string_interpolation?
 
-      # If the interpolation has more than three components, then they've
+      if target_method.body.return_type?
+        loc = target_method.body.children.first
+        explicit_return = true
+      elsif target_method.body.dstr_type?
+        loc = target_method.body
+        explicit_return = false
+      else
+        # We're only expecting one of these two scenarios
+        return
+      end
+
+      # Return unless we have string interpolation
+      return unless loc.dstr_type?
+
+      # If the interpolation does not follow this pattern then the student has
       # done something weird, so let's get a mentor to look at it!
-      refer_to_mentor! unless string_interpolation_has_three_components?
+      refer_to_mentor! unless loc.children[0] == s(:str, "One for ") &&
+                              loc.children[1] == s(:begin, s(:lvar, first_parameter_name)) &&
+                              loc.children[2] == s(:str, ", one for me.")
+
+      # If we're got a correct solution but they've given an explicit
+      # return then let's warn them against that.
+      disapprove!(:explicit_return) if explicit_return
 
       approve_if_whitespace_is_sensible!
     end
@@ -109,6 +137,9 @@ def check_for_correct_solution_without_string_interpolaton!
 
       loc = SA::Helpers.extract_first_line_from_method(target_method)
 
+      # Protect against explicit return
+      #loc = loc.children.first if loc.return_type?
+
       # In the case of:
       # "One for " + name + ", one for me."
       if loc.method_name == :+ &&
@@ -139,28 +170,65 @@ def check_for_correct_solution_without_string_interpolaton!
     end
 
     def check_for_conditional_on_default_argument!
-      loc = SA::Helpers.extract_first_line_from_method(target_method)
+      stmts = SA::Helpers.extract_nodes(:if, target_method)
 
-      # If we don't have a conditional, then let's get out of here.
-      #
-      # TODO: We might want to refactor this to extract a conditional from the
-      # method rather than insist on it being the first line.
-      return unless loc.type == :if
+      # If there are no statements then we can safely get out of here.
+      return if stmts.empty?
+
+      # If there is more than one statement, then let's refer this to a mentor
+      refer_to_mentor! if stmts.size > 1
 
-      # Get the clause of the conditional (i.e. the bit after the "if" keyword)
-      conditional = SA::Helpers.extract_conditional_clause(loc)
+      # Now we have the statement, let's also get the conditional
+      # clause (i.e. the bit after the "if" keyword)
+      if_statement = stmts.first
+      conditional = SA::Helpers.extract_conditional_clause(if_statement)
+
+      if SA::Helpers.lvar?(conditional, first_parameter_name)
+        # Let's warn about using a better default if they do `if name`
+        disapprove!(:incorrect_default_param)
+
+      elsif conditional.send_type? &&
+          SA::Helpers.lvar?(conditional.receiver, first_parameter_name)
+          conditional.first_argument == :nil?
+        # Let's warn about using a better default if they do `if name.nil?`
+        disapprove!(:incorrect_default_param)
 
       # Let's warn about using a better default if they `if name == nil`
-      if SA::Helpers.lvar?(conditional.receiver, :name) &&
+      elsif SA::Helpers.lvar?(conditional.receiver, first_parameter_name) &&
          conditional.first_argument == default_argument
         disapprove!(:incorrect_default_param)
-      end
 
       # Same thing but if they do it the other way round, i.e. `if nil == name`
-      if conditional.receiver == default_argument &&
-         SA::Helpers.lvar?(conditional.first_argument, :name)
+      elsif SA::Helpers.lvar?(conditional.first_argument, first_parameter_name) &&
+         conditional.receiver == default_argument
         disapprove!(:incorrect_default_param)
       end
+
+      # If we have an if without that does not do an expected comparison,
+      # let's refer this to a mentor and get out of here!
+      refer_to_mentor!
+    end
+
+    def check_for_reassigned_parameter!
+      assignments = SA::Helpers.extract_nodes(:or_asgn, target_method)
+
+      # If there are no statements then we can safely get out of here.
+      return if assignments.empty?
+
+      # If there is more than one statement, then let's refer this to a mentor
+      refer_to_mentor! if assignments.size > 1
+
+      # Now we what is being reassigned is the param and it's being rassigned to "you"
+      # let's warn the user
+      assignment = assignments.first
+      if assignment.children[0] == s(:lvasgn, first_parameter_name) &&
+         assignment.children[1] == s(:str, "you")
+        disapprove!(:reassigning_param)
+      end
+
+      # If we have a reassignment that doesn't conform to this
+      # let's refer this to a mentor and get out of here!
+      refer_to_mentor!
     end
 
     # ###
@@ -174,15 +242,6 @@ def one_line_solution?
       target_method.body.line_count == 1
     end
 
-    def using_string_interpolation?
-      target_method.body.dstr_type?
-    end
-
-    def string_interpolation_has_three_components?
-      #target_method.body.pry
-      target_method.body.children.size == 3
-    end
-
     # REFACTOR: This could be refactored to strip blank
     # lines and then use each_cons(2).
     def indentation_is_sensible?

lib/generic/extract_nodes.rb

@@ -0,0 +1,22 @@
+module SA
+  class ExtractNodes < Parser::AST::Processor
+    include Mandate
+
+    def initialize(node_type, node_to_search)
+      @node_to_search = node_to_search
+      @found_methods = []
+
+      define_singleton_method "on_#{node_type}" do |node|
+        found_methods << node
+      end
+    end
+
+    def call
+      process(node_to_search)
+      found_methods
+    end
+
+    private
+    attr_reader :node_to_search, :found_methods
+  end
+end

lib/generic/helpers.rb

@@ -1,4 +1,10 @@
 module SA
+  module InlineHelpers
+    def s(type, *children)
+      Parser::AST::Node.new(type, children)
+    end
+  end
+
   module Helpers
     def self.extract_module_or_class(*args)
       ExtractModuleOrClass.(*args)
@@ -8,6 +14,10 @@ def self.extract_module_method(*args)
       ExtractModuleMethod.(*args)
     end
 
+    def self.extract_nodes(*args)
+      ExtractNodes.(*args)
+    end
+
     def self.extract_first_line_from_method(method)
       # A begin block signifies multiple lines
       # so we return the first line.

manual_test.rb

@@ -0,0 +1,19 @@
+$LOAD_PATH.unshift File.expand_path("../../lib", __FILE__)
+require "analyzer"
+
+output = []
+["two-fer"].each do |slug|
+  path = File.expand_path("#{__FILE__}/../../test/fixtures/#{slug}")
+  statuses = Hash.new {|h,k|h[k] = 0}
+  Dir.foreach(path) do |id|
+    next if id == "." || id == ".."
+
+    begin
+      analysis = TwoFer::Analyze.(File.read("#{path}/#{id}/two_fer.rb"))
+      p id and exit if analysis[:status] == :refer_to_mentor
+    rescue
+      p id and exit
+      statuses["exploded"] += 1
+    end
+  end
+end

scripts/extract_first_refer_to_mentor.rb

@@ -0,0 +1,17 @@
+$LOAD_PATH.unshift File.expand_path("../../lib", __FILE__)
+require "analyzer"
+require 'pp'
+require 'pry'
+
+source = %q{
+class TwoFer
+    def self.two_fer(name = "you")
+        return ("One for " + name + ", one for me.")
+    end
+end
+
+}
+
+puts "\n\n\n\n"
+pp TwoFer::Analyze.(source)
+puts "\n\n\n\n"