Added local variables to blocks

kevinrutherford · May 4, 2009 · 48a8d4d · 48a8d4d
1 parent 0554d66
commit 48a8d4d
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 24 deletions.
diff --git a/History.txt b/History.txt
@@ -2,6 +2,7 @@
 
 === Minor enhancements
 * LargeClass now also warns about any class with > 9 instance variables (#6)
+* Uses ruby2ruby to display code better
 
 == 1.1.0 2009-04-10
 

diff --git a/lib/reek/block_context.rb b/lib/reek/block_context.rb
@@ -1,3 +1,4 @@
+require 'set'
 require 'reek/code_context'
 
 module Reek
@@ -28,6 +29,7 @@ def initialize(outer, exp)
       @parameters = exp[0] if exp
       @parameters ||= []
       @parameters.extend(ParameterSet)
+      @local_variables = Set.new
     end
 
     def inside_a_block?
@@ -43,14 +45,15 @@ def nested_block?
     end
 
     def record_local_variable(sym)
+      @local_variables << Name.new(sym)
     end
 
     def outer_name
       "#{@outer.outer_name}#{@name}/"
     end
 
     def variable_names
-      @parameters.names
+      @parameters.names + @local_variables.to_a
     end
   end
 end
diff --git a/spec/reek/block_context_spec.rb b/spec/reek/block_context_spec.rb
@@ -31,4 +31,10 @@
     element = BlockContext.new(mc, s(s(:lasgn, :x)))
     mc.variable_names.should be_empty
   end
+
+  it 'records local variables' do
+    bctx = BlockContext.new(StopContext.new, nil)
+    bctx.record_local_variable(:q2)
+    bctx.variable_names.should include(Name.new(:q2))
+  end
 end
diff --git a/spec/reek/smells/uncommunicative_name_spec.rb b/spec/reek/smells/uncommunicative_name_spec.rb
@@ -46,6 +46,11 @@
   it 'should report variable name only once' do
     'def simple(fred) x = jim(45); x = y end'.should reek_only_of(:UncommunicativeName, /x/)
   end
+
+  it 'should report a bad name inside a block' do
+    src = 'def clean(text) text.each { q2 = 3 } end'
+    src.should reek_of(:UncommunicativeName, /q2/)
+  end
 end
 
 describe UncommunicativeName, "parameter name" do

diff --git a/spec/reek/smells/utility_function_spec.rb b/spec/reek/smells/utility_function_spec.rb
@@ -86,4 +86,8 @@ def shelve(val)
   it 'should not report references to self' do
     'def into; self; end'.should_not reek
   end
+
+  it 'should recognise an ivar reference within a block' do
+    'def clean(text) text.each { @fred = 3} end'.should_not reek
+  end
 end
diff --git a/spec/slow/inline_spec.rb b/spec/slow/inline_spec.rb
@@ -5,15 +5,15 @@
     ruby = File.new("#{SAMPLES_DIR}/inline.rb").to_source
     ruby.should reek_of(:ControlCouple, /Inline::C#parse_signature/, /raw/)
     ruby.should reek_of(:ControlCouple, /Module#inline/, /options/)
-    ruby.should reek_of(:Duplication, /Inline::C#build/, /\$\?\.==\(0\)/)
+    ruby.should reek_of(:Duplication, /Inline::C#build/, /\(\$\?\ == 0\)/)
     ruby.should reek_of(:Duplication, /Inline::C#build/, /Inline.directory/)
     ruby.should reek_of(:Duplication, /Inline::C#build/, /io.puts/)
-    ruby.should reek_of(:Duplication, /Inline::C#build/, /io.puts\(#endif\)/)
-    ruby.should reek_of(:Duplication, /Inline::C#build/, /io.puts\(#ifdef __cplusplus\)/)
-    ruby.should reek_of(:Duplication, /Inline::C#crap_for_windoze/, /Config::CONFIG\[libdir\]/)
-    ruby.should reek_of(:Duplication, /Inline::C#generate/, /result.sub!\(\(\?-mix:\\A\\n\), \)/)
-    ruby.should reek_of(:Duplication, /Inline::C#generate/, /signature\[args\]/)
-    ruby.should reek_of(:Duplication, /Inline::C#generate/, /signature\[args\].map/)
+    ruby.should reek_of(:Duplication, /Inline::C#build/, /io.puts\("#endif"\)/)
+    ruby.should reek_of(:Duplication, /Inline::C#build/, /io.puts\("#ifdef __cplusplus"\)/)
+    ruby.should reek_of(:Duplication, /Inline::C#crap_for_windoze/, /Config::CONFIG\["libdir"\]/)
+    ruby.should reek_of(:Duplication, /Inline::C#generate/, /result.sub!\(\/\\A\\n\/, ""\)/)
+    ruby.should reek_of(:Duplication, /Inline::C#generate/, /signature\["args"\]/)
+    ruby.should reek_of(:Duplication, /Inline::C#generate/, /signature\["args"\].map/)
     ruby.should reek_of(:Duplication, /Inline::C#initialize/, /stack.empty?/)
     ruby.should reek_of(:Duplication, /Inline::self.rootdir/, /env.nil?/)
     ruby.should reek_of(:Duplication, /Module#inline/, /Inline.const_get\(lang\)/)

diff --git a/spec/slow/optparse_spec.rb b/spec/slow/optparse_spec.rb
@@ -14,21 +14,21 @@
     ruby.should reek_of(:Duplication, /OptionParser#Completion::complete/, /candidates.size/)
     ruby.should reek_of(:Duplication, /OptionParser#Completion::complete/, /k.id2name/)
     ruby.should reek_of(:Duplication, /OptionParser#Switch#parse_arg/, /s.length/)
-    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /block.max/)
-    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /block.max.to_i/)
-    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /indent.+\(l\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /left.collect \{ \|s\| s\.length \}\.max/)
+    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /left.collect \{ \|s\| s\.length \}\.max\.to_i/)
+    ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /\(indent \+ l\)/)
     ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /left.collect/)
     ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /left.shift/)
     ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /left\[-1\]/)
     ruby.should reek_of(:Duplication, /OptionParser#Switch#summarize/, /s.length/)
     ruby.should reek_of(:Duplication, /OptionParser#getopts/, /result\[opt\] = false/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /default_style.guess\(arg=a\)/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /long.<<\(o=q.downcase\)/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.method\(convert\)/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.method\(convert\).to_proc/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.respond_to\?\(convert\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /default_style.guess\(arg = a\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /\(long << o = q.downcase\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.method\(:convert\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.method\(:convert\).to_proc/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /pattern.respond_to\?\(:convert\)/)
     ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /q.downcase/)
-    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /sdesc.<<\("-\#\{q\}"\)/)
+    ruby.should reek_of(:Duplication, /OptionParser#make_switch/, /\(sdesc << "-\#\{q\}"\)/)
     ruby.should reek_of(:Duplication, /OptionParser#order/, /argv\[0\]/)
     ruby.should reek_of(:Duplication, /OptionParser#parse/, /argv\[0\]/)
     ruby.should reek_of(:Duplication, /OptionParser#parse_in_order/, /\$\!.set_option\(arg, true\)/)
@@ -46,7 +46,7 @@
     ruby.should reek_of(:LargeClass, /OptionParser/)
     ruby.should reek_of(:LongMethod, /OptionParser#Completion::complete/)
     ruby.should reek_of(:LongMethod, /OptionParser#List#update/)
-    ruby.should reek_of(:LongMethod, /OptionParser#Switch#PlacedArgument#parse/)
+#    ruby.should reek_of(:LongMethod, /OptionParser#Switch#PlacedArgument#parse/)
     ruby.should reek_of(:LongMethod, /OptionParser#Switch#parse_arg/)
     ruby.should reek_of(:LongMethod, /OptionParser#Switch#summarize/)
     ruby.should reek_of(:LongMethod, /OptionParser#getopts/)

diff --git a/spec/slow/redcloth_spec.rb b/spec/slow/redcloth_spec.rb
@@ -13,18 +13,18 @@
     ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /depth.last/)
     ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /depth.last.length/)
     ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /depth\[i\]/)
-    ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /line_id.-\(1\)/)
-    ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /lines\[line_id.-\(1\)\]/)
+    ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /\(line_id - 1\)/)
+    ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /lines\[\(line_id - 1\)\]/)
     ruby.should reek_of(:Duplication, /RedCloth#block_textile_lists/, /tl.length/)
     ruby.should reek_of(:Duplication, /RedCloth#clean_html/, /tags\[tag\]/)
     ruby.should reek_of(:Duplication, /RedCloth#pba/, /\$1.length/)
     ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /@pre_list.last/)
-    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /@pre_list.last.<<\(line\)/)
-    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /codepre.-\(used_offtags.length\)/)
-    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /codepre.-\(used_offtags.length\).>\(0\)/)
+    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /\(@pre_list.last << line\)/)
+    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /\(codepre - used_offtags.length\)/)
+    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /\(\(codepre - used_offtags.length\) > 0\)/)
     ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /codepre.zero?/)
     ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /used_offtags.length/)
-    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /used_offtags\[notextile\]/)
+    ruby.should reek_of(:Duplication, /RedCloth#rip_offtags/, /used_offtags\["notextile"\]/)
     ruby.should reek_of(:FeatureEnvy, /RedCloth#block_markdown_atx/, /text/)
     ruby.should reek_of(:FeatureEnvy, /RedCloth#block_markdown_rule/, /text/)
     ruby.should reek_of(:FeatureEnvy, /RedCloth#block_markdown_setext/, /text/)