Don't do this at home

A big part of our day-to-day work is to review other people’s code. It is very important for several reasons: for guiding less experienced developers and teaching them the right way of doing things, for learning from other developers (everyone has something to teach and everyone’s got something to learn), and most importantly, to deliver quality code.

But sometimes, when reviewing other people’s work one can find pieces of code that might be working and serving its purpose, but its implementation is poor. In this case, I stumbled upon something worth sharing so anyone can learn something from these mistakes.

The following is an extract from a Ruby on Rails application. In this case, this is a controller action that when called should call a service object and, if successful, redirect to a different route. And, if not successful it should show the form again and display a flash message with the error.

def create
  result = SomeService.new(service_params).call

  if result == true
    redirect_to root_url, notice: t(".success")
  else
    render :new, alert: t(".error", message: result)
  end
end

Notice something?

Exactly, the result of calling SomeService is either a Boolean or a String. This is a problem for a couple of reasons. The first reason is semantics. While the semantic of the return value of the service is intended to tell us if it was successful or not, in practice it will also say what was the error that prevented the service from being successful. The second reason is that you won’t find conditions like result == true in Ruby code in general. This is, for me, a smell, an indication this must be wrong.

What are the possible solutions then? There are a couple of alternatives.

The first one is, intuitively, to return a Hash that holds the result of calling SomeService. For instance:

class SomeService
  def call
    # here's where the magic happens

    if service_was_successful?
      {result: :success}
    else
      {result: :error, message: "Something went wrong :("}
    end
  end
end

The controller, on the other hand, will look much more expressive:

def create
  some_service = SomeService.new(service_params).call

  if some_service[:result] == :success
    redirect_to root_url, notice: t(".success")
  else
    render :new, alert: t(".error", message: some_service[:message])
  end
end

An improvement of this is to return a ServiceResult object instead of a Hash. This object would respond to #success? and #errors and with it not only do we have a better, more expressive controller, but also we are defining a contract, an interface, that all other services in our application should agree on.

Another approach is using a built-in mechanism we already have available that we can use in these situations: exceptions. Let’s think about it. The normal outcome of a service should be success and only fail under certain circumstances. This way, there’s no need to remember what the return value of the service should be. Instead, we should understand what it does and what kind of errors may occur. Let’s reimplement this service again:

class SomeService
  class ThirdPartyServiceUnavailableError < ::Error
  end

  def call
    # here's where the magic happens

    # the service may continue (or not) after this line
    raise ThirdPartyServiceUnavailableError if third_party_service_is_unavaiable?

    # ...
  end
end

The controller also will look much more expressive and direct:

def create
  SomeService.new(service_params).call
  redirect_to root_url, notice: t(".success")
rescue ThirdPartyServiceUnavailableError => e
  render :new, alert: t(".error", message: e.message)
end

However, this is not recommended in most cases as exceptions are expensive and should only be, as its name suggests, exceptional. They’re more than fine to handle the example above (ie a third-party service being unavailable), but it could become extremely troublesome if all business logic relies on them.

As always, remember there’s no rule of thumb on the approach one must take. Each of these alternatives has its benefits and drawbacks, and there’s usually no single correct answer to a design problem. We use all of them in our production code bases. Although we prefer the powerful consistency of result objects, they are not always a good fit for every app (eg. adding this concept to apps that have 7-10 years in production and a set of very different design choices, might bring more noise than benefits and yet another concept to load and process on each team member’s head).

In conclusion, these are all valid choices, always evaluate and pick the tools that work best for the current situation of the business.