reek and feature envy

Feature Envy is the smell (anti-pattern) in which one code fragment makes more use of another object than it does of itself. reek currently (version 0.2.3) includes a very naive check for Feature Envy: For each method inspected, reek counts the messages sent to each recipient and also counts the references to self; if self has the highest count, reek doesn’t report feature envy.

Here’s a contrived example:

class Parcel
  def addressee
    "#{@person.first_name} #{@person.last_name}"
  end
end

When it sees this code, reek says:


[Feature Envy] Parcel#addressee uses @person more than self

That’s because the method sends two messages to @person, so this code fragment should be moved onto that object:

class Parcel
  def addressee
    @person.full_name
  end
end

class Person
  def full_name
    "#{first_name} #{last_name}"
  end
end

Now fewer messages are sent overall, so the Feature Envy is gone.

The current algorithm is quite naive, and reek will certainly report false negatives sometimes. I hope to be able to refine the algorithms and gradually make reek smarter; for example, I’ve considered adding a configuration option so you can turn off certain smells for certain methods, but somehow that feels like the start of a slippery slope. This winter I hope to find the time to work on the algorithms a bit more, so if you do find reek reporting a false negative (for any smell), please send me a code sample — or preferably a new test.

About these ads

2 thoughts on “reek and feature envy

  1. Hi,

    We found that with version 0.3.0, we’re getting warnings even when using calls to local methods.

    In your example, we’d get a warning from Person, because it’s calling first_name and last_name, and reek doesn’t seem to realise they’re locally-defined methods.

    Could this be because our Person is an ActiveRecord object, and reek can’t know about the meta-magic that will happen at runtime (and hence maybe things these are global methods or something?)

    Is it perhaps better practice to use self.first_name anyway when dealing with ActiveRecord, in order to allow you to override the default getters / setters if you need to?

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s