Want to refactor some code but don't know where to start? Just implemented a functionality and want to clean up the code? Need to review a gigantic pull request and find it hard to understand?
Start with spotting and removing code smells.
If it stinks, change it.Grandma Beck
I designed this cheat sheet to help you identify code smells. The first column describes symptoms of a code smell. The second column lists the name of a code smell.
This cheat sheet is my notes from the wonderful Refactoring book written by Martin Fowler. I strongly recommend you to get a copy if you don't have one already.
Use this cheat sheet to check your code every time you need to refactor it. Once you spot any of the described symptoms, you will now know the name of the smell and can research how to remove the smell.
Code Smell Cheat Sheet
|- Duplicated codes
- Same code structure or expression in more than one place
|- A long method||Long Method||- Long methods are bad because long procedures are hard to understand.
- Name a small method after the intention of the code, not implementation details. Small methods should have good names that reveal the intention of the code.
- "The key here is not method length but the semantic distance between what the method does and how it does it."
|- A large class
- A class that's trying to do too much
- A class with too many instance variables
|- A long parameter list||Long Parameter List||- Long parameter lists are bad because they are hard to understand and use and can easily become inconsistent.|
|- A class is commonly changed in different ways for different reasons
- A class suffers many kinds of changes.
|Divergent Code||What we want are:
- "When we make a change we want to be able to jump to a single clear point in the system and make the change."
- Each object is changed only as a result of one kind of change.
- Ideally, have a one-to-one link between common changes and classes.
|- A change requires alerting many classes
- When you want to make a kind of change, you need to make a lot of little changes to a lot of different classes.
|Shotgun Surgery||"When the changes are all over the place, they are hard to find, and it's easy to miss an important change."|
|- A method seems more interested in another class than the one it actually is in.
- A method does not leverage data or methods from the class it belongs to. Instead, it requires lots of data or methods from a different class.
|- Three or four data items clump together in lots of places such as fields in a couple of classes or parameters in many method signatures.||Data Clumps||- "Bunches of data that hang around together really ought to be made into their own object."|
|- Using multiple primitive data types to represent a concept such as using three integers to represent a date||Primitive Obsession||- Don't be afraid to use small objects for small tasks such as money classes that combine number and currency|
|- A switch statement that is duplicated in multiple, different places. If you add a new clause to the switch, you have to painstakingly find each scattered switch statement and change it.||Switch Statements||- "One of the most obvious symptoms of object-oriented code is its comparative lack of switch (or case) statements."
- Consider polymorphism when you see a switch statement.
|- Parallel inheritance hierarchies
- Every time you make a subclass of one class, you also have to make a subclass of another.
- Prefixes of the class names in one hierarchy are the same as the prefixes in another hierarchy.
|Parallel Inheritance Hierarchies||- "Parallel inheritance hierarchies is really a special case of shotgun surgery."|
|- A class that isn't doing enough to pay for itself||Lazy Class||- "Each class you create costs money to maintain and understand."|
|- The only users of a method or class are test cases.||Speculative Generality||- This happens when people thought they need a method or class for a future requirement but it turned out they didn't really need it.|
|- An instance variable is set only in certain circumstances.||Temporary Field||- "Such code is difficult to understand, because you expect an object to need all of its variables. Trying to understand why a variable is there when it doesn't seem to be used can drive you nuts."|
|- A method calling a different method which calls a different method which calls a different method ...||Message Chains||- A message chain couples a client of the method to the structure of the navigation. Any change to the intermediate relationships requires the client to have to change.|
|- A class with lots of methods delegated to this other class||Middle Man||n/a|
|- Classes delving in each others' private parts too much||Inappropriate Intimacy||n/a|
|- Methods that do the same thing but have different signatures for what they do||Alternative Classes with Different Interfaces||n/a|
|- Trying to modify a library class to do something you'd like it to do||Incomplete Library Class||n/a|
|- Classes have nothing but fields and getters and setters for these fields.
- Classes act as dumb data holders and are manipulated in far too much detail by other classes.
|Data Class||- "Data classes are like children. They are okay as a starting point, but to participate as a grownup object, they need to take some responsibility."|
|- A subclass only uses a few methods or data given by the superclass (Unless it's causing confusion and problems, this smell is too faint to be worth cleaning.)
- A subclass does not want to support the interface of the superclass.
|- Using comments to explain what a block of code does||Comments||- Use comments to indicate areas you are not sure and to say why you did something.
- "When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous."