I’ve been extremely frustrated by existing Rails projects that get handed to Pathfinder for fixing. Almost invariably they’re filled with some of the worst Rails code I’ve ever seen. Happily we have amazing Rails developers on staff, so we can make great, usable changes to even the most obfuscated code; but that doesn’t mean we have to write happy blog posts about how great that code is. No sir, I’m here to tell you how it sucks.
A lot of people have spent a lot of time writing about Rails best practices. If you need some good examples, you need look no further than Noel’s previous blog posts (or, to be completely immodest, my own). This is not a post about best practices. This is about the worst of the worst, the things you should never, ever do, the stuff of coding nightmares. So before we start, make special note: this isn’t a blog post to make your code better. This is triage. We’re only trying to stop things from getting worse.
NEVER Override Existing Methods
Never, ever do this. Never override an existing method. Period, full stop, no excuse. Even if you think your implementation is amazingly great and will save the world, unless it completely mimics existing functionality (AND HAS TESTS TO PROVE IT) you are going to make some coder somewhere yearn for death. Imagine the first time they try to use String#empty? only to find you’ve changed it to return always true, or something equally bizarre. Tears everywhere.
But obviously you’ve read on. So, if, somehow, you find you are compelled to do this, at least put the resulting method in a very clear place and document it extensively. Do not put it in a plugin out in the middle of nowhere so that when the code disastrously fails you have to pore through a horrifically winding backtrace to find the problem.
Under no circumstances should you ever override a method called in ActionController::Rescue or ActionView::TemplateError.
I discovered this because someone did exactly that. I recently had a project that for some reason had no error backtraces at all. Apparently the friendly, helpful Rails error pages completely disappear when something fails in ActionView::TemplateError, leading to hours of fun and laughter until I finally found that the original coders had an Enumerable#sum implementation in a plugin.
I’m still not clear on why they felt it necessary to override Enumerable#sum. There’s already a perfectly good implementation of it. And the way that they did it caused Rails to break. Have we learned the lesson here? In case we haven’t, do NOT override existing methods out in the middle of nowhere that break everything. In fact, do NOT override existing methods at all. If your additional functionality is so important, make a new method for it and give it an easy-to-remember name.
NEVER Rescue Everything
Rescue_from has been in Rails core for about a year now. There’s very little reason to have rescues in your individual controller actions. But I can see some edge cases where maybe you want to rescue stuff specifically in actions but don’t want it applying to the whole controller.
What you should NEVER do is have a rescue that isn’t attached to an individual exception, so that it just catches all exceptions. Exceptions are our friends. If you catch all exceptions, then plugins like Exception Notifier (or the very sexy HopToad) never get called. Back traces are never generated. The whole scaffolding of error handling and finding collapses and you’re left with an awful mess that fails inconsistently and you have no idea why.
But that’s just production. In development, anonymous rescues disguise errors that you may have introduced by making tests appear to pass or actions appear to work. Only later when you’re trying to figure out why your code is not acting in the way you expected will you discover the tricky, nasty rescue.
Rescuing everything will just lead to pain, even though in the short-term it seems to make your code work better. Decide what exceptions you’re reasonably expecting and rescue those. Then when strange problems crop up in your code, you’ll be able to address them directly.
NEVER Put Validations in the Controller!!
Man do I ever see this one a lot. I have no idea why either. Models have helper methods for validations! They’re so easy and quick to use. There’s absolutely no excuse for validating anything in a controller.
If Your Controller is Over 1000 Lines Long, You Did Something Wrong
I don’t like making blanket statements, and I hate imposing hard limits, but I hate stupid controllers even more. If you have a controller that is over 1000 lines long, you messed up somewhere along the way. If you have 100 actions in that controller, then you should have 10 separate controllers, not 1 gigantic one. If you have 5 actions in your controller, you should seriously reconsider what goes into each of those actions.
In fact, let’s generalize this one and the previous one out a little further…
Chubby Controllers Must Die
There should be no business logic in a controller.
Read that again.
There should be no business logic in a controller.
Controllers do two things: they take data from the params or session and send it to the model. The MODEL performs all the necessary logic. Then, the controller does the other thing that’s completely necessary: it decides what should be shown to the user. That’s it. The sum total of a controller action is two steps long.
- Send information to the model.
- Decide what to display.
If you are doing ANYTHING ELSE in your action, you are doing it in the wrong place. The end.
NEVER Assume You Don’t Have Nil
Maybe you assume that your ActiveRecord object has an association on it, like address. So you innocently type user.address.telephone and blam! Your application blows up because address was, in fact, nil.
Lesson: if you perform a method on nil, you get NoMethodError. That is bad. Never call a method on nil.
NoMethodError is easily the most common error I see in any Rails application (followed by ActiveRecord::RecordNotFound as a distant second). This is tragic because it is one of the easiest exceptions to prevent. If at any time you might have an object that, for even a moment, you might consider is nil, you must instantly follow these two simple steps:
- Download Andand
- Use object.andand.method INSTEAD OF object.method
There! Application fixed. So easy.
NEVER Copy and Paste Huge Swathes of Code in a View
Or, rather, do copy and paste that huge swath of code: copy and paste it right into an easily-accessible helper. Now, when someone comes along after you and has to change every instance of that code, they can do it in one place, rather than track down every crazy nook and cranny that you hid the view code in, ready to pop out and surprise us with template errors.
ESPECIALLY do this if the code is almost exactly the same but slightly different in two places. Helpers are incredibly handy if they’re used correctly, so use them correctly.
NEVER Copy and Paste Huge Swathes of CSS
Alright, this isn’t Rails-specific, but if I have to pore through a six million line CSS file for all 10 instances of a class that’s exactly the same but named differently because you fundamentally misunderstood how to use CSS, then… well… I guess I’ll get to keep my job since people are making so much work for me.
Fine, fine… But you, since you’re reading this and don’t want to fall into these pits. You should avoid doing this.
DON’T Defy Convention
Rails is very stable and functional. Imagine it like a big fortress made of good code. Its foundation are its conventions, the assumptions Rails wants you to adopt. If you start changing those assumptions willy-nilly, you end up with a house of cards instead of a fortress: the smallest changes can cause the entire thing to come tumbling down.
Rails does not have many conventions, but those that it has you should follow as if they were dictates from your Deity of Choice. Tables are named for the plural of the model. The database is named <application>_<environment>. Model instance variables should be named the same as the model — @user (or @users if plural) for the User model. The controller for that model should be named <plural model>_controller.
If you have a legacy database and you have no control over your table names or columns then you have an out. But if you’re starting a Rails project from scratch there’s no reason to defy convention. Do so at your peril.
This should probably be higher since it’s one of the most annoying things about picking up an existing codebase. A lot of the code we get isn’t well tested, or even tested at all. Untested code is very very difficult to alter: you’re not sure if the small change you’re making will break functionality out in the middle of nowhere, and you have no way of testing it without following through the application flow every time you make a change.
Doing that is a huge waste of time, believe me, and lack of tests make it utterly necessary.
If you haven’t tested your code you aren’t done writing it, and you should certainly be embarrassed and ashamed of releasing it anywhere without corresponding tests.
Check ActiveSupport (and other Rails/Ruby classes) FIRST
I see a ton of methods that already exist in ActiveSupport that are hand-coded from scratch. Stuff like Time#beginning_of_week and pluralize pops right to mind. Also there are a lot of people who do scratch versions of time_ago_in_words and number_to_currency (two of the most helpful ActionView Helpers). Even worse are seeing core pieces of Ruby functionality recoded, like that Enumerable#sum I was talking about earlier.
I don’t have every method in Ruby and Rails memorized. But if I think something is a common use case, I’ll Google it before I start coding it up myself. Even if I don’t find a Ruby or Rails version, nine times out of ten someone else has tackled the exact same problem and you can just appropriate their code, if they’ve allowed it to be copied. Just remember to attribute the code to its original author!
DON’T Abuse Rest
REST is a good philosophy for a certain subset of applications: those that do CRUD operations all the time, and CRUD operations almost exclusively. Adding in a search operation is generally okay too. But REST starts to break down if you add in a whole lot of extra actions in your controller, particularly if these actions fit very poorly into the CRUD hierarchy.
If you are going to be resorting your collection and changing statuses of individual members, and having a lot of eeny-weeny little actions for all your Ajax stuff, and perhaps throwing in the kitchen sink, you should reconsider whether you want to use REST. Most of these little actions won’t ever see the use of more than one of the HTTP verbs, so the savings you’ll enjoy from REST are precisely zero. And your routes.rb will be a lot more complicated.
If your application is going to be doing something non-standard, just don’t bother with REST. Save yourself (and everyone else) a little bit of time.
NEVER Have Data-Only Migrations, or, Worse, Migrations that Change Existing Data
The last one in the list, and boy, is it a doozy.
Migrations are the wrong place for your data. Migrations, in fact, should be deleted every now and again and replaced with the schema.rb. If you do that but you had data in your migration, where are you then?
Screwed, that’s where.
Even if you don’t occasionally clear out migrations, if you change your default data you’re going to have to drop and recreate the database to insert the changes into it. That’s problematic. No, the clear place to put data is a Rake task, or maybe a yml file attached to a Rake task. Then you’ll be able to easily run it from the command line, and you can even put it into a migration if you feel you absolutely have to. (Though again it would be a good idea to keep it separate.)
Even worse than this is having a migration that only changes data already in the database. If that data isn’t loaded in, the migration fails. If the data is already changed, the migration fails. Never do this and your applications will be the better for it.
You probably won’t be surprised to learn that this list ballooned significantly since I initially wrote a draft of this post. There are just so many great worst practices out there it’s hard to codify them all. If you’re committing any of these sins, do not despair. It is usually simple to fix them: I think most of these (okay, except the CSS one) qualify as low-hanging fruit for code corrections.
You might not see any value today by avoiding these worst practices. The value you get is when you return to your code later, or are forced to modify it. Then you’ll be glad that you did things in a sensible manner.
But even if you didn’t do things in a sensible manner, after reading this post, at least you’ll be glad you didn’t do things in the worst way possible.