Improved algorithm for assessing Feature Envy

lib/reek/method_checker.rb

@@ -15,6 +15,7 @@ def initialize(smells, klass_name)
       @refs = ObjectRefs.new
       @lvars = Set.new
       @num_statements = 0
+      @depends_on_self = false
     end
 
     def process_defn(exp)
@@ -32,17 +33,17 @@ def process_args(exp)
     end
 
     def process_attrset(exp)
-      @refs.record_reference_to_self if /^@/ === exp[1].to_s
+      @depends_on_self = true if /^@/ === exp[1].to_s
       s(exp)
     end
 
     def process_lit(exp)
-      @refs.record_ref(exp)
+      val = exp[1]
+      @depends_on_self = true if val == :self
       s(exp)
     end
 
     def process_lvar(exp)
-      @refs.record_ref(exp)
       s(exp)
     end
 
@@ -71,30 +72,32 @@ def process_yield(exp)
 
     def process_call(exp)
       receiver, meth, args = exp[1..3]
+      @refs.record_ref(receiver)
       process(receiver)
       process(args) if args
       s(exp)
     end
 
     def process_fcall(exp)
+      @depends_on_self = true
       @refs.record_reference_to_self
       process(exp[2]) if exp.length >= 3
       s(exp)
     end
 
     def process_cfunc(exp)
-      @refs.record_reference_to_self
+      @depends_on_self = true
       s(exp)
     end
 
     def process_vcall(exp)
-      @refs.record_reference_to_self
+      @depends_on_self = true
       s(exp)
     end
 
     def process_ivar(exp)
       UncommunicativeName.check(exp[1], self, 'field')
-      @refs.record_reference_to_self  # BUG: also a ref to the ivar!
+      @depends_on_self = true
       s(exp)
     end
 
@@ -109,13 +112,13 @@ def process_lasgn(exp)
     end
 
     def process_iasgn(exp)
-      @refs.record_reference_to_self
+      @depends_on_self = true
       process(exp[2])
       s(exp)
     end
 
     def process_self(exp)
-      @refs.record_reference_to_self
+      @depends_on_self = true
       s(exp)
     end
 
@@ -140,16 +143,20 @@ def self.is_override?(class_name, method_name)
       return false unless klass.superclass
       klass.superclass.instance_methods.include?(method_name)
     end
+
+    def method_name
+      @description.to_s.split('#')[1]
+    end
 
     def is_override?
-      MethodChecker.is_override?(@class_name, @description.to_s.split('#')[1])
+      MethodChecker.is_override?(@class_name, method_name)
     end
 
     def check_method_properties
       @lvars.each {|lvar| UncommunicativeName.check(lvar, self, 'local variable') }
-      @refs.record_reference_to_self if is_override?
-      FeatureEnvy.check(@refs, self) unless UtilityFunction.check(@refs, self)
-      LongMethod.check(@num_statements, self)
+      @depends_on_self = true if is_override?
+      FeatureEnvy.check(@refs, self) unless UtilityFunction.check(@depends_on_self, self)
+      LongMethod.check(@num_statements, self) unless method_name == 'initialize'
     end
   end
 end

lib/reek/object_refs.rb

@@ -1,6 +1,8 @@
 $:.unshift File.dirname(__FILE__)
 
+require 'rubygems'
 require 'sexp'
+require 'reek/printer'
 
 module Reek
 
@@ -9,31 +11,43 @@ class ObjectRefs
 
     def initialize
       @refs = Hash.new(0)
+      record_reference_to_self
     end
 
     def record_reference_to_self
       record_ref(SELF_REF)
     end
 
     def record_ref(exp)
-      @refs[exp] += 1
-#      puts "record_ref(#{exp.inspect}) -> #{@refs.inspect}"
+      type = exp[0]
+      case type
+      when :gvar
+        return
+      when :self
+        record_reference_to_self
+      else
+        @refs[exp] += 1
+      end
     end
 
     def refs_to_self
       @refs[SELF_REF]
     end
 
+    def max_refs
+      @refs.values.max or 0
+    end
+
     # TODO
     # Should be moved to Hash; but Hash has 58 methods, and there's currently
     # no way to turn off that report; which would therefore make the tests fail
     def max_keys
-      max = @refs.values.max or 0
-      @refs.keys.select { |key| @refs[key] == max }
+      max = max_refs
+      @refs.reject {|k,v| v != max}.keys
     end
 
     def self_is_max?
-      max_keys.include?(SELF_REF)
+      max_keys.length == 0 || @refs[SELF_REF] == max_refs
     end
   end
 end

lib/reek/printer.rb

@@ -30,6 +30,11 @@ def process_default(exp)
       s(exp)
     end
 
+    def process_array(exp)
+      @report = Printer.print(exp[1])
+      s(exp)
+    end
+
     def process_lvar(exp)
       @report = exp[1].to_s
       s(exp)
@@ -56,7 +61,26 @@ def process_dvar(exp)
     end
 
     def process_gvar(exp)
-      @report = Printer.print(exp[1])
+      @report = exp[1].to_s
+      s(exp)
+    end
+
+    def process_ivar(exp)
+      @report = exp[1].to_s
+      s(exp)
+    end
+
+    def process_vcall(exp)
+      meth, args = exp[1..2]
+      @report = meth.to_s
+      @report += "(#{Printer.print(args)})" if args
+      s(exp)
+    end
+
+    def process_fcall(exp)
+      meth, args = exp[1..2]
+      @report = meth.to_s
+      @report += "(#{Printer.print(args)})" if args
       s(exp)
     end
 
@@ -84,7 +108,7 @@ def process_iter(exp)
     def process_call(exp)
       receiver, meth, args = exp[1..3]
       @report = "#{Printer.print(receiver)}.#{meth}"
-      @report += "(#{args})" if args
+      @report += "(#{Printer.print(args)})" if args
       s(exp)
     end
   end

lib/reek/smells.rb

@@ -109,8 +109,8 @@ def detailed_report
   end
 
   class UtilityFunction < Smell
-    def recognise?(refs)
-      refs.refs_to_self == 0
+    def recognise?(depends_on_self)
+      !depends_on_self
     end
 
     def detailed_report
@@ -121,10 +121,14 @@ def detailed_report
   class LargeClass < Smell
     MAX_ALLOWED = 25
 
+    def self.non_inherited_methods(klass)
+      return klass.instance_methods if klass.superclass.nil?
+      klass.instance_methods - klass.superclass.instance_methods
+    end
+
     def recognise?(name)
       klass = Object.const_get(name) rescue return
-      super_methods = klass.superclass ? klass.superclass.instance_methods : []
-      @num_methods = klass.instance_methods.length - super_methods.length
+      @num_methods = LargeClass.non_inherited_methods(klass).length
       @num_methods > MAX_ALLOWED
     end
 
@@ -139,11 +143,15 @@ def initialize(context, symbol_type)
       @symbol_type = symbol_type
     end
 
+    def self.effective_length(name)
+      return 500 if name == '*'
+      name = name[1..-1] while /^@/ === name
+      name.length
+    end
+
     def recognise?(symbol)
       @symbol = symbol.to_s
-      return false if @symbol == '*'
-      min_len = (/^@/ === @symbol) ? 3 : 2;
-      @symbol.length < min_len
+      UncommunicativeName.effective_length(@symbol) < 2
     end
 
     def detailed_report