How to find common code smells and avoid them
What? How can code "smell"??
Well it doesn't have a nose... but it definitely can stink! :mask:
When we start working on a project from halfway, we very often find code smells we are not comfortable with. Martin Fowler very well explained what is a code smell, it is a surface indication that usually corresponds to a deeper problem in the software system. Your ruby code smells. and it’s okay, mine does, too! Maybe it has some long methods with too many parameters, few classes that try to do too much. No codebase is perfect, but also it can not be an excuse not to be aware of things and refactorings that could improve the state of things — so let’s look at a few example code smells and come up with ideas to fix it.
One of the common code smells is duplicate method: a situation when a given method call is made two or more times in the same context. When developer fixes a bug, but same symptoms are faced again later on, this can be the result of code duplication, and a bug being fixed in one occurrence of the imperfect code but not in the duplicated versions. This poses an overhead in terms of maintenance. Let’s assume we have a class that represents a home which has a method assessing whether the current weather helps to stay inside or leave home. To assess the weather, the Home class collaborates with a weather source and asks it for current weather at the home’s address:
class Home def weather_assessment if @weather_source.weather_at(@home).skies == :clear 'Go outside. Play football' elsif @weather_source.weather_at(@home).temp > 20 'Still Go outside. Play football but sunscreen please' else 'stay inside. sleep' end end end
In the above example, the Home#weather_assessment method makes two separate WeatherSource#weather_at calls that probably resulting in firing synchronous requests over the network by passing the same argument twice. A good way will be to make a single call and storing its result locally.
class Home def weather_assessment weather = @weather_source.weather_at(@home) if weather.skies == :clear 'Go outside. Play football' elsif weather.temp > 20 'Still Go outside. Play football but sunscreen please' else 'stay inside. sleep' end end end
Let’s create a few more methods for our home class — this time ones that use a navigation source collaborator to calculate distance, directions, and whether one needs a transport to get to the home’s geographical coordinates given a starting latitude and longitude.
class home def directions(source_lat, source_lon) @nav_source.directions(from: [source_lat, source_lon], to: [@lat, @lon]) end def distance(source_lat, source_lon) @nav_source.distance(from: [source_lat, source_lon], to: [@lat, @lon]) end def needs_transport?(source_lat, source_lon) @nav_source.borders?(from: [source_lat, source_lon], to: [@lat, @lon]) end end
Have a look how all of these methods take the same pair of parameters? This smell is called data clump, it usually means there is a higher-level object missing from the implementation. In this case such an object could be a Location struct representing the given pair of latitude/longitude coordinates:
Location = Struct.new(:lat, :lon) class home def directions(source_location) @nav_source.directions(from: source_location, to: @location) end def distance(source_location) @nav_source.distance(from: source_location, to: @location) end def needs_transport?(source_location) @nav_source.borders?(from: source_location, to: @location) end end
Let’s assume that we don’t exactly know the location of our home :flushed:.
In this case we could be tempted to guard the NavSource calls with a conditional check:
class home def directions(source_location) if @location @nav_source.directions(from: source_location, to: @location) end end def distance(source_location) if @location @nav_source.distance(from: source_location, to: @location) end end def needs_transport?(source_location) if @location @nav_source.borders?(from: source_location, to: @location) end end end
This smell is called repeated conditional – doing the same check over and over again suggests that there might be a better solution. Unless such a set of checks has exceptional readability, refactoring usually yields a better solution –but whether the refactoring should consist of introducing a separate LocatedHome class or teaching the NavSource about handling NullLocations usually highly depends on the codebase in question.
Let’s now add a predicate that says whether the developers at the given Company program in Ruby —
and let’s make it parameterizable so the caller can choose whether this should be a strict question or a loose one.
class company def ruby_developers?(strict = true) languages = @employees.map(&:primary_language).uniq if strict languages == ['Ruby'] else languages.include?('Ruby') end end end
This kind of code is called the boolean parameter smell; the caller knows exactly which code path the method will take and controls its execution from the outside. The typical refactorings are to either split the parameterized predicate into dedicated methods or to split the class into more classes, each of which would implement one of the code paths. In this case it seems separate methods work quite well:
class company def only_ruby_developers? languages == ['Ruby'] end def ruby_developers? languages.include?('Ruby') end private def languages @employees.map(&:primary_language).uniq end end
A code smell that’s a bit more problematic to refactor, but happens quite often, is feature envy.
Let’s add an company#good_fit? method that assesses whether a given company is a good fit for a given employee (passed in an argument); we consider the company a good fit when the employee is sociable or likes the location:
class company def good_fit?(employee) employee.sociable? || employee.likes?(@location) end end
Notice how this method is much more interesting in interrogating its parameter; if it weren’t for the reference to the @location instance variable it would be a utility function — a method that operates solely on its arguments and can be moved anywhere in the system without any change to its body.
Refactoring feature envy smells is more involved, but the general notion is the same: There is a chance that this method would work better if it was implemented in the context of its parameter:
class company def good_fit?(employee) employee.would_like_to_work?(@location) end end class Employee def would_like_to_work?(location) sociable? || likes?(location) end end
To find common code smell we can also use a tool called Reek, It is a code smell detector that can analyze Ruby files and pinpoint the smells, and you can think about it as a RuboCop for your architecture and code quality. In next series, I will try to focus on configuring and using reek. :wink:
Now that you know how to find — and, potentially, refactor — smells in your code it’s super important to remember that smells do not necessarily mean that the code is bad.Investigating your code smells and refactoring the ones that are indeed troublesome — but also learning how to accept the ones that are not; both experiences will let you level up as a developer.