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.
-
ALWAYS Test
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.
A couple thoughts:
First, I still have some (admittedly unused) code that dates from pre 1.0 rails days. A lot of what is now common core functionality had to be written by developers back then I believe Enumerable#sum is one of those as was #inject. Still, I feel you pain as I’m porting to merb/datamapper.
Second, re:CSS. I agree. I would add that if you CSS has 1000s of selectors, there is a better way to structure your document. Step one use the class property! () jQuery and Sass will be happier as well.
You’re off-base on rule 13. While geared towards schema migrations, there isn’t any valid reason that I know of that they can’t be data migrations.
Lets take your reasonings one by one:
“If you do that but you had data in your migration, where are you then?”: I personally don’t agree with using migrations to load default data. The alternative is to keep a file (or set of files) that have all of your default data. There isn’t any reason that these files could serve the same role that schema.rb does in relation to migrations; That is, when you compress you migrations, all of the new default data get moved into those data files.
“if you change your default data you’re going to have to drop and recreate the database to insert the changes into it.”: Migrations shouldn’t be expressed as a full representation of the data, but rather as a set of operations to perform on a database to move it to the new desired state. I could write all of my schema migrations containing only “create_table :force => true” type operations, but I would be missing the point.
“If that data isn’t loaded in, the migration fails”: The same can be said for schema migrations. Try removing a column from a non-existent table and watch it error out. You always take risks in assuming the state of your DB, schema included, but you can still do so responsibly. For schema changes, you adopt the policy of not changing the schema outside of migrations. For data migrations, I tend to write them as simple transformations that assume as little as possible about the actual content outside of what gets enforced by the applications validations.
“If the data is already changed, the migration fails”: Also applies to schema changes. In fact, this is _easier_ to avoid in data migrations, since writing idempotent transformations is, in my experience, simple.
Good post. I’m glad I’m not the only one who is guilty of breaking rule 13! I was starting to feel rather ashamed but the more I think about it the more I think there is a case for using it.
I think rule 13 is acceptable as long as you are using it on what I would term “static data”. That is data which is never changed by the end user (and certainly not data input by the end user) but is used by your application to perhaps populate a drop down box or something similar.
I work in a University which is divided into faculties. Now these faculties are pretty constant but once in a while they change name slightly and I don’t want to be unloading and reloading the faculties table with the new names because I’ve got live data in the production system so instead I use migrations.
Now scenarios like the above I personally think are OK. However you raise a good point about using YML files and rake to populate the defaults and that’s something I think I’ll also do now so that anyone setting up a dev server from scratch can have the correct values in there without having to trawl through every migration.
In summary I think, if used correctly on static data, rule 13 is acceptable as long as you have the YML files and rake tasks to build the data from scratch without performing every migration. So the migrations are used purely for migrating existing databases where the data is referenced and cannot be deleted.
Interesting and well thought out list. After about a solid year of Rails development, I think I may agree with all of your rules except for #13. A lot of the sites I work on deal with a static set of data, mostly geographic data – data that will never change. The data is prepared and generated once, then dumped into an SQL file, committed into the repo and executed within a migration. I see no better way for everyone to sync their working copies, along with the production DB, against new static data. Anyway, a good read!
While I agree with most of this, I disagree with #6. It contradicts the never rescue everything point raised first. If you expect not to have a nil but you do have a nil there is a reason and that is probably not an esoteric mystery. It’s probably that you have bad data because you didn’t validate input or that you have bad logic that results in your expectation isn’t met. andand and try seem to gloss this over. Let’s fix the root cause and not lower our expectations.
If you think REST is limited to applications that exclusively do “CRUD”-style operations … I think you misunderstand REST. There’s nothing that says a resource must support all the HTTP methods, or that hypertext can’t direct a client to to something decidedly un-CRUD-y. Perhaps you can expand on this point, a bit? (Honestly curious what you mean.
Thanks for the positive commentary!
Scott, I think that you and I are actually agreeing on a lot of these points… at least, that’s the impression I get from your comments. You say you don’t agree with using migrations to load default data: that’s pretty well what I’m saying here. Put them in a separate file. Load them from migrations with rake tasks.
Your point that migrations should represent deltas is completely correct. You should never go back and change existing migrations. But that’s one of the two options available to you when you have your data in your migrations: and opposed to the other one, it’s the only way you can be 100% sure that the data is inserted correctly without failure. But I think you can see why this is a really bad idea.
The other option is having a migration only to change data existing in the database. This makes two huge assumptions that may prove unfounded: that the data exists and that it hasn’t been changed already. If you’re on a project with a lot of people, inserting such a dubious migration into the application is a sure way to make lots of exciting new enemies.
I’m actually surprised I’m getting a lot of pushback about #13, and you guys do have some really good points about it. Consider this, then: let’s say you have a model Faculty that has and belongs to many Teachers (to shamelessly steal part of your example, Anton). You decide to insert a join model between them called Tenures, and you drop the old faculties_teachers table. You suddenly lose all your associations between Faculty and Teachers. So, before you do this, you have to prepopulate the Tenures table with the appropriate faculty_id and teacher_id. Now the static data in your migration no longer represents the actual state of your data.
If you had a rake task to generate Faculties, Teachers, and the relationship between them, though, it’s unlikely anything at all would change in that task: it would still be able to do faculty.teachers << teacher so nothing would change.
I guess my point is that static data tends not to be as static as one thinks. If you have data that is literally totally immutable, then go ahead and put it in a migration. But I’ve come across very little that is literally totally immutable, and by putting data in a migration you risk the data integrity of your project. Rake tasks that run after migrations are a lot safer, more repeatable, and require less work on your part.
So that’s the reasoning behind #13.
You’re correct in some cases with #6, however many times you’re just putting off the problem. This is violating the pragmatic concept of crashing early. As ActsAsFlinn says, it could be because of bad data or something like that. Just sticking andand everywhere will mask problems and they will creep up elsewhere. Before sticking andand somewhere you should think to yourself, “should this value ever be nil?” and if not, do not stick an andand there. Use andand in cases where nil is a valid value.
Thanks for the interesting points. Re: point 12:
“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.”
I think you’re a little misguided as to what REST actually is. If you’re going to be doing something non-standard, just what exactly should you be bothering with? Wait.. a minute, I guess if you’re doing something non-standard you’re probably not doing REST. Yes, of course!
@Josh – I’m not sure I follow the problem with your example exactly. If you are using object IDs in your migrations then I agree you are going to get yourself into massive trouble. Is that essentially what you are driving at?
In the environment I work with I couldn’t do that anyway – I develop on MySQL but deploy to Oracle. The primary keys that Rails sets up on Oracle start at 1001 whereas on MySQL they start at 1. So right off the bat you couldn’t even have migrations which used primary keys/object IDs.
So I’d always be doing :
Faculty.find_by_code(‘SBS’).update_attribute(:name, ‘Southampton Business School’)
…using a column (in this instance called code) which I know will never change, rather than…
Faculty.find(1001).update_attribute(:name, ‘Southampton Business School’)
…which is decidedly more risky and I wholeheartedly agree is a disaster waiting to happen.
Overall I think my view on what is acceptable/workable and what isn’t is influenced by the fact I’m usually the sole developer working on projects and I rule the database with an iron fist so (fingers crossed) I don’t get any surprises.
If I was part of a large team where a lot of people could be interfering then perhaps my outlook on migrations would be different and my use of them would change. Still, it’s useful to be aware of the potential pitfalls.
Correction : Rails sets up a sequence on Oracle for primary keys which starts at 10000, not 1001.
Regarding migrations, I would say that while you’re right that those are significant pain-points, I don’t think I’d go so far as to say that you should never do those things. It’s just that if you do those things, you MUST add error handling code to the migration, and anticipate each of the potential points of failure. That’s extra work of course, but I suspect that there are plenty of cases where your proposed solution ends up being even more work still. It’s one of those cases where it really pays to know what you’re doing.
As an addendum to my above comment, when you get into tricky stuff, it pays to build migrations that are liberal in what they accept and conservative in what they produce. Whatever the expected post-migration state of the database is for the normal case, should also be the expected post-migration state of the database for the exceptional case. And if and only if you cannot possibly produce that expected state should your migration fail.
A thousand-line controller? Are you kidding me? A 100 LOC controller is too long.
[...] Pathfinder Development » Rails Worst Practices: 13 Coding Nightmares You Should Avoid – [...]
I posted my comments here http://jroller.com/dscataglini/entry/rails_worst_practices_blanket_statements
You can reduce rule 7 and 8 into one:
NEVER USE COPY & PASTE!!!!
It shows laziness and lack of abstraction. Is a maintenance nightmare waiting to happen…
Greetings,
I know this is a week old (and I’m loquacious, sorry), but I wanted to add a note about rule #13. I have strong opinions about this one, having come off a project which did it a LOT, and having to be ‘that guy’ who tried to encourage our new developers to avoid it.
I put migrations into three categories. Structural changes (add/remove tables/columns, etc.), static data (a zip code to lat/long conversion table), and data (static or dynamic) changes (populate a new join table between users and topics).
Structural changes are what migrations are great at. Use it and prosper. (schema.rb is unfortunately usually shite in a big team in my experience, mainly due to conflicts, and its ever-changing nature.)
For static data, I have not yet found a good solution for bulk loading large amounts of static data into a newly created database, especially a test database (fixtures don’t cut it for this stuff). Rails just isn’t built for that, and it’s frustrating sometimes. You end up in twisty little maze of implementations, all different.
Data changes are trouble, and I recommend against them as well. You have two basic choices, either use the model (slow and harms refactorability) or direct SQL which bypasses your validations, so can lead to subtle data errors.
*I* feel #14 should be NEVER use models in your migrations. If you find yourself tempted to use models in your migrations, you have clear evidence that you need a rake task. A one-off rake task, perhaps, but a rake task.
I’ve probably had a dozen models that I’ve refactored away over the years that broke somebody’s migration, and the only option was to go back and change their migration, which feels dirty for a reason.
That said, I also believe that a fresh development database should always be creatable from the migrations and that you should set up CC.rb to do a migrate to create its databases.
A great back and forth related to this is on this very site at: http://www.pathf.com/blogs/2008/07/resolved-should-schemarb-be-included-in-your-source-control/
– Morgan
Nice post. But don’t you think that you are are too dogmatic regarding some rules?
I must agree with those people arguing against #13.
One thing I will NEVER do is re-run a migration. A migration is a run-once and once only script (per database) and therefore I have no problem with it updating any of my database state whether schema or data.
Sometimes changing a schema requires setting data to a value such as adding a column and setting a ‘default’ value based on existing data. A migration allows that to be done inline so that migrations can run one after each other without having to stop to run a rake task.
To clarify my run-once statement, once a database is in production, there is no way I’m dropping and re-creating with migrations. It’s live and must stay live so backups become the order of the day there.
[...] Rails Worst Practices: 13 Coding Nightmares You Should Avoid Ostensibly for Rails, but most of these apply to anyone using an MVC framework, and are worth a look. (tags: programming webdev tips) [...]
Great post, I agree with pretty much everything, but there’s one part of #13 that no-one hear has touched on yet: testing.
I need to write a data change migration (splitting data from one table into another table, and removing redundant columns). Given that the product is in production now, I must have a test that proves my migration works. So, how do I test my migration? I need to pull the database up to the point prior to the migration, setup some data, run the migration, then see that the results are correct. This requires that all previous migrations run fine, which is currently not the case (some old pure-data migrations are of course failing hard).
I’ve been searching, talking with co-workers, I just don’t know what the solution to this is. I’ve personally always had the mindset that all migrations should be up- and down-able for this reason. Any one have suggestions here?