The biggest Rails code smell you should avoid to keep your app healthy
Ruby on Rails shines with creating prototypes and solutions which can be quickly shown to your clients. Rails speed is often crucial on the phase where you are building a bond with your client — if client sees effects fast and can provide feedback it usually improves overall experience. It’s a very crucial phase, so having a technology on your side is a great benefit which Rails provides to you.
As nearly everything in this world, quick prototyping benefit comes with a cost — and this cost is a nasty one, because it needs to be paid shortly after you enter production phase. Priorities shift from implementing features fast to keep the application in good shape. And by keeping the app in a good shape I mean — avoid regressions, provide basic monitoring and focus on keeping client’s income over everything else. You need to become a responsible developer. This is a hard challenge so we wrote a book about this particular topic.
In fact, many of those regressions, bugs and dangers to system stability and/or maintainability can be avoided in the earlier, prototype phase. By identifying code smells on the prototype phase and not introducing them you can avoid many regressions and emergencies during production phase. Those are looking innocent on the first look, but are proven through many years of maintaining production app to be a real problem.
As an introduction to this topic I had many choices — which code smell is the worst? Which is valuable to be described? There is a big list of those smells, but finally there was a clear winner for me. I’ll try to focus on showing you why it’s bad and how to fix it.
And the winner are… ActiveRecord callbacks!
This will be a controversial one, since AR callbacks are widely used in most of Ruby on Rails codebases. But in fact, most of them can be avoided or eliminated to avoid implicitness and keep maintainability intact.
The problem with callbacks is that they’re destroying the linear flow of your code. Without callbacks you can inspect the flow by starting on the controller phase, then going to model definitions and inspecting method implementations and then ending on the last call of the method in a code. With callbacks you need to check when they are called (since they can be conditional which introduces another maintainability challenge to keep them up to date) then jump to their definitions, understand in which order they’re evaluated (while callbacks are bad, coupled together callbacks are far worse), then jump to your method definition again…
AR callbacks are painful if you’d like to extract a piece of logic from ActiveRecord to a separate object, which is a common safe step in our refactoring process. Those are also a big obstacle if you decide to split AR model into smaller ones — they just need to be rewritten in a non-trivial way.
The general rule of keeping maintainable apps is: favour explicitness over implicitness. AR callbacks are an anti-thesis of this. The advantage of DRYing up your code using them is not worth it — not to mention it is not what DRY is about, really.
And the fact is, they can be easily avoided. Let’s take an example model with some callbacks in it:
Let’s see controller’s code (from scaffold):
From the controller’s side it is not clear that after Customer without auto_created credentials is created an e-mail is sent. There is a conditional hidden on the model level, so this way SRP of this controller is broken.
A little better solution is to get rid of callbacks and introduce two class methods — register and auto_create:
The auto_registered? method is gone, since it is not used anymore by this code. The conditional logic is no more, so the code here is a little bit simpler.
Let’s see how controller should be changed:
The conditional logic is moved up to the application layer. This is generally a good advice to keep your conditionals the closest to the boundary as possible. In Rails this natural boundary is HTTP protocol, so a controller.
This conditional in fact can be totally eliminated by introducing #register and #auto_create POST actions, or moving this even higher to the routing constraint (as described in Fearless Refactoring book) and select a method there.
This way branching logic is only on the top level of your code, and the flow is totally linear. It is even better to extract a service object and move the mailer logic to it — this way it is even simpler to think about this code, simplifying the controller logic further.
Summary
Choosing the winner in terms of code smells was a big challenge for me — there are lots of them, so I’ve been forced to choose wisely ;). But I believe since AR callbacks popularity it is the most common smell I see and it’s a rather nasty one. I hope after reading this article you’ll reconsider using the ActiveRecord callback — especially that often it is easy to avoid them with many techniques.
If you’ve liked what you read, let me know. There is plenty of code smells in Rails (and not only in Rails!) that are worth highlighting and raising awareness about them. If you want to prove me I’m wrong and callbacks are the best, don’t hesitate to discuss with me — I’m happy to discuss about this particular issue.
Thank you for reading — I’m available on Twitter if you want to engage me. If you like this subject, you can be also interested by Arkency blog. We write a lot about maintaining old, legacy applications, remote & async work, being a responsible developer and more!
