What is a" feature envy" code and why is it considered a code smell?

Question

This question on SO talks about correcting what the OP thought is feature envy code. Another example where I saw this nifty phrase being quoted is in a recently given answer here in programmers.SE. Although I did drop in a comment to that answer asking for the information I thought it would of general help to programmers following Q&A to understand what is meant by the term feature-envy. Please feel free to edit additional tags if you think appropriate.

possible duplicate of [How large is ok for a Class?](http://programmers.stackexchange.com/questions/11846/how-large-is-ok-for-a-class) See also: [Is it a code smell if an object knows a lot of its owner?](http://programmers.stackexchange.com/questions/100537/is-it-a-code-smell-if-an-object-knows-a-lot-of-its-owner) — gnat, Sep 22 '13 at 08:56

score 114 · Accepted Answer · edited Sep 21 '13 at 07:00

114

Feature envy is a term used to describe a situation in which one object gets at the fields of another object in order to perform some sort of computation or make a decision, rather than asking the object to do the computation itself.

As a trivial example, consider a class representing a rectangle. The user of the rectangle may need to know its area. The programmer could expose width and height fields and then do the computation outside of the Rectangle class. Alternatively, Rectangle could keep the width and height fields private and provide a getArea method. This is arguably a better approach.

The problem with the first situation, and the reason it is considered a code smell, is because it breaks encapsulation.

As a rule of thumb, whenever you find yourself making extensive use of fields from another class to perform any sort of logic or computation, consider moving that logic to a method on the class itself.

edited Sep 21 '13 at 07:00

Doc Brown

199,015
33
367
565

answered Sep 21 '13 at 04:35

jhewlett

2,224
1
17
15

9

+1, though your example is not realistic, since a useful Rectangle class would typically expose width and height fields either. – Doc Brown Sep 21 '13 at 06:57
4

And though the breaking of encapsulation *may* a occur together with "Feature envirism", in most real-world examples I have seen so far it was probably not the case. It happens more in situations "hey, I need to compute some things just in the code using that object, and I am not sure if I can touch that object's implementation / if we should give the object the responsibility for that computation". And then one implements the calculation using fields **which are already exposed** (though a much cleaner implementation would be probably possible within the object). – Doc Brown Sep 21 '13 at 07:09
2

@DocBrown Imagine a rectangle drawn on the surface of a torus, cone or sphere. If my shape-drawing library produces objects which are capable of producing the correct results in such contexts, you'd be foolish not to leave them to calculate their own areas - in any context. – itsbruce Sep 21 '13 at 09:24
Let's say you had some DTO's that represented a rectangle, .. they get serialized to json and get used on the client. Calculating the area in that case will happen on the client side? Or would you advise to have an "area" property that gets calculated on the server side? (with a CalculateArea() function inside the DTO? or outside? In a lot of modern situations the whole thing isn't quite as clear. Where to put what, depends a LOT on the situation I guess :) – Mvision Sep 22 '13 at 12:09
@DocBrown I'm curious, does the concept of feature envy as a code smell contradict the arguments given here? http://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197 Some excerpts: "Adding functions beyond the minimum necessary to let clients get their jobs done decreases the class's comprehensibility and maintainability." "The addition of member functions beyond those truly required violates the open/closed principle, yields fat class interfaces, and ultimately leads to software rot." – Oskar N. Sep 23 '13 at 22:52
1

@OskarN.: By definition we're talking about functions that *are* needed. They're obviously needed if other classes are re-implementing them over and over again. – Aaronaught Sep 24 '13 at 00:11
2

@OskarN.: it depends; sometimes the decision is clear, sometimes its a matter of taste, and most often its a matter of experience. In your article, there are good reasons why Scott Meyers writes "**sometimes** less is more", and that it took him years to understand when *not* to apply his "algorithm for deciding when to make f a member function". – Doc Brown Sep 24 '13 at 05:40
@Aaronaught: re-implementing can be also avoided by placing a reusable function in a common place *outside* the class in stake, so that's neither a pro- or counter argument. – Doc Brown Sep 24 '13 at 05:43
1

@DocBrown: Not really, that's just a slightly more subtle case of feature envy that's almost equally likely to lead to spaghetti code. Unless you *can't* modify the class, you should follow the [Tell, Don't Ask](http://martinfowler.com/bliki/TellDontAsk.html) rule. – Aaronaught Sep 25 '13 at 00:27
@itsbruce: If an object like a rectangle includes an "area" property, its contract should specify what that property means and what invariants clients are entitled to rely upon. Even setting aside topological issues, if e.g. the area, height, and width are reported using type `float` but might be internally stored as `double`, there may be value in specifying that the reported area will always be the *reported* height times the reported width, or specifying that the area may be computed using higher-precision values. What's important is that people implementing the class and those using it... – supercat Jan 26 '15 at 18:07
...have compatible expectations. Guaranteeing that the indicated equality holds might be helpful for consumers, but would constrain implementation; expressly stating that such equality may not hold may interfere with some things consumers could otherwise do with the class, but would reserve some freedom for future implementations. Unfortunately, many class designers neither grant freedom to consumers nor reserve it to implementers, yielding a worst-of-both-worlds scenario. – supercat Jan 26 '15 at 18:10

score 2 · Answer 2 · answered Sep 23 '13 at 21:58

There is a possible situation when it is OK to use another class/struct methods extensively - when your class/struct is a container for data. Usually there is a little you can do with this data without external context.

Such classes can still hold some internal logic but more often they are used as containers:

class YourUid {
 public:
  YourUid(int id_in_workplace_, int id_in_living_place_, DB* FBI_database, int id_in_FBI_database);
  bool IsInvalidWorker() const { return id_in_workplace == consts::invalid_id_in_workplace; }
  bool CanMessWith() const { return !FBI_database_.is_cool(id_in_FBI_database_); }
  int id_in_workplace;
  int id_in_living_place;
 private:
  int id_in_FBI_database_;
  const DB* FBI_database_;
};

@jhewlett in his answer refers to this article to prove that you should no use other class members extensively, but there is another code smells situation described there with advocates my example:

Long Parameter List. Limit the number of parameters you need in a given method, or use an object to combine the parameters.

@gnat The Q is about Why it is considered "code smell". jhewlett gives an A with some too general assumtions questioned in the comments. My answer is 2 cents to distinguish "code smell" from normal practice. — Riga, Sep 23 '13 at 22:42

What is a" feature envy" code and why is it considered a code smell?

2 Answers2

Linked