Lessons learned and Code Smells detected with Reek

From the several static analysis tools we support on Ebert, Reek has become one of the most important tools that helps our team to write better Ruby code.

Unlike most tools, Reek is not a “linter” that checks for formatting or naming inconsistencies, but it checks for code smells on Ruby code, symptoms that might indicate deeper problems in the design of your app, from the internals of a specific class to how one entity communicates with another inside your system.

Some of the code smells that Reek looks for can teach you about aspects of the code that you might not be paying attention to. Here are some of the smells that have taught us some valuable lessons about our code.

Instance Variable Assumption

Instance variables in Ruby can be tricky — uninitialized instance variables will be nil while unitialized local variables will throw a NameError instead. This can be a source of bugs if different methods depending on the same instance variable but can’t assert that the variable was initialized as expected yet.

class APIWrapper
  def build_client
    @client = APIClient.new
  end
  def get(path)
    @client.get(base_uri + path)
  end
  def post(path)
    @client.post(base_uri + path)
  end
end

Both APIWrapper#get and APIWrapper#post assume that @client will be an APIClient instead without any guarantee that the variable will be properly initialized, requiring that whoever uses the APIWrapper needs to send build_client first.

There a few ways to deal with an Instance Variable Assumption:

1. Replace build_client with an initialize method that creates the @client for the APIWrapper instance.
2. Add a client method that memoizes the @client object, and change the get and post methods to use the client method instead of the instance variable.

I personally prefer private reader methods with memoization (or delegation, when necessary) to avoid instance variables completely, aiming encapsulation through methods over mutable shared state — this is extra helpful if you throw inheritance in the mix.

class APIWrapper
  def get(path)
    client.get(base_uri + path)
  end
  def post(path)
    client.post(base_uri + path)
  end
  private
  def client
    @client ||= APIClient.new
  end
end

Data Clump

A Data Clump smell is present when multiple methods of a class receive the same parameter lists, like:

class PostRepository
  def draft(title, description, author)
    # code code code 
  end
  def publish(title, description, author)
    # code code code 
  end
  def update(id, title, description, author)
    # code code code 
  end
end

All three methods from PostRepository receive title, description and author. If we need to add a new property to a Post, like tags, we would have to change all three methods to receive the new property, pushing further the duplication of parameters on our methods.

Martin Fowler’s on Data Clump is straight to the point:

Often data clumps are primitive values that nobody thinks to turn into an object.

The first step is to replace data clumps with objects and use the objects whenever you see them. An immediate benefit is that you’ll shrink some parameter lists. The interesting stuff happens as you begin to look for behavior to move into the new objects.

A clever Rubyist might suggest using a Hash in the lines of def publish(attributes = {}), but a Hash is not the best abstraction for situations like this. Data Clumps are best handled by introducing new abstractions that represent data structures that exist in the domain of your application. In our example, a Post PORO could do the trick:

Post = Struct.new(:title, :description, :author)
class PostRepository
  def draft(post)
    # code code code
  end
  def publish(post)
    # code code code
  end
  def update(id, post)
    # code code code
  end
end

Simulated Polymorphism

Simulated Polymorphism is a collection of three different Code Smells: Nil Check, Manual Dispatch and Repeated Conditional. These smells don’t look so problematic at first (and sometimes a something.nil? or object.respond_to?(:method) is enough to get the job done) but multiple displays of these smells in your codebase might indicate the need for better abstractions and encapsulation to reduce the complexity of your code and the cognitive work required to understand and the jumps and hoops that your code is doing.

They can be resolved in several ways: to reduce nil and other type checks, you can ensure that invalid data does not cross specific boundaries of your application or use a Null Object as a fallback for missing data.

Reek is an incredible tool for the Ruby ecosystem, and the result of the excellent OSS work from its core team: Timo Rößner, Matijs van Zuijlen, Piotr Szotkowski and Waldyr Souza. If you are interested in looking behind the curtains I strongly recommend you to follow the discussions and evolution of Reek on GitHub.

Ebert automates code reviews with Reek and other static analysis tools so so your team can ship better code sooner and safer. Sign up for an account now and get start reviewing your repositories today!