Refactoring Rails with pattern-matching (Noaidi)
by Paweł Świątkowski
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.
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:
The action part here is where code branches into two possible flows: when we perform
@post.save it is either
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):
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
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:
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:
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?
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:
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:
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
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.
The code after this refactoring is still far from being perfect.
save_postshould 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.
Last but not least, decoupling. Note that in
if [email protected]?snippet. But since I’m validating explicitly, I’m no longer bound to
ActiveModelvalidations. 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.