Refactoring Rails with pattern-matching (Noaidi)

More than 3 months have passed since I published my previous post about pattern-matching in Ruby. I received quite a lot of feedback, most of which was positive. But from the rest of comments I understood one thing: I didn’t explain enough why it is worth to have pattern-matching in the first place.

Since then I knew I had to write a follow-up post where I try to show the benefits. I’m using simple Rails blog-like scaffold (oh God, I haven’t used scaffolds since 2008 or so) as a base for refactoring, in which I try to prove my point.

Create and update – the obvious candidates

We are going to edit a controller, which Rails generously generated for us. In the beginning we focus on actions that actually do stuff, i.e. create and update. To get first things first, we are going to start with removing all the respond_to do |format| parts, which are totally useless anyway. Then, let’s look at what we are left with:

def create
  @post = Post.new(post_params)

  if @post.save
    redirect_to @post, notice: 'Post was successfully created.'
  else
    render :new
  end
end

The action part here is where code branches into two possible flows: when we perform @post.save it is either true or false. Depending on this, we decide what to call next. This dichotomy – that something might be true or not – is what we witness a lot in programming. But is it really what we want to achieve? It is quite easy to tell what does it mean when @post.save returns true: the relevant record has been successfully saved in the database. What if it’s false? It actually means that record is invalid, which almost always indicates that validation did not pass. We can make sure about that by looking how the save method is defined (in Rails 4.2):

def save(*)
  create_or_update
rescue ActiveRecord::RecordInvalid
  false
end

What’s this? Another binary branching, but this time using exceptions for controll flow (yes!). The question when you see the code like this should be: can create_or_update throw different error than ActiveRecord::RecordInvalid?

Of course it can! That means that we actually have 3 possible outcomes of trying to save the post: one if everything goes ok, second if ActiveRecord::RecordInvalid is raised and third when any other error appears. Armed with that knowledge we can write our save_post method, which explicitly states those 3 possibilities:

def save_post(post)
  if !post.valid?
    return [:invalid, @post.errors]
  end

  begin
    post.save!
    [:ok, post]
  rescue StandardError => e
    [:error, e]
  end
end

This might seems a bit cumbersome, but at least it tells what our options are: if the post is invalid, return imediately (without pretending that we want to save it), otherwise try to save it and return appropriate symbol-object pair. We are good to go with Noaidi-based pattern-matching now, so let’s rewrite our create action using this method:

def create
  @post = Post.new(post_params)

  Noaidi.match save_post(@post), {
    [:ok, Post] => ->(post) { redirect_to post, notice: 'Post successfully created' },
    [:invalid, any] => ->(validation_errors) { render :new },
    [:error, StandardError] => ->(error) { render :error_500, error: error }
  }
end

Here we are clearly stating what can happen and how we react to it. Especially (and this is new compared to original implementation) if something grave happens (for example database goes down or we simply violate some constraint) we take explicit action – in this case rendering some error_500 template, when we can try to explain to the user what happened, instead of generic “Something went wrong” page.

Can we refactor show?

def show
  @post = Post.find(params[:id])
end

It is hard to imagine a simpler controller action. Can we make anything better here?

The answer is: yes. But let’s start with another question: What happens when there is no record with given id? The answer is simple. ActiveRecord::RecordNotFound is raised, Rails intercepts it and displays a generic 404 page. Is it desired? It might be, however… If your application allows to delete records you can be sure that sooner or later someone will bump into a link to non-existing record somehow, be it from Google, link on another website or bookmark in the browser. Do you really want to punish the user with 404 that does not say anything? You should not.

Instead, you could provide the user with a list of latest posts, perhaps. Or maybe some random posts – whatever you like. The point is, you should not left the user alone, because it’s your app (with deletion allowed) that caused him to end up with a no-longer-valid resource URL. Let’s write find_post method then:

def find_post(id)
  post = Post.find_by(id: id)
  if post
    [:ok, post]
  else
    [:not_found, other_posts(id)]
  end
end

other_posts in this case is the method returning desired list of, well, other posts, that will be presented to user. It can be as simple as Post.all. Now the show action could look like:

def show
  Noaidi.match find_post(params[:id]), {
    [:ok, Post] => ->(post) { render locals: { post: post } },
    [:not_found, Array] => ->(posts) { render :not_found }
  }
end

Again, we write very explicitly what possible outcomes are. You may think that, since there are just two options, we could use if as well. That’s true, but try to have a bigger picture in mind. If instead of just deleting posts from database you will mark them as deleted or unpublished, you might want different behaviour for posts that were previously available and now are not, and different posts that never existed (the latter is probably some malicious application probing, not coming via a dead link).

By the way, if you really used Post.all as your other_posts method and tried to request a non-existing post, you will get a No match exception from Noaidi. Why? Because Post.all does not return Array. Did you know that? Now you do, thanks for pattern-matching ;)

Few more words

1. Remember that I’m not trying to say that pattern-matching is by far superior to a standard Ruby-way. I’m only saying that it might be useful, especially when you get used to it and start to think outside the true-false box. If I haven’t convinced you, I probably won’t, no matter what I’d write. And that’s totally fine. Let’s all use what we think is best, without getting to proving anything to each other or to excessive preaching.

2. The code after this refactoring is still far from being perfect. find_post, other_posts or save_post should not belong to the controller, but now you can easily extract them to separate service objects, repository objects or whatever you like. This way you leave your controller clean, without any additional unnecessary non-action code.

3. Last but not least, decoupling. Note that in save_post I used if [email protected]? snippet. But since I’m validating explicitly, I’m no longer bound to ActiveModel validations. There’s much less stopping me from using for example dry-validation instead. I see this as another benefit: by clearly writing what you are doing step by step you spot places for improvement more easily.