When to Refractor - Bad Smells in Code
Notes from Refactoring Ruby by Jay Fields, Shane Harvie, Martin Fowler & Kent Beck.
Problem – This stinks, when you have the same expression in two different methods in the same class
Solution – Extract the method to a new method in the class and use in place
Duplication in Sibling Classes
Problem – Two methods in sibling class that that have the same expression.
Solution – Extract the method into one on the shared base class and use this instead.
Duplication across unrelated classes
Problem – The same method or expression across two unrelated classes
Solution – Extract the method into a new class or module and then use either inheritance or composition to give the class the new method.
Problem – The longer the method the harder it can become for someone to understand
Solution – Long methods should be broken down into smaller methods with a single responsibility
Problem – Code within a method needs a comment to explain what it is doing ie the distance between what the method does on the surface and how it does it internally
Solution – The code should be extracted into a new method and given a new that describes its function clearly
Other potential problems – Conditionals and loops can give sign for extractions. Loops can be switched out with collection closure methods and then these can be extract out further
Problem – Lots of instance variables in a class is generally a sign that the class is doing to much. Especially when different instances of a class use different sets of variables
Solution – Instance variables that make sense to go together should be grouped with any associating methods and extracted to a subclass, separate class or module
Long Parameter List
Problem – passing long lists of parameters into a function can become hard to understand and difficult to follow.
Solution – Swap these long lists with passing the object in instead or creating a name parameter instead
Caveat – This can increase coupling between objects by passing objects through instead of parameter lists. Instead do use parameter list but be aware of the trade off
Problem – Changes to the same object happen to completely separate reasons. ie the same object needs to be changed when a user type is added or when a new sport is added.
Solution – A sign that this class is doing to much and should instead be to separate classes
Problem – The opposite of Divergent Change, when you have to make changes across multiple different classes when you make one type of change.
Solution – Move the methods and fields into there own classes this could be an in-line class, existing or separate class.
Problem – A method being more interested in the data from another class then its own
Solution – Move the envious methods over to the other class
Not so cut a dry – If the method uses data from several classes the general rule of thumb is that the method belongs in the class of which the most data is used. Or put things together that change together
Problem – You frequently see the same instances variables hanging around together, passed into the same methods together.
Solution – extract these same parameters into a class of there own and then change the method signatures to use the object. As long as your replacing two or more instance variables you’ll come out ahead. One way to test if variables belong together is to delete one and see if the rest still make sense.
Problem – using primitive types ie (string, float, fixnum etc.) when a small object would be better
Problem – Case statements are a sign of duplication usually. One case statement could appear several times around the code base as its usually switching on a type code
Solution – Polymorphism, depending on the number of case statements could be overkill and using an explicit method or state/strategy be better
Parallel Inheritance Hierarchies
Problem – Every time you create a subclass of one class you also make a subclass of another class. Usually the prefixes of the two different hierarchies will be the same
Problem – A class/module that doesn’t do enough to justify it being around
Solution – Collapse the class hierarchy or make the classes/modules in-line
Problem – When design decisions are taken for potential or future use cases that aren’t in the current spec or brief. Increase complexity since the things aren’t required
Solution – Remove this code, good highlighter is when the only users of a method or class are the test cases!
Problem – When a instance variable is only set in certain circumstances. Code is difficult to understand due to the expectation of objects using all there variables
Solution – Extract class on the concerning code or introduce a null object to create alternative component
Problem – Long chains of objects reaching across other objects for information. Introduces tight coupling of the structure make changes more difficult
Solution – You can hide the delegates by forwarding on methods to reduce the chain length or you can look at the resulting method and potentially extract it and move it down the message chain
Problem – When looking at a classes interface we see that most of the methods delegate to another object in the class.
Solution – There comes a point when it becomes better to remove the middle man and talk directly to the object. If its only a few methods you can in-line them into the caller or if it is a lot you can replace delegation with a hierarchy and include a module instead
Problem – Class that know far to much about other classes private parts.
Solution – Overly intimate class need to be broken up where its moving methods or fields , or extracting classes or hiding delegates it depends on the severity of the infidelity
Alternative Classes with different Interfaces
Problem – Methods that do the same thing but have different signatures for what they do.
Solution – Move method to the classes till there protocols are the same
Incomplete Library class
Problem – A third party library is missing a method / algorithm / data type you need
Solution – Monkey patching classes in ruby makes small additions easily possible
Problem – A class that just has attributes and nothing else. They are most certainly being manipulated in far to much detail by other classes.
Solution – Look at moving these attributes out to other classes with more responsibility