Allow matching on number of smell instances

agree with the second version:

some_source.should_not reek_much(:maximum_smells => 4400)
# or an alternate syntax:
some_source.should_not reek.more_than(4400).smells
some_source.should reek.less_than(4400).smells
# '.smells' is just unnecessary sugar to make it read nicer.

I think I'd use that to keep the overall smells from going up. But since that would still leave me with the "waterbed" or "balloon" problem (push out the smells on one area and the smells might show up somewhere else), I'm also going to quarantine certain files/directories as "smell free zones". It's very annoying to greenfield an new area, feel quite pleased with how clean it is, and then come back a few months later to a great big mess. What happened?!

How about starting with

Dir[...].should_not reek_of(:UncommunicativeName)

for some smallish Dir down the tree?

That's also a good ratchet-like approach. Unfortunately, our codebase seems to collect all of the smells. ;-) I think I'll use that approach for some of them... but:

 $ reek app/**/*.rb lib/**/*.rb | tee TODO.reek | perl -pe 's/^(\[[^\]]*\])?.*/\1/' | sort | uniq -c
(string):335: warning: useless use of == in void context
(string):370: warning: useless use of a variable in void context
(string):43: warning: ambiguous first argument; put parentheses or even spaces
(string):253: warning: unused literal ignored
(string):33: warning: (...) interpreted as grouped expression
(string):140: warning: useless use of a variable in void context
(string):14: warning: `*' interpreted as argument prefix
(string):41: warning: `*' interpreted as argument prefix
(string):42: warning: (...) interpreted as grouped expression
    544
    142 [Control Couple]
   1843 [Duplication]
    590 [Feature Envy]
      1 [Large Class]
    649 [Long Method]
     89 [Long Parameter List]
      1 [Long Yield List]
    191 [Nested Iterators]
     79 [Uncommunicative Name]
    313 [Utility Function]

(are those $stderr warnings coming from reek itself? I think they are.)

So to handle this code, I'd put an hour or two into cleanup, and then I could use something like:

files = Dir['app/**/*.rb', 'lib/**/*.rb']
files.should_not reek_of(:UncommunicativeName, :LongYieldList, :LargeClass)
files.should_not reek_of(:UtilityFunction).more_than(313).smells
files.should_not reek_of(:NestedIterators).more_than(191).smells

And so on. Basically, the more places I can use to ratchet up the code quality, draw a line and say "this far, no farther!", the better. :-)

And yes, like you said... I'll probably start with smaller directories down the tree, rather than all of app and lib (as in my example).

The $stderr warnings are from the underlying Ruby parser. (So they're smells too :).

I'd say that UncommunicativeName and Duplication are the easiest to remove -- not least because Reek's Duplication check right now is very simple. My guess is you could eliminate UncommunicativeNames everywhere for not much effort? And then work through a Dir at a time cleaning up the Duplication reports. I'd expect to see some of the other smells go away as a result of doing just that...

I'm quite reluctant to add numerical theshholds for smells. But here's an alternative thought:

require 'my_class'

describe MyClass do
  it 'reports no code smells' do
    MyClass.should_not reek
  end
end

Would that help you clean up incrementally?

Absolutely. I like that a lot. I'm envisioning something like the following:

# specific smells we've cleaned up *everywhere*
describe "our codebase" do
  [
    :UncommunicativeName,
    :Duplication,
    :LargeClass,
    :LongYieldList,
  ].each do |smell|
    it "should not have any #{smell} smells" do
      Dir[appropriate_file_glob].should_not reek_of(smell)
    end
  end
end

# specific classes or directories that we've cleaned up completely
[
  NewClass,
  GoodClass,
  CleanClass,
  'app/models/my_clean_zone/**/*.rb'
].each do |klass_or_location|
  describe klass_or_location
    it "should be smell-free" do
      case klass_or_location
      when String then Dir[klass_or_location].should_not reek
      else                              klass.should_not reek
      end
    end
  end

# Incompletely cleaned up classes (or directories) can be handled individually
# and eventually moved into the category above (no smells)
describe FooBarKindaDirty do
  it "should be free of some smells" do
    FooBarKindaDirty.should_not reek_of(:LongMethod, :UtilityFunction)
  end
end

What do you think?

Cool -- I'm on it this weekend. One smell tweak, while we're sketching code:

# specific classes or directories that we've cleaned up completely
[
  NewClass,
  GoodClass,
  CleanClass,
  Dir['app/models/my_clean_zone/**/*.rb']
].each do |klass_or_location|
  describe klass_or_location
    it "should be smell-free" do
      klass_or_location.should_not reek
    end
  end

please ignore the several typos in there (mismatched 'do...end', klass/klass_or_location). That's the downside of not having a 'preview' button. ;-)

I can see why you might be reluctant to add numerical limits. It's better to just wipe out particular smells entirely within a single context.

I considered that, but Dir['nice/*/type/.glob'].to_s is quite atrocious. A good specdoc is important to me. ;-)

so, if you made the 'reek' and 'reek_of' matchers automatically typeglob strings via Dir[], then we could say:

# specific classes or directories that we've cleaned up completely
[
  NewClass,
  GoodClass,
  CleanClass,
  'app/models/my_clean_zone/**/*.rb'
].each do |klass_or_location|
  describe klass_or_location
    it "should be smell-free" do
      klass_or_location.should_not reek
    end
  end
end

and the specdoc would still look nice, and it would work as expected.

There is already another way to do what you want:

dir = Dir['lib/**/*.rb'].to_source
dir.report.should have_at_most(85).smells

Not quite so sugary, perhaps, but it does the job.

Works for me. :) Thanks!