Yesterday, I asked Twitter:
Rails programmers: what's an example of one thing you find in other people's Rails
code that you (almost) always consider to be wrong?Quite a few people responded, and several asked if I’d post the aggregated answers. Here’s an attempt. I don’t necessarily agree with everything here. I’d love to hear more of these in the comments.
@briandoll’s response was particularly interesting:
extensive logic in views. If the erb would be ugly and harder to pull off in
haml, it's likely a code smellThis is something I always liked in Java about XMLC:http://www.enhydra.org/tech/xmlc/index.html. It was really hard to end up with a mess of co-mingling code and views, because you would have to work harder in XMLC to even make that possible. Amrita used to have that same characteristic, but I’m not sure it still does (or if it is even maintained).
I’ve been resistant to HAML but maybe it’s time to give it another look.
Also, as @mbleigh points out, any code in views is suspect. CSS should be in CSS files. Javascript should be in Javascript files.
Enough said.
@seancribbs singles out auto-generated tests. I’ll have to agree with him.
@alancfrancis says simply “rspec”. Certainly not a religious topic. I’m sure if you could somehow see in the code which editor someone used, we’d see a lot of TextMate, vi, and emacs in the responses.
@brendanbaldwin says “I’ve seen Polymorphic Associations and Single Table Inheritance abused to absurdity.”
As have I. I’ve also abused them to absurdity myself. The storage of a column called “type” which holds a class name is a pretty good indicator that something fishy is going on. It’s fishy but not always bad. I think, though, that any time you use it you should ask yourself more than once if it’s the right solution. Databases don’t do what they do best as well when you have lots of STI and polymorphic associations.
I once had to debug a problem with a commercial, Java-based portal product. The mail module was failing, saying it couldn’t connect to the mail server. In fact, what was happening was my login credentials were incorrect. Guess what the offending JSP looked like (pseudo-code)?
try {
// do everything here
} catch (Exception e) {
out.println("Unable to connect to the mail server") ;
}Exceptions are one of the only places in Ruby where you rely on checking the classes of objects to do useful and interesting things. Never rescue Exception unless it’s at the end of a stanza in which you’ve already handled one or more other exception types.
UPDATE: from the comments, Phil didn’t mean what I typed here:
That’s not actually what I meant. Never rescue Exception unless you’re working with code that abuses the
exception hierarchy. In normal circumstances you should be rescuing StandardError instead. If you rescue
Exception, you won’t even be able to ctrl-c your way out of your code(!) among other problemsMore info on his weblog here
class Potato
attr_accessor :size
end
potato = Potato.new
potato.instance_eval do
size = 123
end
p potato.size
# => nil
Ruby’s assignment to method-call syntax sugar only works when there’s an explicit receiver (such as self.size = 123)
Always run with -w. Fix any warnings you see. A good place to see them is when running your application’s comprehensive test suite.
RJS, render, nested named routes, and ERb are the targets of these criticisms.
validates_uniqueness_of is a strange critter. It’s so easy to use, that it’s tempting to slap it into your code and consider it good enough. But even the documentation for it warns you:
Using this validation method in conjunction with ActiveRecord::Base#save
does not guarantee the absence of duplicate record insertions, because
uniqueness checks on the application level are inherently prone to race
conditions.Here are some rules of thumb:
If you create your own configuration file and have to check RAILS_ENV, you’re not using a built-in facility of Rails.
OK, so this isn’t really what he meant. But it’s a good thing to think about. If you find yourself writing a lot of code to do something simple, you might be missing something Rails (or a plugin) provides. I’ve seen a whole lot of wheels re-invented inside models, controllers, and helpers. Part of Rails mastery is learning to ask yourself, “Shouldn’t somebody have already done this for me by now?”
Seems like if you find yourself manipulating Ruby’s load path in your Rails code, you’re doing something strange. If you don’t know you’re doing something strange, then you’re probably doing something wrong.
As Dave Thomas likes to say “Don’t not use positive logic” (or something like that).
Can someone tell me where the use of ”!!true” crept into the Ruby vernacular? Sure, it’s syntactically valid to do:
!!!!!!!!!!!!!!!!!!ok?
But why?
It’s April 1st, so my mind wanders to the hypothesis that someone introduced this and started spreading it as an elaborate joke. If so, well played. Is there a good use of this idiom I’m missing?
ActiveRecord migrations define a simple API/DSL for doing schema manipulation. But when they run, they’re just Ruby classes and methods executing within the Rails environment. So you can do anything you like in that context.
Sometimes people use migrations to load seed data. Pratik doesn’t like this.
I’m on the fence on this one. In small doses it doesn’t strike me as a horrible practice. That being said, I don’t tend to do it. I’d be interested in hearing opinions.
I guess it depends on what you’re doing with the parameters. Sometimes long parameter lists on GET requests are a sign that there are two actions’ worth of work happening in a single action. Or that you’re doing something that belongs behind a POST or PUT. This one doesn’t bother me as much as it does @Adkron.
if foo.save && bar.save
if bar fails to save, foo doesn’t. Oops. I’ve seen this more than once in the wild.
Obtrusive Javascript and RJS are the themes here.
What’s the worst use of RJS you’ve seen? I love RJS. I may be one of the offenders. Doesn’t feel like it (usually) though.
Smalltalk people, as a collective culture, seem to get this right. Kent Beck documents a simple pattern in his Smalltalk Best Practice Patterns called “Compose Method”. He says that one way to determine whether a new method is required is whether all of the code in a given method operates at the same level of abstraction. That’s a simple but powerful rule. Think about it next time you’re coding and I think you’ll agree.
I agree with Mike on this one. I don’t tend to write any comments, so I may be on the anti-comment side to a fault. My goal is always to write code that is clear enough that comments don’t add anything. If you name everything well and keep your classes, modules, and methods short, you’re already going to make comments redundant in a lot of cases.
Whenever I see a comment inside a method definition, I consider it a bad sign. In most cases, whenever you encounter a comment inside a method, it’s a marker for where code should be extracted into a new method.
If you’re writing a framework, the rules are different. But most of the time as a Rails developer, you shouldn’t be writing a framework.
Sorry, comments are closed for this article.
April 1st, 2009 at 11:52 PM
Interesting topic. I disagree with this:
“Try to avoid code that causes a database query (even through a shielded API) in views and helpers”
When using fragment caching as a significant tool in the performance war, lazy execution of queries (during the “rendering” portion of the request) is vital. In fact, I try to run as few queries as possible before the view for this reason.
That’s not to say we write SQL in the views. We generally instantiate presenter objects in the controller, which are called on in the view. If there is a warm cache, the views don’t ask them for information and they don’t run any queries.
-Bryan
April 1st, 2009 at 11:54 PM
Chad, I’m with you on RJS, and I’ve never quite understood its detractors—but perhaps that’s because the projects I’ve been on have always used it with restraint. I don’t think I’ve ever seen an RJS template longer than four lines, but I’d love to see examples of what people find objectionable.
April 1st, 2009 at 11:57 PM
`!!foo` is used to coerce the value of foo into a boolean. So if foo is 42, `!!foo` returns true, whereas if foo is nil, !!foo is false. I used it myself in methods that end in ?, because the convention is that method that end in ? are asking a question, so they should therefore return true or false, never 42 or nil. Returning 42 or nil from a method that ends in ? would usually work because you might use in the context of an if statement, like `save if valid?`, but it would still seem odd it at the irb prompt you typed `something.valid?` and got 42. Is there a better way to coerce an object to a boolean? Perhaps defining `to_b` on Object to be `!!self`?
April 1st, 2009 at 11:57 PM
That’s not actually what I meant. Never rescue Exception unless you’re working with code that abuses the exception hierarchy. In normal circumstances you should be rescuing StandardError instead. If you rescue Exception, you won’t even be able to ctrl-c your way out of your code(!) among other problems.
Everybody gets this wrong, which is probably indicative of poor naming choices within Ruby itself, but whatever.
More info at http://technomancy.us/114.
April 2nd, 2009 at 12:09 AM
Gosh, that sounds just like [every other web platform I’ve ever seen, used, or heard of]. :)
April 2nd, 2009 at 12:23 AM
Interesting list…thanks for compiling.
Regarding #15 (seed data in migrations)...we pull this out into a separate rake task (load seed data). This allows the seed data to adapt as the project adapts. If you have a lookup table with several sentinel values, then when you want to update that list, you need to be aware that you added them in a previous migration.
So, I pretty much never load seed data there. Another issue related to this is how to properly use the rake db tasks. I definitely include data transformations (which I always try to write in SQL) in my migrations. On a production site, you will need to migrate data (as opposed to the schema) from time to time to accomodate the changing schema. So, these guys can get embedded in the migrations as you move along. Once a migration hits production, for all intents and purposes we can throw it out (along with all other production-run migrations). After code/migrations hit production, you should be using db:schema:load to get you to the production schema, and then db:migrate to run the pending migrations in your development environment.
This is something I see messed up on many, many rails projects.
-John
April 2nd, 2009 at 12:39 AM
Greetings, I’m with Paul Barry on #14; I appreciate the flexibility of nil and false being false, and everything else being true, but sometimes I want to be explicit about the boolean nature of a method return.
I have zero problem with RJS; but again, virtually all RJS I’ve seen is 4-5 lines at the most.
1 is a special case of #10, but enough of a serious no-no that I’m okay with it being separate. (The project I’m being paid to clean up has SQL in views; fricken outsourced teams…) Not a fan of HAML, but it’s the thought that counts.
15 REALLY burns me. If it’s created in a migration, it’s lost when you clone_structure_to_test, so it’s not available for your tests. :( Further, it’s probably using a model in the migration, which is a MAJOR no-no to me (and shoule be another addition).
I’d add: A reliance on gems that aren’t config.gem’ed.
Fwiw, the project I’ve inherited did 13 of these 20, including zero tests except for auto-generated ones (unmaintained afterwards). (14/21 if you include my addition.) Seriously, why do people outsource development?!? ;)
— Morgan
April 2nd, 2009 at 12:40 AM
Good points, Bryan. I thought about that as I wrote this but didn’t mention it. Thanks for doing so :)
April 2nd, 2009 at 12:41 AM
Greetings,
WHOA! WTF?!? Sorry about that…! Can I add #22, a lack of ‘preview’ on blogs? Oh, wait, that’s not a Rails-ism…
— Morgan
April 2nd, 2009 at 01:39 AM
Double bangs got started because someone observed that they can be used to things to “real” booleans which might be useful in certain circumstances:
!!nil #=> false !!42 #=> true
April 2nd, 2009 at 05:08 AM
In response to Dave Thomas’ objection to “14. Obfuscated Logic”, here’s a good case for using a double-shebang:
def approved? !! approved_at # approved_at is a datetime end
Without the double-shebang, this method would return nil or a datetime object instead of a boolean, which could lead to unexpected results.
April 2nd, 2009 at 11:05 AM
5 (or more specifically, dealing with #5 in a legacy codebase) was the reason I wrote tattletale:
http://github.com/ggoodale/tattletale
Basically, it patches ruby’s standard raise and logs everything raised. It adds a few new assertions to Test::Unit so you can look for particular classes of exception being raised and handled, or not being raised at all, or whatever. It’s tiny and dumb, but very useful for finding out what’s being tossed about. (Try it with older versions of Rails – the sheer quantity of raising and catching going on is a bit frightening.)
April 2nd, 2009 at 11:26 AM
If you use spaces and not tabs, you’re officially silly. What possible reasons are there to use spaces over tabs? You want to control how your code appears in their editor? Not everyone likes 2-character indentations, and there is no reason for you to force it on them. cf. XHMTL/CSS vs Huge images and image maps.
April 2nd, 2009 at 02:12 PM
About using seed data in migrations, I think it’s Ok as long as you use SQL to load data into the database and don’t rely on models cause’ they will eventually change or be removed from the project. One might say that it is wrong and you shouldn’t use SQL directly but i disagree. Just the fact that you’re writing a migration, you’re working close enough with the schema and as long as you keep it it simple and ANSI compatible, INSERTS and UPDATES won’t cause you problem.
Talking about database, one thing that i really hate is the lack of consideration of database constraints and indexes that most rails developers suffers from. People rely only on the model validations and forget that indexes exists.
Another common pattern I see a lot is people over pushing stuff to the model. We have the fat-model-skinny-controller philosophy but some people doesn’t understand that you should be putting your business logic in the models but not the application control and workflow. The controllers shouldn’t contain business logic but they have a role, otherwise they would be vanished.
Interesting topic :) Cheers!
April 2nd, 2009 at 03:00 PM
What I’ve meant about the obtrusive thing is that is kinda evil using rails methods like link_to_remote, link_to_function and etc…because they return behavioral code (JS) inside the content code (html), which turns harder any further maintenance. The solution is making any DOM manipulation or JS stuffs from inside .js files or yet from inside .rjs files but never from inside erb/haml files.
April 2nd, 2009 at 09:53 PM
The thing about RJS and JavaScript in general I think most people misunderstand is that they think people should be unobtrusive because of some moral reason or because their app should work for those who turn JavaScript off. These are perfectly valid reasons too, but like testing it’s mostly a matter of good practice which in turn will make everything much easier for you in the long run. Keeping behaviour separate from content and layout results in de-coupled, modular, structured and reusable code that’s much easier to maintain and evolve. I’ve had to rescue a few apps where the initial requirements were simple, but as they grew so did the mess created by ad-hoc, inline JavaScript until nobody dared touch it for fear of breaking the entire site.
There are some problems that can be solved well enough with RJS and other inline or generated JavaScript, but they are extremely narrow in scope. Simply put, using inline JavaScript and CSS is like putting all your controller and model logic in the view; it just doesn’t scale.
April 2nd, 2009 at 10:42 PM
I love RJS, too. I actually think it’s one of the best features of Rails. And like Glenn and Morgan, I’ve never seen RJS files with more than 4-5 lines, so I don’t think it’s hard to maintain. The benefit to the end user is real: fewer HTTP round-trips to update multiple parts of the page than if you wrote JS with AJAX, and therefore better performance. Yet the programmer can achieve those fewer round-trips without breaking the modularity of the code.
Regarding obtrusive and unobtrusive JavaScript (which I’ll call OJS and UJS to avoid fingeritis): when I was first introduced to UJS I thought it was the coolest thing ever! But after using it I realized that:
(a) It hurts the user experience because event handlers get hooked up too late, so if you click before the page has loaded the browser will act as if you had JS turned off. For a real-life example that has bitten me as a user, go to http://www.dzone.com and click the login link in the top right corner of the page quickly, before the browser had had a chance to hook up the UJS.
(b) If you hand-code your HTML and JS then yes, UJS does make your code more maintainable. But ActionView abstracts things out for you so you don’t even have to maintain the OJS. That’s the nice thing about a framework like Rails—it helps you keep the code you see clean but can also generate code that improves the user experience.
But OJS does have some real benefits, such as letting Google follow your links without JavaScript but still letting the user use your cool JavaScript handlers. My solution is an extension I wrote to the link_to_remote helper function. I called my extension link_to_remote_with_seo and I describe it here (and I finally got around to publishing it today thanks to this discussion):
http://bmorearty.wordpress.com/2009/04/02/link-to-remote-with-seo/
April 3rd, 2009 at 08:31 PM
Wait, we’re not allowed to use RJS, render, or ERb now?
And what’s the best-practice alternative to polymorphic associations?
I’d appreciate a follow-up article or at least a comment explaining each of these things. If these are sure signs that a Rails developer sucks, why isn’t anyone talking about them?
April 4th, 2009 at 12:04 AM
Hey Chad,
Awesome timing ( for me ;) ) as I am going to be giving a talk at GoRuCo along these lines and will definitely be incorporating some of these ideas
Outline of some other relevant stuff I have outlined to discuss:April 4th, 2009 at 03:56 PM
It’s a great list and I find it very useful as someone who is learning the ropes of rails development. However, I was very intrigued by # 2. I agree that bad indentation is a no-no. However, I have been using tabs instead of spaces ever since I started programming. Is there a reason why it’s a no-no to use tabs? I am not married to tabs but I would like to know why spaces are preferred before I decide to change my ways.
I had a question on # 3 as well. I am currently learning about writing tests in Rails now. I think tests are necessary but the question, as always is that of balance. I am especially on the fence with Testing views. Is it because Test::Unit makes it look horrid or is it something that is going to be a challenge for someone’s behind irrespective of testing framework used?
I am also intrigued by # 8. I thought the Rails Framework was designed to abstract out common development tasks to enable developers to focus on the logic of their application. How come using features of Rails is a no-no? Perhaps I am not getting some kind of inside joke.
I understand that you may or may not agree with the opinions that you have crowd sourced here but I thought that you would be in a better position to explain the reasons why those who raised issues with the aforementioned practices are against it.
Thanks!
April 4th, 2009 at 04:08 PM
def approved? !! approved_at endvs.
def approved? approved_at != nil endwhich one is more readable?
April 4th, 2009 at 05:03 PM
@ste: What if you wanted to return false if approved_at = false? In your modified example, approved? would return true.
April 5th, 2009 at 07:03 AM
Bad log stragegies.
Bad logging means hours of searching in a production environment to that corner case you’ve forgot to implement. Especially if you use the default stragegy of Rails.
Rails logs all HTTP request including parameters (even worse when added a passwordfilter, because then it will throw a regexp on every logline, where’s my performance??) on INFO level???
How am I supposed to find something useful in the production logging if you have a site with a lot of traffic?
The rails logfiles are overwhelmed by HTTP requests so info logging with real business value almost becomes invisible, so the developer will not pay any attention to it.
April 5th, 2009 at 07:53 AM
I’ve been adding data in my migrations because it was, for me, the best place to add things you need when you run your app first, and I wanted to be sure they would never be reloaded afterwards (software being Typo open source blogware). Even though I don’t like the idea of using a model in my migration, I don’t consider it as a no-no and can’t find reasons to.
April 5th, 2009 at 12:56 PM
I don’t agree with 8 and 15 … For me RJS is awesome and much better to mantain instead of thousands of lines of JS and Jquery code in views or js files.
I totally disagree with ERB is a bad thing, ok haml is cool but i’m pretty sure in most of cases it impossible to use if your designers (that understand XHTML) create the templates. And to be realistic, erb performance doesn’t matter in most of cases.
And I agree Fréderic de Villamil, for me, the right place to create small amount of data in first run is on the migrations. Something like first role, or create admin user are some points that I think migration is a good place.
April 5th, 2009 at 01:04 PM
How about using “approved_at.blank?” instead of “approved_at.nil?” or ”!!approved_at” ?
What is the added value of the double-bang in comparison of using an existing Rails extension?
April 5th, 2009 at 02:33 PM
Chad, excellent list. I have encountered all of these errors (and more) in Rails code that has fallen into my lap in one way or another. I tend to find that the grossest offenders are in Rails code, not in “pure” Ruby code. It seems that to be a Rails developer, you only need to know just enough Ruby to be dangerous. Perhaps we as a community should do more to encourage that Rails developers become more proficient in Ruby.
That being said, I have seen some poor Ruby code as well, which brings me to one Ruby no-no that is almost never seen in Rails: abuse of eval. There is a a Ruby gem for XSPF that as of my last review of the code (a couple years ago) used eval in over 40 places of its 500 lines of code. It was used to set instance variables with dynamic names (never mind instance_variable_set) and in many other unnecessary and unsafe ways. It turned out that by setting the title of an mp3 to something like ‘screw you”; `rm -rf /`; ”’, you could run arbitrary Ruby code in the environment running the gem.
I immediately stopped using it and sent a rather strongly worded email to the author. I’m afraid I never heard back. I would say that this sort of thing is far, far worse than any of the offenders listed above. Bad Rails code might make your app slow or difficult to maintain or debug, but at least it rarely provides malicious users with such a gaping security hole to exploit.
As to rule #14, the bang-bang (!!foo) is a solution in search of a problem. In almost all cases, Ruby will evaluate an object’s “truthiness” in the way that you expect. If I were to define approved?, I would almost always define it s:
This is sufficient in Ruby because you should not expect the return value of a query method to be significant beyond its truthy or falsy characteristic. The truthiness of the approved_at object (nil or a Time object) is the same as the truthiness you expect from the approved? query method (true of false). The only time where I would define a query method more precisely is where a differentiation needs to be made between false and nil.
April 5th, 2009 at 03:03 PM
@Ben Smith and @Ashish Dixit and others interested in tabs vs. spaces, please see what Jamie Zawinski wrote on the subject many years ago and which I still think is true. As long as there is no standardized tab size (and it’s too late for that now), there is no way that tabbed source code will always look good when you open the file. And even when you adjust tab size it can be unreadable. I’ve worked with code where some methods were using 4-space tabs and others were using 8-space or 2-space. I didn’t feel silly at all for not contributing to that mess. The only way to avoid this is to Not Use Tabs, period.
As for `!!`, I find it a very useful idiom and one that should be promoted. It’s useful in Ruby in general but can really be a life-saver in Rails since boolean AR model attributes are special:
It can also hurt a lot when you try to hand an object a boolean value and they end up with data that you didn’t want them to see, e.g. when doing a custom XML response.
April 5th, 2009 at 03:07 PM
Oh well, I though you used Markdown. Here is a Pastie of what I was trying to show.
April 5th, 2009 at 04:00 PM
20 Chatty Comments -
Make a distinction between meaningless incomprehensive comments and useful comments that give the PURPOSE and ASSUMPTIONS made. Sometimes in a method a certain design choice may need JUSTIFICATION. The day when Ruby has such meta-programming capability so the purpose, assumptions and justifications are obvious, I will also say that explicit comments are not needed any more. Until then, most code is insufficient documented.
April 5th, 2009 at 04:09 PM
Sorry for large text in my previous reply – copy and paste gave the large size and it seems impossible to edit the commnent.
April 6th, 2009 at 10:48 PM
The mind reels. Don’t not use positive english.
April 7th, 2009 at 07:43 PM
I’ve used migrations to populate models that are more or less lookup tables. E.G. Race values for an employee database. It’s not a big deal for testing beacause I don’t hit the db when using those associated models in a spec.
I usually use SQL in this case, but what is the preferred method. It seems obvious you want the data entry for this type of model to be scripted. What makes migrations bad in this case?
It’s preferable to run migrations, and then another script to load the data?
April 9th, 2009 at 07:04 AM
So what exactly is “wrong” with tabs instead of spaces, besides going against the convention? I don’t like the convention, I like using tabs, it gives me better indentation in my opinion, and when I refactor code, tabs are easier to get rid of or add.
Also, I think chatty comments are definitely better than no comments. Sure, you should write clear code, and in ruby, it’s much easier to make what you’re doing clear, and perhaps for rails development it isn’t essential to have comments all the time, since the rails way is so well known to everyone. But if you’re breaking rails conventions or doing a general application in ruby (or any other language for that matter), not writing comments will kill the maintainability of the code.
April 9th, 2009 at 04:31 PM
I do not see a compelling reason to use the double-bang!! All values in Ruby can be evaluated to true of false WITHOUT the need of coercion. If you don’t want to expose an object via a return statement then use blank? or nil?.
April 11th, 2009 at 04:31 PM Woah, its a RJS lovefest in here. I do not like leaning on RJS and here are my reasons why:
In my current app we use a Page/Controller-specific javascript api. We limit our XHR response to one javascript function call. This can be refactored to be purely JSON, but we have not had a need to do this yet.
We also are using unobtrusive JS. This means we can more heavily use Webrat (not the Selenium mode) and Screw Unit to do the bulk of our testing. We still have Selenium tests, but we don’t need to rely on Selenium as much as we would need to if we were using RJS.
Also, we can support browsers w/o javascript pretty much for free. Given our patterns, I have absolutely no desire to use RJS.
We actually tried RJS, and it quickly started to become a confusing mess. We did not want to maintain that code, so we strove for something better.
I’m going to blog about our little framework once it solidifies.
April 22nd, 2009 at 01:39 AM
Great discussion. The fact that the questions and answers fall on April 1st makes it even more enjoyable to read.
April 24th, 2009 at 09:23 AM
Whoa! Just too much religion in here!!
There’s a strange notion within the Rails community that the best practice is to work AGAINST the framework. The UJS zealots is a pretty good example of this…
I’m totally down with @Brian Morearty on the RJS. It’s one of the best things with Rails. I’m working on a huge app and we’re using link_to_function, link_to_remote, remote_form* and RJS templates
- and I’ve never found it hard to maintain -on the contrary!And no, you’re not supposed to put humongous chunks of render :update blocks in your controller. Maybe it’s just that people don’t know how to use RJS templates properly?
And about the unobtrusiveness, I don’t care if can’t use the site with Lynx!
April 24th, 2009 at 01:21 PM
I still believe that ”...and I’ve never found it hard to maintain [RJS templates]”. The strike through is just because I was unaware of the textile syntax… :)