Kevinrutherford reek

Comments and changes to this ticket

• You flagged this item as spam.
  

  nicholas a. evans April 1st, 2009 @ 01:19 PM

  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?!

• 

  Kevin Rutherford April 1st, 2009 @ 01:26 PM

  How about starting with

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

  for some smallish Dir down the tree?

• You flagged this item as spam.
  

  nicholas a. evans April 1st, 2009 @ 01:59 PM

  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. :-)

• You flagged this item as spam.
  

  nicholas a. evans April 1st, 2009 @ 02:01 PM

  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).

• 

  Kevin Rutherford April 1st, 2009 @ 02:07 PM

  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...

• 

  Kevin Rutherford April 2nd, 2009 @ 02:04 PM

  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?

• You flagged this item as spam.
  

  nicholas a. evans April 2nd, 2009 @ 02:23 PM

  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?

• 

  Kevin Rutherford April 2nd, 2009 @ 02:27 PM

  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
  

• You flagged this item as spam.
  

  nicholas a. evans April 2nd, 2009 @ 02:30 PM

  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.

• You flagged this item as spam.
  

  nicholas a. evans April 2nd, 2009 @ 02:37 PM

  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.

• 

  Kevin Rutherford April 3rd, 2009 @ 07:53 AM

  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.

• You flagged this item as spam.
  

  nicholas a. evans April 3rd, 2009 @ 12:48 PM

  Works for me. :) Thanks!

• 

  Kevin Rutherford April 7th, 2009 @ 04:33 PM

  • State changed from “new” to “open”
• 

  Kevin Rutherford April 9th, 2009 @ 09:05 PM

  • State changed from “open” to “resolved”

  (from [0cbce97780af2001da68829fb8685dcc3076463a]) Rspec matchers now work for any object [#33 state:resolved] http://github.com/kevinrutherfor...