Refactored to add ObjectRefs class

lib/reek/method_checker.rb

@@ -2,6 +2,7 @@
 
 require 'reek/checker'
 require 'reek/smells'
+require 'reek/object_refs'
 require 'set'
 
 module Reek
@@ -11,7 +12,7 @@ class MethodChecker < Checker
     def initialize(smells, klass_name)
       super(smells)
       @class_name = @description = klass_name
-      @calls = Hash.new(0)
+      @refs = ObjectRefs.new
       @lvars = Set.new
       @num_statements = 0
     end
@@ -31,7 +32,7 @@ def process_args(exp)
     end
 
     def record_reference_to_self
-      @calls[Sexp.from_array([:lit, :self])] += 1
+      @refs.record_reference_to_self
     end
 
     def process_attrset(exp)
@@ -40,7 +41,7 @@ def process_attrset(exp)
     end
 
     def process_lit(exp)
-      @calls[exp] += 1
+      @refs.record_ref(exp)
       s(exp)
     end
 
@@ -67,7 +68,6 @@ def process_yield(exp)
     def process_call(exp)
       record_receiver(exp[1])
       params = exp[3]
-      process_actual_parameters(params)
       process(params) if exp.length > 3
       s(exp)
     end
@@ -96,7 +96,7 @@ def process_ivar(exp)
 
     def process_lasgn(exp)
       @lvars << exp[1]
-      @calls[s(:lvar, exp[1])] += 1
+      @refs.record_ref(s(:lvar, exp[1]))
       process(exp[2])
       s(exp)
     end
@@ -126,7 +126,7 @@ def self.is_global_variable?(exp)
 
     def record_receiver(exp)
       receiver = MethodChecker.unpack_array(process(exp))
-      @calls[receiver] += 1 unless MethodChecker.is_global_variable?(receiver)
+      @refs.record_ref(receiver) unless MethodChecker.is_global_variable?(receiver)
     end
 
     def self.unpack_array(receiver)
@@ -135,35 +135,24 @@ def self.unpack_array(receiver)
       receiver
     end
 
-    def is_override?
+    def self.is_override?(class_name, method_name)
       begin
-        klass = Object.const_get(@class_name)
+        klass = Object.const_get(class_name)
       rescue
         return false
       end
-      klass.superclass.instance_methods.include?(@description.to_s.split('#')[1])
+      klass.superclass.instance_methods.include?(method_name)
+    end
+
+    def is_override?
+      MethodChecker.is_override?(@class_name, @description.to_s.split('#')[1])
     end
 
     def check_method_properties
       @lvars.each {|lvar| UncommunicativeName.check(lvar, self, 'local variable') }
       record_reference_to_self if is_override?
-      FeatureEnvy.check(@calls, self) unless UtilityFunction.check(@calls, self)
+      FeatureEnvy.check(@refs, self) unless UtilityFunction.check(@refs, self)
       LongMethod.check(@num_statements, self)
     end
-
-    def process_actual_parameters(exp)
-      return unless Array === exp and exp[0] == :array
-      exp[1..-1].each do |param|
-        if Array === param
-          if param.length == 1
-            record_reference_to_self if param[0] == :self
-          else
-            @calls[param] += 1
-          end
-        else
-          record_reference_to_self if param == :self
-        end
-      end
-    end
   end
 end

lib/reek/object_refs.rb

@@ -0,0 +1,39 @@
+$:.unshift File.dirname(__FILE__)
+
+require 'reek/checker'
+require 'reek/smells'
+require 'set'
+
+module Reek
+
+  class ObjectRefs
+
+    def initialize
+      @refs = Hash.new(0)
+    end
+
+    def record_reference_to_self
+      record_ref Sexp.from_array([:lit, :self])
+    end
+
+    def record_ref(exp)
+      @refs[exp] += 1
+    end
+
+    def refs_to_self
+      @refs[Sexp.from_array([:lit, :self])]
+    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 }
+    end
+
+    def self_is_max?
+      max_keys.include?(Sexp.from_array([:lit, :self]))
+    end
+  end
+end

lib/reek/smells.rb

@@ -18,12 +18,9 @@ def initialize(context, arg=nil)
 
     def self.check(exp, context, arg=nil)
       smell = new(context, arg)
-      if smell.recognise?(exp)
-        context.report(smell)
-        true
-      else
-        false
-      end
+      return false unless smell.recognise?(exp)
+      context.report(smell)
+      true
     end
 
     def recognise?(stuff)
@@ -99,34 +96,21 @@ def detailed_report
   end
 
   class FeatureEnvy < Smell
-
-    # 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 self.max_keys(calls)
-      max = calls.values.max or return [Sexp.from_array([:lit, :self])]
-      calls.keys.select { |key| calls[key] == max }
-    end
-
-    def initialize(context, *receivers)
-      super(context)
-      @receivers = receivers == [nil] ? [] : receivers
-    end
 
-    def recognise?(calls)
-      @receivers = FeatureEnvy.max_keys(calls)
-      return !(@receivers.include?(Sexp.from_array([:lit, :self])))
+    def recognise?(refs)
+      @refs = refs
+      !refs.self_is_max?
     end
 
     def detailed_report
-      receiver = @receivers.map {|r| Printer.print(r)}.sort.join(' and ')
+      receiver = @refs.max_keys.map {|r| Printer.print(r)}.sort.join(' and ')
       "#{@context} uses #{receiver} more than self"
     end
   end
 
   class UtilityFunction < Smell
-    def recognise?(calls)
-      calls[Sexp.from_array([:lit, :self])] == 0
+    def recognise?(refs)
+      refs.refs_to_self == 0
     end
 
     def detailed_report

spec/integration_spec.rb

@@ -38,13 +38,3 @@
     end
   end
 end
-
-describe 'Reek source code:' do
-  Dir['lib/**/*.rb'].each do |source|
-    describe source do
-      it 'should report no smells' do
-        `ruby -Ilib bin/reek #{source}`.should == "\n"
-      end
-    end
-  end
-end

spec/reek/feature_envy_spec.rb

@@ -45,11 +45,3 @@
     @rpt.should be_empty
   end
 end
-
-describe FeatureEnvy, '#report' do
-  it 'should report the envious host' do
-    mchk = MethodChecker.new([], 'Class')
-    smell = FeatureEnvy.new(mchk, [:lvar, :fred])
-    smell.report.should match(/fred/)
-  end
-end

spec/reek/method_checker_spec.rb

@@ -47,3 +47,21 @@
     rpt.should be_empty
   end
 end
+
+describe MethodChecker, '#is_override?' do
+  it 'should be false for non-override method' do
+    MethodChecker.is_override?('String', 'gsub').should == false
+  end
+
+  it 'should be true for overriding method' do
+    MethodChecker.is_override?('MethodChecker', 'to_s').should == true
+  end
+
+  it 'should be false for non-existent class' do
+    MethodChecker.is_override?('Flibble', 'to_s').should == false
+  end
+
+  it 'should be true for smells' do
+    MethodChecker.is_override?('UtilityFunction', 'recognise?').should == true
+  end
+end

spec/reek/object_refs_spec.rb

@@ -0,0 +1,130 @@
+require File.dirname(__FILE__) + '/../spec_helper.rb'
+
+require 'reek/object_refs'
+
+include Reek
+
+describe ObjectRefs, 'when empty' do
+  before(:each) do
+    @refs = ObjectRefs.new
+  end
+
+  it 'should report no refs to self' do
+    @refs.refs_to_self.should == 0
+  end
+end
+
+describe ObjectRefs, 'with no refs to self' do
+  before(:each) do
+    @refs = ObjectRefs.new
+    @refs.record_ref('a')
+    @refs.record_ref('b')
+    @refs.record_ref('a')
+  end
+
+  it 'should report no refs to self' do
+    @refs.refs_to_self.should == 0
+  end
+
+  it 'should report :a as the max' do
+    @refs.max_keys.should == ['a']
+  end
+
+  it 'should not report self as the max' do
+    @refs.self_is_max?.should == false
+  end
+end
+
+describe ObjectRefs, 'with one ref to self' do
+  before(:each) do
+    @refs = ObjectRefs.new
+    @refs.record_ref('a')
+    @refs.record_ref('b')
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+  end
+
+  it 'should report 1 ref to self' do
+    @refs.refs_to_self.should == 1
+  end
+
+  it 'should report :a as the max' do
+    @refs.max_keys.should == ['a']
+  end
+
+  it 'should not report self as the max' do
+    @refs.self_is_max?.should == false
+  end
+end
+
+describe ObjectRefs, 'with many refs to self' do
+  before(:each) do
+    @refs = ObjectRefs.new
+    @refs.record_reference_to_self
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+    @refs.record_ref('b')
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+  end
+
+  it 'should report all refs to self' do
+    @refs.refs_to_self.should == 3
+  end
+
+  it 'should report self among the max' do
+    @refs.max_keys.should == [Sexp.from_array([:lit, :self])]
+  end
+
+  it 'should report self as the max' do
+    @refs.self_is_max?.should == true
+  end
+end
+
+describe ObjectRefs, 'when self is not the only max' do
+  before(:each) do
+    @refs = ObjectRefs.new
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+    @refs.record_ref('b')
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+  end
+
+  it 'should report all refs to self' do
+    @refs.refs_to_self.should == 2
+  end
+
+  it 'should report self among the max' do
+    @refs.max_keys.should be_include('a')
+    @refs.max_keys.should be_include(Sexp.from_array([:lit, :self]))
+  end
+
+  it 'should report self as the max' do
+    @refs.self_is_max?.should == true
+  end
+end
+
+describe ObjectRefs, 'when self is not among the max' do
+  before(:each) do
+    @refs = ObjectRefs.new
+    @refs.record_ref('a')
+    @refs.record_ref('b')
+    @refs.record_ref('a')
+    @refs.record_reference_to_self
+    @refs.record_ref('b')
+  end
+
+  it 'should report all refs to self' do
+    @refs.refs_to_self.should == 1
+  end
+
+  it 'should not report self among the max' do
+    @refs.max_keys.should be_include('a')
+    @refs.max_keys.should be_include('b')
+  end
+
+  it 'should not report self as the max' do
+    @refs.self_is_max?.should == false
+  end
+end

spec/reek/report_spec.rb

@@ -59,7 +59,7 @@
   end
 
   it 'should return non-0 for different smells' do
-    @sorter.compare(LongMethod.new('x'), FeatureEnvy.new('y', 1)).should == -1
+    @sorter.compare(LongMethod.new('x'), LargeClass.new('y')).should == -1
   end
 end
 
@@ -77,6 +77,6 @@
   end
 
   it 'should differentiate different smells with identical contexts' do
-    @sorter.compare(LongMethod.new('x'), FeatureEnvy.new('x', 2)).should == 1
+    @sorter.compare(LongMethod.new('x'), LargeClass.new('x')).should == 1
   end
 end