spec/reek/tree_dresser_spec.rb @@ -1,4 +1,8 @@ -== 1.2.2 (2009-11-2) +== 1.2.4 (under development in github) + +=== Minor Changes + +== 1.2.3 (2009-11-2) === Minor Changes * New smell: Attribute (disabled by default) History.txt @@ -23,7 +23,11 @@ module Reek @myself = nil end - def each(type, ignoring, &blk) + def local_nodes(type, &blk) + each_node(type, [:class, :module], &blk) + end + + def each_node(type, ignoring, &blk) if block_given? @exp.look_for(type, ignoring, &blk) else lib/reek/code_context.rb @@ -91,7 +91,6 @@ module Reek def process_call(exp) @element.record_call_to(exp) - @element.check_for_attribute_declaration(exp) process_default(exp) end lib/reek/code_parser.rb @@ -85,7 +85,6 @@ module Reek def record_call_to(exp) @calls[exp] += 1 record_receiver(exp) - check_for_attribute_declaration(exp) end def record_receiver(exp) lib/reek/method_context.rb @@ -18,12 +18,9 @@ module Reek end end - attr_reader :attributes - def initialize(outer, name, exp) super(outer, exp) @name = name - @attributes = Set.new @parsed_methods = [] end @@ -40,20 +37,10 @@ module Reek @parsed_methods.select {|meth| meth.parameters.length >= min_clump_size } end - def record_attribute(attr) - @attributes << Name.new(attr) - end - def record_method(meth) @parsed_methods << meth end - def check_for_attribute_declaration(exp) - if [:attr, :attr_reader, :attr_writer, :attr_accessor].include? exp[2] - exp[3][1..-1].each {|arg| record_attribute(arg[1])} - end - end - def outer_name "#{@outer.outer_name}#{@name}::" end lib/reek/module_context.rb @@ -18,6 +18,8 @@ module Reek # class Attribute < SmellDetector + ATTRIBUTE_METHODS = [:attr, :attr_reader, :attr_writer, :attr_accessor] + def self.contexts # :nodoc: [:class, :module] end @@ -39,10 +41,25 @@ module Reek # MethodContext, ClassContext and ModuleContext all know which # calls constitute attribute declarations. Need a method on # ModuleContext: each_public_call.select [names] {...} - mod.attributes.each do |attr| + attributes_in(mod).each do |attr| found(mod, "declares the attribute #{attr}") end end + + # + # Collects the names of the class variables declared and/or used + # in the given module. + # + def attributes_in(mod) + result = Set.new + collector = proc { |call_node| + if ATTRIBUTE_METHODS.include?(call_node.method_name) + call_node.arg_names.each {|arg| result << arg } + end + } + mod.local_nodes(:call, &collector) + result + end end end end lib/reek/smells/attribute.rb @@ -35,7 +35,7 @@ module Reek result = Set.new collector = proc { |cvar_node| result << cvar_node.name } [:cvar, :cvasgn, :cvdecl].each do |stmt_type| - mod.each(stmt_type, [:class, :module], &collector) + mod.local_nodes(stmt_type, &collector) end result end lib/reek/smells/class_variable.rb @@ -45,7 +45,7 @@ module Reek end def check_num_methods(klass) # :nodoc: - actual = klass.each(:defn, [:class, :module]).length + actual = klass.local_nodes(:defn).length return if actual <= value(MAX_ALLOWED_METHODS_KEY, klass, DEFAULT_MAX_METHODS) found(klass, "has at least #{actual} methods") end lib/reek/smells/large_class.rb @@ -63,8 +63,8 @@ module Reek condition = node.condition result[condition] += 1 unless condition == s(:call, nil, :block_given?, s(:arglist)) } - klass.each(:if, [:class, :module], &collector) - klass.each(:case, [:class, :module], &collector) + klass.local_nodes(:if, &collector) + klass.local_nodes(:case, &collector) result end end lib/reek/smells/simulated_polymorphism.rb @@ -32,43 +32,50 @@ module Reek end end - module CvarNode - def name - self[1] + module SexpExtensions + module CaseNode + def condition + self[1] + end end - end - module IfNode - def condition - self[1] + module CallNode + def receiver() self[1] end + def method_name() self[2] end + def args() self[3] end + def arg_names + args[1..-1].map {|arg| arg[1]} + end end - end - module CaseNode - def condition - self[1] + module CvarNode + def name() self[1] end + end + + CvasgnNode = CvarNode + CvdeclNode = CvarNode + + module IfNode + def condition + self[1] + end end end class TreeDresser - # SMELL: Duplication - # Put these into a new module and build the mapping automagically - # based on the node type - EXTENSIONS = { - :cvar => CvarNode, - :cvasgn => CvarNode, - :cvdecl => CvarNode, - :if => IfNode, - :case => CaseNode, - } def dress(sexp) sexp.extend(SexpNode) - if EXTENSIONS.has_key?(sexp[0]) - sexp.extend(EXTENSIONS[sexp[0]]) + module_name = extensions_for(sexp.sexp_type) + if Reek::SexpExtensions.const_defined?(module_name) + sexp.extend(Reek::SexpExtensions.const_get(module_name)) end sexp[0..-1].each { |sub| dress(sub) if Array === sub } sexp end + + def extensions_for(node_type) + "#{node_type.to_s.capitalize}Node" + end end end lib/reek/tree_dresser.rb @@ -101,20 +101,20 @@ describe CodeContext do @ctx = CodeContext.new(nil, ast) end it 'yields no calls' do - @ctx.each(:call, []) {|exp| raise "#{exp} yielded by empty module!"} + @ctx.each_node(:call, []) {|exp| raise "#{exp} yielded by empty module!"} end it 'yields one module' do mods = 0 - @ctx.each(:module, []) {|exp| mods += 1} + @ctx.each_node(:module, []) {|exp| mods += 1} mods.should == 1 end it "yields the module's full AST" do - @ctx.each(:module, []) {|exp| exp[1].should == @module_name.to_sym} + @ctx.each_node(:module, []) {|exp| exp[1].should == @module_name.to_sym} end context 'with no block' do it 'returns an empty array of ifs' do - @ctx.each(:if, []).should be_empty + @ctx.each_node(:if, []).should be_empty end end end @@ -128,24 +128,24 @@ describe CodeContext do @ctx = CodeContext.new(nil, ast) end it 'yields no ifs' do - @ctx.each(:if, []) {|exp| raise "#{exp} yielded by empty module!"} + @ctx.each_node(:if, []) {|exp| raise "#{exp} yielded by empty module!"} end it 'yields one module' do - @ctx.each(:module, []).length.should == 1 + @ctx.each_node(:module, []).length.should == 1 end it "yields the module's full AST" do - @ctx.each(:module, []) {|exp| exp[1].should == @module_name.to_sym} + @ctx.each_node(:module, []) {|exp| exp[1].should == @module_name.to_sym} end it 'yields one method' do - @ctx.each(:defn, []).length.should == 1 + @ctx.each_node(:defn, []).length.should == 1 end it "yields the method's full AST" do - @ctx.each(:defn, []) {|exp| exp[1].should == @method_name.to_sym} + @ctx.each_node(:defn, []) {|exp| exp[1].should == @method_name.to_sym} end context 'pruning the traversal' do it 'ignores the call inside the method' do - @ctx.each(:call, [:defn]).should be_empty + @ctx.each_node(:call, [:defn]).should be_empty end end end @@ -169,7 +169,7 @@ EOS ast = src.to_reek_source.syntax_tree ctx = CodeContext.new(nil, ast) - ctx.each(:if, []).length.should == 3 + ctx.each_node(:if, []).length.should == 3 end end end spec/reek/code_context_spec.rb @@ -37,91 +37,3 @@ describe CodeParser do 'def options() ozz.on { |@list| @prompt = !@list } end'.should_not reek end end - -describe CodeParser do - context 'with no attributes' do - it 'records nothing in the class' do - klass = ClassContext.from_s('class Fred; end') - klass.attributes.should be_empty - end - it 'records nothing in the module' do - ctx = ModuleContext.from_s('module Fred; end') - ctx.attributes.should be_empty - end - end - - context 'with one attribute' do - shared_examples_for 'one attribute found' do - it 'records the attribute' do - @ctx.attributes.should include(Name.new(:property)) - end - it 'records only that attribute' do - @ctx.attributes.length.should == 1 - end - end - - context 'declared in a class' do - before :each do - @ctx = ClassContext.from_s('class Fred; attr :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'reader in a class' do - before :each do - @ctx = ClassContext.from_s('class Fred; attr_reader :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'writer in a class' do - before :each do - @ctx = ClassContext.from_s('class Fred; attr_writer :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'accessor in a class' do - before :each do - @ctx = ClassContext.from_s('class Fred; attr_accessor :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'declared in a module' do - before :each do - @ctx = ModuleContext.from_s('module Fred; attr :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'reader in a module' do - before :each do - @ctx = ModuleContext.from_s('module Fred; attr_reader :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'writer in a module' do - before :each do - @ctx = ModuleContext.from_s('module Fred; attr_writer :property; end') - end - - it_should_behave_like 'one attribute found' - end - - context 'accessor in a module' do - before :each do - @ctx = ModuleContext.from_s('module Fred; attr_accessor :property; end') - end - - it_should_behave_like 'one attribute found' - end - end -end spec/reek/code_parser_spec.rb @@ -3,24 +3,96 @@ require File.dirname(__FILE__) + '/../../spec_helper.rb' require 'reek/smells/attribute' require 'reek/class_context' -require 'spec/reek/smells/behaves_like_variable_detector' - include Reek include Reek::Smells describe Attribute do before :each do @detector = Attribute.new - @record_variable = :record_attribute end + context 'with no attributes' do + it 'records nothing in the class' do + ctx = ClassContext.from_s('class Fred; end') + @detector.attributes_in(ctx).should be_empty + end + it 'records nothing in the module' do + ctx = ModuleContext.from_s('module Fred; end') + @detector.attributes_in(ctx).should be_empty + end + end + + context 'with one attribute' do + shared_examples_for 'one attribute found' do + it 'records the attribute' do + @detector.attributes_in(@ctx).should include(:property) + end + it 'records only that attribute' do + @detector.attributes_in(@ctx).length.should == 1 + end + end + + context 'declared in a class' do + before :each do + @ctx = ClassContext.from_s('class Fred; attr :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'reader in a class' do + before :each do + @ctx = ClassContext.from_s('class Fred; attr_reader :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'writer in a class' do + before :each do + @ctx = ClassContext.from_s('class Fred; attr_writer :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'accessor in a class' do + before :each do + @ctx = ClassContext.from_s('class Fred; attr_accessor :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'declared in a module' do + before :each do + @ctx = ModuleContext.from_s('module Fred; attr :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'reader in a module' do + before :each do + @ctx = ModuleContext.from_s('module Fred; attr_reader :property; end') + end + + it_should_behave_like 'one attribute found' + end + + context 'writer in a module' do + before :each do + @ctx = ModuleContext.from_s('module Fred; attr_writer :property; end') + end + + it_should_behave_like 'one attribute found' + end - [ClassContext, ModuleContext].each do |klass| - context "in a #{klass}" do + context 'accessor in a module' do before :each do - @ctx = klass.create(StopContext.new, s(:null, :Fred)) + @ctx = ModuleContext.from_s('module Fred; attr_accessor :property; end') end - it_should_behave_like 'a variable detector' + it_should_behave_like 'one attribute found' end end end spec/reek/smells/attribute_spec.rb adc267e124c5178250f41208b782317529cda3cf Kevin Rutherford kevin@rutherford-software.com http://github.com/kevinrutherford/reek/commit/2f520b0535e5b2b3f21bf608a30d993703b6be95 2f520b0535e5b2b3f21bf608a30d993703b6be95 2009-11-03T07:30:15-08:00 2009-11-03T07:30:15-08:00 Removed another attribute from ModuleContext 75c2194a37df7db535a6bcdc4ba3f12721054888 Kevin Rutherford kevin@rutherford-software.com