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…

2 thoughts on “thoughts on feature envy

  1. If the adaptee is not the caller, but the class being adapted, whose helper method is being called, then it can use its own setters and getters, can’t it? So that’s a force pushing the method towards the adaptee.

    A helper method is part of defining the interface, without having to reify an adapter object or facade pattern class. If the caller uses the helper to tell the adaptee to do something, rather than ask it about something, then it seems legitimate to me. For example, if a client wraps Array#unshift and Array#push in add_to_queue(x), x = remove_from_queue, that seems OK to me, because you don’t want to change what Array looks like. But the need for an interface like that suggests the Array should be replaced by a Queue class which has_an Array. But more generally, I’d be inclined to make the helper a method of the adaptee (which is being told); just because only one class uses the method now, doesn’t mean when we achieve code reuse (cough) that other clients won’t need the helper method in the same way as the first client.

    I suppose in Java, in which I’m not fluent, the helper would literally be part of an interface the adaptee implements.

    Maybe Reek needs to parse comments with directives to
    tell it this kind of semantic detail, which can’t be inferred correctly from the code?

  2. One problem, I find, with leaving the helper with the only user is that it may not be found when a second user comes along. A point you touch on. That’s enough risk for me to decide where it should go. However, I often find these methods with the only user and wonder why I put it there. I could be lofty and claim different states of mind, but that might well be the case!

Leave a Reply

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

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

Google photo

You are commenting using your Google 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 )

Connecting to %s