When to Refractor - Bad Smells in Code

Notes from Refactoring Ruby by Jay Fields, Shane Harvie, Martin Fowler & Kent Beck.

Duplicated Code

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.

Long 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

Large Class

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

Divergent Change

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

Shotgun Surgery

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.

Feature Envy

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

Data Clumps

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.

Primitive Obsession

Problem – using primitive types ie (string, float, fixnum etc.) when a small object would be better

Case Statments

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

Lazy class

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

Speculative Generality

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!

Temporay Field

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

Message Chains

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

Middle Man

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

Inappropriate Intimacy

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

Data Class

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

Comments