I recently saw a terrific talk by Sandi Metz at the Abstractions conference where she encouraged the audience to learn to identify the concrete code smells from Martin Fowler's Refactoring. Two of the biggest takeaways I heard in the talk were that we as developers should be able to identify code smells by name, and also to identify and complete the prescribed refactorings to clean them up.

Sandi also made a terrific, subtle, and often-missed point that I'd like to expand on here: code smells are pieces of code that we might want to refactor. For example, if a given smelly piece of code rarely needs to be changed or even read by the development team, there's little reason to burn time on refactoring that code. And it even goes a bit further: some code smells, like refactorings, are polar opposites of one another, such that the obvious refactoring of one smell can cause the other smell to surface.

With refactoring, it's clear from the names of pairs like Extract Class and Inline Class, or Extract Method and Inline Method, that they are inverses of each other. Less clear, perhaps, are the tensions between smells.

Feature Envy

Feature Envy describes a situation where a given class uses mostly data and methods from some other class, instead of its own data and methods. In the following example, we could say that USContactFormatter envies the features of User:

class User
		attr_accessor :address_line_1, :address_line_2, :city, :state, :zipcode
end

class USContactFormatter
		def initialize(user)
				@user = user
		end

		def mailing_address
				[
				@user.address_line_1,
				@user.address_line_2,
				"#{@user.city}, #{@user.state} #{@user.zipcode}"
				].compact.join("\n")
		end

		def phone_number
				@user.phone.gsub(/\D/, "")
		end
end

The problem, in principle, is that it can get unwieldy to always be sending messages to another object to do work, rather than just delegating the behavior to that object. Encapsulation and "Tell Don't Ask" are both relevant principles here.

The typical refactoring for feature envy moves methods over to the class where they "belong" (the class that's the focus of the envy). In our case above, that might look like this:

class User
		attr_accessor :address_line_1, :address_line_2, :city, :state, :zipcode

		def mailing_address
				[
				address_line_1,
				address_line_2,
				"#{city}, #{state} #{zipcode}"
				].compact.join("\n")
		end

		def phone_number
				phone.gsub(/\D/, "")
		end
end

And now nobody can accuse us of feature envy: we've got our behavior (mailing_address and phone_number) in the same class that's got the data.

But I introduced this post with the idea that there would be tension, so how could the clear improvement above possibly cause issues?

Large Class

The trouble happens when a piece of data or behavior is important to a system. Addressing feature envy in the obvious way (as above) accrues methods on the class where that data lives. The more times you fix the feature envy, the bigger that class grows.

To start, things might just look like this:

But as systems grow and important domain concepts congeal, it's common to end up with something like this:

And this is how projects end up with 2000-line User classes. Large Classes can be difficult to manage for a whole slew of reasons that have been well documented.

What To Do?

We could sort of cheat by denormalizing the User state across multiple objects. This approach could work fine, especially if we're dealing in immutability as I always prefer. Meanwhile we've introduced complexity into the system, in the form of a Middle Man smell.

Similar tradeoffs exist between the Message Chain and Large Class smells. And while I haven't enumerated more, I feel pretty confident that the two that occurred to me off the top of my head aren't the only two.

I suspect we simply cannot write the perfect solution, even for this small problem space (not to mention entire systems). Does this mean that code smells are without merit? That we should just plug our noses and ignore them? That's certainly the easy way: just flip a coin and get on with it. But we wouldn't be doing our job if we decided to punt on hard decisions.

When we decide to address a code smell, thinking about the tradeoffs can help us make better decisions. By understanding what problems we're trying to solve, and what problems we'll accept as a consequence of our decision, we'll be better equipped to solve the problems that code smells warn us about.

And as systems evolve and grow over time, the tradeoffs behind our initial decisions change, which means we probably ought to revisit our decisions. As the saying goes:

When the facts change, I change my mind. What do you do, sir?

• John Maynard Keynes