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…