Feature envy in component design

Have you ever heard of the "feature envy" code smell?

One concise definition is "a method [that] accesses the data of another object more than its own data." In Refactoring, Martin Fowler says you can spot this smell when "a method seems more interested in a class other than the one it is in. The most common focus of the envy is the data."

Consider the following function:

function greeting(user) {
  let fullName = user.title ? `${user.title} ` : '';
  fullName += user.name;

  return `Welcome, ${fullName}`;
}

Here, greeting is arguably too envious of the user object's data. One could imagine all the ways a user's full name could change: maybe suffixes like PhD are added down the road. Changes like this would force us to update greeting, which indicates it might have too much knowledge about user.

A solution to this is to create something like a fullName method on User that contains all this business logic:

class User {

  fullName() {
    let fullName = this.title ? `${this.title} ` : '';
    fullName += this.name;

    return fullName;
  }

}

The greeting function can then be simplified

function greeting(user) {
  return `Welcome, ${user.fullName}`;
}

and it's now much less "envious" of user's data. Our new User class is an improvement because it better encapsulates its data and behavior.

So, how can we keep an eye out for envious objects? The root cause of feature envy is misplaced responsibility. If an object, component or method seems like it's accessing too much data from another object, it's good to step back and ask whether that knowledge should reside elsewhere.

One way feature envy creeps up in Ember development is when domain-specific components are written as dumb components. Consider the following example:

{{! some-route/template.hbs }}

{{blog-post-form
  title=blogPost.title
  content=blogPost.content}}

Here, a form component is being rendered which takes in title and content properties. When asked why a <blog-post-form> component would be designed with this interface, rather than accepting a single blogPost property, I often hear a line of reasoning that goes something like this: Making <blog-post-form> take in a blogPost model bakes too many assumptions into the form component. If we make it "dumb", simply taking in title and content as plain strings, <blog-post-form> can now be reused elsewhere with other data. This makes the component more flexible and composable with the rest of our application.

My reaction to this is to ask, is this component actually going to be reused? Dumb components like color pickers are designed to be reused, which is why they make so few assumptions about their use; but <blog-post-form> is being written as a domain-specific component - a component that's designed to do something specific with data that belongs to the application's primary domain. If the component is being designed for that purpose and is not intended to be reused, this is a clear case of feature envy.

One way to see this is to recognize that fundamentally, component invocations are function calls. Our {{blog-post-form}} Handlebars code can be thought of as a render function:

renderBlogPostForm() {

}

If you were writing this function in regular JavaScript code, what would you expect its arguments to be?

A single blogPost seems most appropriate:

renderBlogPostForm(blogPost) {

}

Let's compare this interface to our current template, where we pass in title and content properties to our render function:

renderBlogPostForm(title, content) {

}

Take a look at the two invocations:

renderBlogPostForm(blogPost);
renderBlogPostForm(blogPost.title, blogPost.content);

With the latter function signature, every invoker is now responsible for reaching into blogPost and accessing its data.

The question we should ask is, is it appropriate for our various callers to be responsible for this knowledge? Or should our renderBlogPostForm function be responsible? After all, our function has blogPost in its name. This is highly suggestive that our render function itself should be responsible for how to access data from a blogPost model, rather than whatever object happens to be invoking the function.

A second issue is that the renderBlogPostForm(title, content) signature suggests to others that they can pass any data they want to for these parameters, as long as they're strings. But we've only written our <blog-post-form> component to handle blogPost data; it would actually be an abuse of our component if callers passed in data that didn't come from a blogPost.

For these reasons, let's update our component's interface so that callers don't have to be envious of blogPost's data:

{{! some-route/template.hbs }}

{{blog-post-form blogPost=blogPost}}

This simple interface communicates clearly to callers that this component is domain-specific, and intended to work with a single blogPost model.

In closing, there are definitely situations where "dumbing down" components enables reuse in unexpected and beneficial ways. But writing your domain-specific components as dumb components introduces tons of feature envy into your application, and makes your components a burden to change.

Keep an eye out for envious objects, and if knowledge seems to be spilling over into the wrong place, step back and ask who should be responsible for it.

Watch our series on Component Side Effects for a deeper dive on this subject.