| CARVIEW |
*
programming and human factors
by Jeff Atwood
May 18, 2006
Code Smells
I'm often asked why the book Refactoring isn't included in my recommended developer reading list. Although I own the book, and I've read it twice, I felt it was too prescriptive-- if you see (x), then you must do (y). Any programmer worth his or her salt should already be refactoring aggressively. It's so essential to the craft that if you have to read a book to understand how it works, you probably shouldn't be a programmer in the first place.
There's nothing wrong with codifying refactoring guidelines in a book. But the most important guideline is to watch for warning signs in your own code-- so called "code smells".
Developing your "code nose" is something that happens early in your programming career, if it's going to happen at all. I combined all the documented code smells I could find into this reference; most of these smells should be familiar to you.
Code Smells Within Classes
| Comments | There's a fine line between comments that illuminate and comments that obscure. Are the comments necessary? Do they explain "why" and not "what"? Can you refactor the code so the comments aren't required? And remember, you're writing comments for people, not machines. |
| Long Method | All other things being equal, a shorter method is easier to read, easier to understand, and easier to troubleshoot. Refactor long methods into smaller methods if you can. |
| Long Parameter List | The more parameters a method has, the more complex it is. Limit the number of parameters you need in a given method, or use an object to combine the parameters. |
| Duplicated code | Duplicated code is the bane of software development. Stamp out duplication whenever possible. You should always be on the lookout for more subtle cases of near-duplication, too. Don't Repeat Yourself! |
| Conditional Complexity | Watch out for large conditional logic blocks, particularly blocks that tend to grow larger or change significantly over time. Consider alternative object-oriented approaches such as decorator, strategy, or state. |
| Combinitorial Explosion | You have lots of code that does almost the same thing.. but with tiny variations in data or behavior. This can be difficult to refactor-- perhaps using generics or an interpreter? |
| Large Class | Large classes, like long methods, are difficult to read, understand, and troubleshoot. Does the class contain too many responsibilities? Can the large class be restructured or broken into smaller classes? |
| Type Embedded in Name | Avoid placing types in method names; it's not only redundant, but it forces you to change the name if the type changes. |
| Uncommunicative Name | Does the name of the method succinctly describe what that method does? Could you read the method's name to another developer and have them explain to you what it does? If not, rename it or rewrite it. |
| Inconsistent Names | Pick a set of standard terminology and stick to it throughout your methods. For example, if you have Open(), you should probably have Close(). |
| Dead Code | Ruthlessly delete code that isn't being used. That's why we have source control systems! |
| Speculative Generality | Write code to solve today's problems, and worry about tomorrow's problems when they actually materialize. Everyone loses in the "what if.." school of design. You (Probably) Aren't Gonna Need It. |
| Oddball Solution | There should only be one way of solving the same problem in your code. If you find an oddball solution, it could be a case of poorly duplicated code-- or it could be an argument for the adapter model, if you really need multiple solutions to the same problem. |
| Temporary Field | Watch out for objects that contain a lot of optional or unnecessary fields. If you're passing an object as a parameter to a method, make sure that you're using all of it and not cherry-picking single fields. |
Code Smells Between Classes
| Alternative Classes with Different Interfaces | If two classes are similar on the inside, but different on the outside, perhaps they can be modified to share a common interface. |
| Primitive Obsession | Don't use a gaggle of primitive data type variables as a poor man's substitute for a class. If your data type is sufficiently complex, write a class to represent it. |
| Data Class | Avoid classes that passively store data. Classes should contain data and methods to operate on that data, too. |
| Data Clumps | If you always see the same data hanging around together, maybe it belongs together. Consider rolling the related data up into a larger class. |
| Refused Bequest | If you inherit from a class, but never use any of the inherited functionality, should you really be using inheritance? |
| Inappropriate Intimacy | Watch out for classes that spend too much time together, or classes that interface in inappropriate ways. Classes should know as little as possible about each other. |
| Indecent Exposure | Beware of classes that unnecessarily expose their internals. Aggressively refactor classes to minimize their public surface. You should have a compelling reason for every item you make public. If you don't, hide it. |
| Feature Envy | Methods that make extensive use of another class may belong in another class. Consider moving this method to the class it is so envious of. |
| Lazy Class | Classes should pull their weight. Every additional class increases the complexity of a project. If you have a class that isn't doing enough to pay for itself, can it be collapsed or combined into another class? |
| Message Chains | Watch out for long sequences of method calls or temporary variables to get routine data. Intermediaries are dependencies in disguise. |
| Middle Man | If a class is delegating all its work, why does it exist? Cut out the middleman. Beware classes that are merely wrappers over other classes or existing functionality in the framework. |
| Divergent Change | If, over time, you make changes to a class that touch completely different parts of the class, it may contain too much unrelated functionality. Consider isolating the parts that changed in another class. |
| Shotgun Surgery | If a change in one class requires cascading changes in several related classes, consider refactoring so that the changes are limited to a single class. |
| Parallel Inheritance Hierarchies | Every time you make a subclass of one class, you must also make a subclass of another. Consider folding the hierarchy into a single class. |
| Incomplete Library Class | We need a method that's missing from the library, but we're unwilling or unable to change the library to include the method. The method ends up tacked on to some other class. If you can't modify the library, consider isolating the method. |
| Solution Sprawl | If it takes five classes to do anything useful, you might have solution sprawl. Consider simplifying and consolidating your design. |
This list was derived from the Smells to Refactorings PDF, and the Smells to Refactorings Wiki, which also provide additional guidance on the specific refactorings that might be helpful in each instance. The important thing, from my perspective, isn't the refactoring-- it's learning to recognize the scent of your own code.
And if you want examples of the stinkiest code imaginable, How to Write Unmaintainable Code is a good place to start.
There's also a nice, simplified taxonomy of code smells here:
https://www.soberit.hut.fi/mmantyla/BadCodeSmellsTaxonomy.htm
He (correctly, IMO) groups the smells into five general categories:
Bloaters
OO Abusers
Change Preventers
Dispensables
Couplers
I found "How To Write Unmaintainable Code" about a year ago, and I could not stop laughing. Re-read it this morning, and I still can't stop :-)
Thanks, Jeff!
pinano on May 20, 2006 01:04 AM"Overload and Bewilder
In C++, overload library functions by using #define. That way it looks like you are using a familiar library function where in actuality you are using something totally different."
Or, overload it to have an A and a W version.. :)
"The more parameters a method has, the more complex it is. Limit the number of parameters you need in a given method, or use an object to combine the parameters"
Im just curious -- doesnt this violate some principle by having objects to tightly coupled?
joe on May 20, 2006 06:56 PMHey, I know that Cheese Shop! It's the one in Rockridge right next to Bart! I bet you sip your latte and refactor away in the caffe next door, with a splendid chunk of Osau Iraty and Triscuits you lucky dog...
Sorry, nothing much else to contribute :).
Rob Conery on May 21, 2006 01:02 AM> Any programmer worth his or her salt should already be refactoring aggressively.
But as a practical matter, many organizations actively inhibit refactoring, and teach the programmers that "this code is too complex to change."
> It's so essential to the craft that if you have to read a book to understand how it
> works, you probably shouldn't be a programmer in the first place.
A little overzealous, in my opinion. -If- the programmers have been trained correctly, they should have picked up some form of refactoring skills. But in many cases, they haven't; they've even been trained that old, crufty code is gold and must -never- be changed.
I don't feel it's reasonable to imply all these coders are a dead loss because they got started on the wrong foot.
> There's nothing wrong with codifying refactoring guidelines in a book. But the most important
> guideline is to watch for warning signs in your own code-- so called "code smells".
If your job is >50% writing your own code. Lots of maintenance programmers write very little code from scratch, and spent almost all there time working with someone else's crufty code.
> Developing your "code nose" is something that happens early in your programming career,
> if it's going to happen at all.
9 times out of ten, you're right. But every time someone says something like this, in any field, someone appears as an outlier on the skills graph.
You make some good points; I just feel you've overstated your arguments on this one.
Ron Ruble on May 21, 2006 06:02 AMArgh, I accidentally deleted Jeff Perrin's comment while cleaning up blog spam. My apologies, Jeff.
As I recall, Jeff said that I erred in calling Refactoring prescriptive. He said that the book actually encourages you to be flexible.
Jeff Atwood on May 21, 2006 07:48 PMI rarely pull rank on my team, but this time I've made this post required reading. Great stuff.
That said, there's only one smell I'm not quite sold on. Data Classes. I've found them quite handy when passing between classes (i.e. Long Parameter List). Also useful when you have a parent / child type relationship, and are basically just storing the data in a collection for reference or display.
Also I thought Ron Ruble made some interesting points about maintenence coders. Often many get their start that way, and are told to get in and get out as quickly as possibe with minimal impact to the code. More changes require more testing, and all management wants is to get it fixed as quick as possible, not sponsor a rewrite because it's "smelly".
Thanks again, great post!
Robert
Great post Jeff!
Andrej Gaevskij on May 21, 2006 11:19 PMI think that refactoring alone solves only part of the problem. To really make the code better you must have a good understanding of unit testing and patterns in addition to refactoring. I don?t feel comfortable refactoring code that has no unit tests so the first thing I do when I have to work on someone else?s crud code is to add unit tests.
Derek Pierson on May 23, 2006 06:18 PMActually, Jeff, the "code smells" metaphor came out of a discussion between Martin Fowler and Kent Beck while Fowler was still writing the Refactoring book.
Together, they wrote chapter 3, "Bad Smells in Code," which is not prescriptive at all, but does present a catalog of smells.
You may not be impressed by the refactoring catalog itself, but IMHO you should at least add the first four chapters to your recommended reading list.
I found the book most valuable for the non-reference part; for instance, the discussion about how to explain just what you're doing when you're refactoring was helpful for me.
I also liked the example he used at the beginning, which seemed more realistic and creative than a lot of the book examples I see (e.g. Kent Beck's Money example in TDD by Example). I did my first test-backed refactoring by working through Fowler's movie rental code.
Kyralessa on May 25, 2006 05:14 PMTo me Refactoring is not "You see this then do this" but more "If you want to this change, here are the steps to do it safely and reliably".
Don't get me wrong, your list of code smells are good but they what I would call anti-patterns. I.e. patterns of bad code that should send up a red flag.
Rob Conley on June 7, 2006 05:11 AMI am currently having to deal with, Long Method, Conditional Complexity, Large Class, Refused Bequest, Inappropriate Intimacy, Shotgun Surgery and Oddball Solution. Its pure hell.
Dave on January 25, 2007 09:11 PMGreat article, but I also have a hold out for targeted usage of DataClasses; they have their uses when all you need to do is buffer a "record" style collection; particularly if a "record" contains one or more internal collections as well.
Shrike on March 17, 2007 11:23 PMSurely the cheese shop in the photot is the Neals Yard Dairy? Purevyors of the famous Stinking Bishop...
Steve Freeman on May 2, 2007 09:07 AMSome points of disagreement:
1) Assuming that you are right in that most of us know how to refactor, I think that's because experience gave us the needed knowledge. Time and pain was needed. This time and pain, as in any knowledge area, can be reduced by reading books, like Fowler's.
2) Apart from this, I think even a developer with experience and knowledge of most refactorings could miss some smells. We are humans and we acquire bad habits.
3) A big part of the knowledge gained with experience is too intuitive and that brings some problems. When things are intuitive and you suddenly find someone that doesn't share your intuition you have to fight back with solid explicit arguments. Fowler's book helps you ease the work needed to find those arguments letting you show clearly why your intuition is the way it is. You can do it yourself of course, but it's easier to reuse the effort someone else did, if you share it of course.
4) The book defines a vocabulary to deal with our intuition or implicit concepts, that is very very important. Now we can share it and we can communicate more efficiently. The same advantage we discovered after design patterns appeared.
5) You agree with Fowler that smells are important. The book is a reference to smells. IMO refactoring is more about identifying design problems (smells) than about the relatively simple things needed to make them disappear, but it's both of course.
6) Things that are important, like smells, must be made explicit so the problem can be easily studied and possibly build some more knowledge on higher level concepts. One of those higher level concepts that could be further developed was the development philosophy of Continuous Design in which a core concept is refactoring.
7) I think there's a confusion between simplicity and importance. Not only complicated books are important.
Daniel Cadenas on December 8, 2007 01:17 PMSo, what's the difference between the "middleman" code smell and the "facade" design pattern?
KG on December 27, 2007 02:11 PMThe link to How to Write Unmaintainable Code was blocked by our firewall with a nice big red ACCESS DENIED. Category: Computer Crime.
Had me rolling in my cube for a while.
Adrian. on February 21, 2008 06:22 PMThanks for responding Jeff. That's a good point about usability.
youtube on March 18, 2008 10:37 AMI think you're just jealous you didn't write it? :)
The Fowler refactoring book is great for junior developers - in saying it's "too prescriptive" I think you completely miss the point -
what's important is that it introduces the object thinking that underlies his process.
It's a great introduction to object thinking and more complex issues such as use of interface inheritance and test driven development - would recommend it to my junior devs... it should be required reading for all .NET developers
The first time I read Fowler's book my response was "eh" -- most of the refactorings described stuff I'd been doing for years, stuff that should be second nature to any competent programmer. It was only when I first started using an IDE with good refactoring support that I realized the true value of the book: _Naming_ the refactorings. Not unlike patterns, in that respect.
Petter Hesselberg on May 21, 2008 06:44 AMWard Cunningham also has a nice set of code smells at his site: https://c2.com/xp/CodeSmell.html
If a code stench is a really bad code smell, what would we call an often recurring smell... a code stink?
</J>
John Boal on December 3, 2008 04:46 PM
You said:
<I> It's so essential to the craft that if you have to read a book to understand how it works, you probably shouldn't be a programmer in the first place. </I>
Looking at the popularity of the book in programmers community, your statement is suggesting that most of the programmers are not programmers.
waheed on December 14, 2008 08:29 AM
You may find it humourous that when I saw the smell for 'Dead Code', I instictively tried to vote it up. Spending too much time on StackOverflow.
John MacIntyre on February 9, 2009 08:28 AMI like the Refactoring book primarily because it gives names to the stuff I already do. The main advantage of that is communication with other people:
* for junior programmers, the book gives them the steps on how to do things
* for senior programmers, the book allows me to talk with other senior programmers that read the book with less words.
Comments: I like to know the what in the method javadocs normally, because I don't want to go through the code to figure what it does especially if I am just a consumer of the interface. I do understand the importance of the why as well.
Conditional Compexity: sometimes is necessary. (Comments come into play) writing another API to do validations for low level things (e.g. match string if provided times 10 search keys) may yield to something that looks complex, but I see no need to refactor this as it will make things harder to debug if you're going across several classes when having everything in one "validate" method is sufficient.
Archimedes Trajano on April 11, 2009 02:21 PMYou may find it humourous that when I saw the smell for 'Dead Code', I instictively tried to vote it up. Spending too much time on StackOverflow.
requiem gold on April 18, 2009 01:37 AMYou may find it humourous that when I saw the smell for 'Dead Code', I instictively tried to vote it up. Spending too much time on StackOverflow.
requiem lant on April 18, 2009 01:37 AMYou may find it humourous that when I saw the smell for 'Dead Code',
requiem money on April 18, 2009 01:38 AMI instictively tried to vote it up. Spending too much time on StackOverflow
cheap requiem lant on April 18, 2009 01:38 AMYou may find it humourous that when I saw the smell for 'Dead Code',
requiem online gold on April 18, 2009 01:38 AM| Content (c) 2009 Jeff Atwood. Logo image used with permission of the author. (c) 1993 Steven C. McConnell. All Rights Reserved. |
Subscribe in a reader
Subscribe via email