thoughts on feature envy

I’m currently debating with myself on how and where to take Reek next. Specifically, I’m thinking about the FeatureEnvy / UtilityFunction class of code smell and their relation with classes and class methods. I’ve begun unravelling some thoughts over on the ruby-reek mailing list, and I thought I’d seek your opinion too. Much of what follows may turn out to be complete tripe…

There are always two views of any code, imo: specification-time and run-time. Code smells are, I think, (very nearly) 100% in the specification-time space; they deal with the code as written: does this name communicate well, is this code fragment coherent with those around it, will these two fragments change at different times, and so on; aspects of the specification that don’t affect behaviour. Classes are the means by which we describe the behaviour of runtime objects. It’s nice and sexy that in Ruby we also have runtime objects representing those classes, but I don’t necessarily see them as being the same beasts. One is a specification, the other is a runtime mechanism.

So one view of FeatureEnvy and UtilityFunction is that they help to improve the cohesiveness of the specification, by encouraging us to move code fragments closer to the state/data on which they operate. And this is where it begins to get hazy for me…

Consider a “helper” method that is only called by the methods of a single class, but which makes no use of the state of that class’s instances; where should its specification live? Shoulld it live with the class that uses it, or should it move towards the data on which it operates? Usually such methods exist in order to help simplify the caller’s interaction with something — I’ll posit that such methods are usually Adapters. So which is worse — have the adapter live with the adaptee or with the only client? Should it only move to (or towards) the adaptee when a second client needs it, perhaps?

One might argue that moving it to the adaptee creates a case of SpeculativeGeneralisation; in which case it should remain with the only client until it is duplicated by some other class. On the other hand, moving the fragment towards the adaptee can help to encapsulate the adaptee better, often allowing it to expose less to the outside world. All of which seems to imply that we can’t determine the best response to seeing Feature Envy without looking at other smells, current and future.

One approach would be to not treat FeatureEnvy / UtilityFunction as smells, but instead to look at the adaptee and call foul if we see a getter or setter; and in the absence of getters and setters, wait for duplication to show us what code needs to be shared, and hence moved. I think I may try living with that approach for a month and see what happens…

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.