DataMapper models trigger false positive Large Class

[ashmoran]

I get the following code smell with a DataMapper model:

"app/models/response.rb" -- 1 warnings:
Response has at least 27 methods (Large Class)

I'm sure you'll agree the class doesn't have that many methods...

class Response
  include DataMapper::Resource

  property :id,           Serial
  property :response,     Integer,  :nullable => false
  property :made_at,      Time,     :nullable => false, :default => lambda { Clock.current_time }

  belongs_to  :proposed_question

  def correct?
    question.accepts_as_correct?(response)
  end

  def <=>(other)
    other.made_at <=> made_at
  end

  def ==(other)
    made_at == other.made_at &&
      response == other.response
  end
end

Similarly wacky numbers come out of all my other DM models, 37 being the highest...

[kevinrutherford]

Ash, Thanks for helping me analyse this on Friday! Our conclusion was that there are in fact two problems here:

First, LargeClass goes looking for the class in the object space, and if it finds it then it uses the in-memory class's method count. That shouldn't happen; the test should be based on the supplied chunk of source code. (Could you check that this is indeed occurring in your case? Please run the failing test on its own; if it passes, we know that reek is finding the class in memory from one of your other tests.)

And second, LargeClass should be disabled for in-memory reek parsing. Raised separately as #28.

[ashmoran]

Well I only just got my head round it at the end :)

I will run the spec isolated in just a sec. But I remember we came to the conclusion that in general, the in-memory analysis will differ from the source-based analysis. eg my ugly bit of code:

{ :name => "Name", :identifier => "Identifier" }.each do |property, property_label|
  define_method("dm_validate_#{property}_unique_in_study") do
    property_unique_in_study?(property) || [false, "#{property_label} is already taken"]
  end    
end

MyClass.should_not reek will see two methods here, but a source analysis will see none. Using eval to generate a thousand-line method will be similarly different. And as you say, you may not even have the source that makes these changes. (Maybe this should all go under #28)

So how would you write the documentation to help reek users decide when to do an analysis source-based and when to do it object-based?

[ashmoran]

Behaviour confirmed: in a Merb app, the following line in spec_helper.rb causes the models to load, which causes reek to look in ObjectSpace:

Merb.start_environment(:testing => true, :adapter => 'runner', :environment => ENV['MERB_ENV'] || 'test')

I have the following line at the top of my reek_spec.rb:

require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

I could replace this with:

require 'reek'
require 'reek/spec'

But it will only take effect when that file is run in isolation. I guess I should wait for the bugfix?

[kevinrutherford]

I believe this is fixed in e235ca9 (soon to be released as 1.1.3).

[ashmoran]

Nudge me when 1.1.3 is out and I'll switch to the github gems to test it.

[mrship]

Ashley pointed this release out to me, so I tested it in my environment it is indeed fixed. Many thanks.

[kevinrutherford]

Good news -- thanks for the test!