The problem with code smells

Like most developers I know, I have used code smells to describe problems in code since I first heard about them. The idea was introduced by Kent Beck in Fowler’s Refactoring back in 1999, and has taken root since then. The concept of code smells has several benefits, not least the fact that it gives names to ideas that were previously only vague. Having a list of named code quality anti-patterns helps all of us discuss them on the same terms.

But while I was writing Refactoring in Ruby with @wwake, and writing Reek at the same time, I began to feel a little uneasy about them. I was never able to put my finger on exactly why that was, or what I was uneasy about, but the feeling never went away. This year I think I have finally understood what I think about code smells, and why I think we can do somewhat better. So before reading on, take a moment to list the things you don’t like about them. Then let’s compare lists. Go ahead, I’ll wait.

Ok, done that? Here are my current reasons for wanting a different tool for describing code quality:

  • The names aren’t all that helpful for people unfamiliar with the concepts. If you had never heard of them, what would you make of Feature Envy, Shotgun Surgery, Data Clump etc? Sure, the names are memorable, but that only helps with hindsight, for people who have taken the time and trouble to investigate and learn them.
  • Some code smells can overlap. For example, I’m often unsure whether I have seen Feature Envy or Inappropriate Intimacy, Divergent Change or Large Class. There’s a sense in which this doesn’t matter, of course; but it undermines their use as a communication tool.
  • Some of the smells can be subjective or contextual, leaving the quality of the code open to interpretation. For example, how large is a Large Class or a Long Parameter List?
  • Some of the smells apply only under certain circumstances. For example, a Switch Statement is perfectly fine at the system boundary when we are figuring out the type of an incoming message, but often bad news when it represents a type check on code we own. And it can be acceptable at the system boundary to grab the fields of a Data Class in order to display them, while elsewhere that might be seen as Feature Envy.
  • The list of code smells is not canonical; different people have added their own smells to Beck & Fowler’s original list. This situation is even worse when it comes to smells in unit tests; try looking for a canonical list of test smells and you’ll find no consensus whatever. In my opinion, this fact alone completely undermines the idea that code smells form a pattern language for describing code quality.
  • There are no clear and obvious code smells covering some dynamic problems, such as the coupling between variables whose values depend on each other, or the problems introduced by mutable objects.

Did you have the same list, or something similar?

So, what can we do about it? I think the answer lies with Connascence. This is an idea introduced by Meilir Page-Jones in two books in the 1990s, and later popularised by @jimweirich in a series of conference talks. I’m not going to cover Connascence in detail here — you can find it all for yourself by reading @jimweirich‘s articles or looking at my summary slides. I just wanted to take a moment to write down my current opinions about code smells. I’ll probably write in more detail about Connascence in the coming weeks, but for now what do you think?

Advertisements

12 thoughts on “The problem with code smells

  1. Code smells are a great metaphor to describe potential design problems but in my mind they have exactly the same issues as design patterns. Sometimes in code reviews these labels can be used without thought criticise the code. If we blindly use these labels, “oh that’s feature envy” without exploring the code further we may be making mistakes to code further down the line. We’ve learnt that refactoring to patterns isn’t always the right way. The same can be said for code smells. Don’t let code reviews turn into the Spanish inquisition.

  2. I attended one of Jim’s connascence talks a couple of years back, during which I had a similar thought. I managed to speak to him briefly after and he agreed that some classes of smells seemed to correspond with the various connascences. I was wondering if it might lead to some kind of “taxonomy of code smells”. I’d be interested to see how such a thing might develop,

  3. I don’t think that your reasons stack up well. Many of the same objections could be levelled against patterns, if you were to ignore the context and forces sections. There are also certainly cases where several patterns overlap.

    I have to admit that I’ve never really considered code smells to be an objective quality measure. For me, they are more of a ‘placeholder for a conversation.’ Sound familiar :-?

    • Fair comment. I think smells /are/ patterns, but haven’t always been treated with the same formality of structure. And while I’d like them to be a “placeholder”, I don’t see people holding them in that regard. People seem to want them to be ‘answers’ somehow.

  4. Code smells just tell you what not to do. They don’t really tell you how to go about dealing with the smell. Using connascence gives you a direction for improvement (refactor to a lesser form of connascence). It’s positive to say what should be done, rather than just say what should not be done.

  5. I found that in practice, it’s not true that (as an enthusiastic Kent Beck or Ron Jeffries said a while ago) all the design books you need to do XP is the TDD book and the Refactoring book. I personally didn’t understand many things about how to write effective code until I started going back to old OOD literature. I ended up buying lots of dusty second-hand out-of-print books :-) I don’t think I would have learned as much by just removing smells. Same goes for just following the four rules of Simple Design. I found that this is true also of many developers I know and worked with and coached. The problem is that no simple advice like “go after smells” or “remove duplication” or “respect SOLID” is enough. These things mostly tell you what to avoid, not what to look for. They are excellent weapons for criticizing other people’s code :-)

    We need a lot more to teach good design. I’m not sure exactly what; your book is certainly a step in the right direction; we need plenty of *exercises*, not just theory. We also need theory!

  6. Reading the second paragraph I thought the list was about the metaphor. My list started out as: a smell can be hard to trace when it is strong; when the problem is fixed, the smell can remain; to track down a smell you may need a dog… But it turns out I was actually closer to the mark than I thought, because we don’t have adequate language to describe real smells or tastes. Think of those wine experts: “I’m getting oak…I’m getting mushrooms…I’m getting on your nerves.” :-)

    Smells operate at a more subconscious level, as those who write about putting the smell of baking bread into shops, or fresh coffee into a house you want to sell, point out.

    Mabye “vague”, “hunch”, and so on, are appropriate here. I think this might be bumping into expertise, and “tacit knowledge”, which is inherently difficult to express. Andy Hunt’s Pragmatic Thinking and Learning talks about this a bit. Once you are past following rules, and then past bending rules, you do things by what “feels” right, but you can no longer explain it. I am not saying it cannot be codififed, or that it should not be codified, but that it may take a while yet.

    The CASE tools have not won yet, which suggests this is nontrivial to codify.

  7. Feature Envy differs from Inappropriate Intimacy in that when you have Feature Envy it’s obvious that you’re accessing the public members/methods of another class, inappropriate is when it’s really inappropriate as when you exposure members that should be private as public, Feature Envy is a small easy to fix issue, but not a big design issue.

    Still I completely agree about the name of the smells being tricky, and I will investigate about that Connascence thing.

  8. Pingback: A problem with Primitive Obsession | silk and spinach

Leave a Reply

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

WordPress.com Logo

You are commenting using your WordPress.com 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 )

Google+ photo

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

Connecting to %s