5 ms·

10 Most Common Rails Mistakes

58 points by djug 12y ago

alphadevx 12y ago
Not a bad list, but I think many of those are not unique to Rails and just look like common anti-patterns.
deleted 12y ago
[deleted]
3pt14159 12y ago
Personally, I wouldn't make current_user return "guest". That would be surprising. I would just return nil. There are lots of places in an application that you'll want to conditionally show logic, like whether or not to show a "reply" button, and it is more natural to say "if current_user" than "if current_user.name != 'Guest'".
- taylorfausak 12y ago
  I would much rather have it return a null object instead of nil. You could make it better by adding a #guest? method to your User class. if current_user.guest? Although I don't like how the example given returns an OpenStruct instead of a NullUser, for instance.
- jstin 12y ago
  I've been using the CanCan(Can) gem to handle all permissions logic, both in controllers and views. It abstracts all user state logic to a single file https://github.com/CanCanCommunity/cancancan https://github.com/CanCanCommunity/cancancan I find this pattern to be the most manageable especially in large projects. But I agree with your point. "if current_user" is miles better than "if current_user.name != 'Guest'". Edited to add: As far as this articles use of current_user.name being guest, it makes sense to use a presenter to display the data in the view. Conditionally displaying a user name or 'Guest' in a view is kinda gross. Again I'll suggest a gem for this pattern https://github.com/drapergem/draper https://github.com/drapergem/draper All objects coming into the view should be wrapped, especially and ActiveRecord objects.
  - michaelmior 12y ago
    Draper is great in general for keeping display logic away from controllers and views (as per mistakes 1 and 2).
- rubiquity 12y ago
  nil is annoying and should be avoided when possible. nil gives no useful feedback when blowing up and communicates nothing about the intent of the code. Using a Guest object is a great use of polymorphism and duck typing, which are great features of Ruby (and object orientation). Having nils all over the place and having to do `if some_obj` all of the time is messy. Your example is pretty simple, so my solution will sound like overkill, but I would use some other type of object such as a Presenter to handle the rendering logic.
  - 3pt14159 12y ago
    Naw, you're complicating things. Nil should not be avoided, it should be accepted. If there isn't a current user then don't pretend that there is. Anywhere you are doing `if some_obj` in view code you'll probably be doing `if some_obj.is_a? Guest` or `if some_obj.guest?` anyway. I've worked on very large and well used applications (billions of loads, multiple data centers, hundreds of thousands of lines of code), not that that should really matter to the argument (logical fallacy and all that), but the thing that I've found messes up Rails applications is large model objects ("god objects"), poorly thought out data relationships, and inconsistent assumptions across systems that lead to hacks. Not nils.
    - rubiquity 12y ago
      > Anywhere you are doing `if some_obj` in view code you'll probably be doing `if some_obj.is_a? Guest` or `if some_obj.guest?` anyway. Sorry, you're completely wrong. Polymorphism and duck typing exist specifically so that you don't have to do `is_a?` checks. > but the thing that I've found messes up Rails applications is large model objects ("god objects"), How do you think god object models get created in the first place? By not extracting out logic. When you have models that have very different state, you actually have very different objects. Those differences should be extracted into different classes.
  - Finbarr 12y ago
    Agree that `if some_obj` all the time is annoying. Instead, better to introduce semantic helper methods like `if signed_in?` (which incidentally is included with gems like Devise).
- awj 12y ago
  As taylorfusak mentioned, a User#guest? method is a more natural way to handle that problem. You also could make use of ActiveModel::Conversion#to_partial_path, like so: <%= render partial: current_user %> This implicitly calls to_partial_path on the current user object and looks up a template to render based on that result. So the user model would return "users/user" and your guest object could return "users/guest". The problem with using nil here is that you have to remember to check it everywhere, all of your tests have to exercise both the nil and not-nil cases, and most of the "else" clauses are just going to substitute default values anyways. In my opinion if your views know so much about your user model that substituting a "guest" object is hard, maybe it's a sign you need to pull some logic back out of your views.
  - 3pt14159 12y ago
    I disagree. Generally you will have sections of your app that are only for logged in users, so your ACL logic in your controller handily takes care of that, while still giving you the convenience of being able to call current_user and know that you actually have a user, while still being able to say things like `if current_user` where you need it. I do like your render partial example, but in most cases you will want to be doing something like showing a like button if the user is signed in, in which case you can do: <%= render partial: current_user %> And if the current_user is nil the partial will just be skipped.
    - awj 12y ago
      I'm not sure I understand your point. If your controller logic ensures that users will always be logged in, why would you need to test for the presence of current_user?
      - 3pt14159 12y ago
        Sometimes we want to just say current_user.username
- logicallee 12y ago
  This is just responding to your specific example: >There are lots of places in an application that you'll want to conditionally show logic, like whether or not to show a "reply" button, and it is more natural to say "if current_user" than "if current_user.name != 'Guest'". "if current_user.name != 'Guest'" is way more natural (not saying that is good or bad.) I read that as "if the current user is 'Guest'" whereas the former reads, "if there is a current user". There is obviously a user. When isn't there one? Don't read too much into this, I am just speaking about what looks natural in your specific comment.
  - mercer 12y ago
    I think what partly complicates this is our 'semantic'(?) understanding of a 'user' in the context of a Rails app, where it might make more (or less) sense to return nil, but where this is not immediately obvious. If instead you'd have a car object in the context of an app that sells junkyard items, it would make sense to return '0' instead of 'nil' when it concerns wheels (wow, this might be a terrible example...). It seems to me that using too much of this implicit 'semantic thinking' is to be avoided, especially when the semantic meaning is not entirely clear or always agreed upon, like with user objects. And if you look at this that way, it might be best to avoid returning nil in general (except perhaps for 'primitives?), and instead always go for a boolean return value. (I have little experience in these matters, so please correct me if I'm wrong!)
- oelmekki 12y ago
  > it is more natural to say "if current_user" than "if current_user.name != 'Guest'". It is even more natural to say `if signed_in?` :) Devise provides such a method that checks for effective auth in warden, afaik (so it should know to make difference between a NullObject and an actual User). It would be easy to implement it yourself : def signed_in? current_user.is_a? User end
- BrianVanLoo 12y ago
  The point of that particular item in the article was to eliminate the conditional logic for these display items from the place that it occurs: in the views. Make the goal to get rid of all the places where you want to put <% if ... %> in your view code. So rather than add even more logic to determine if it's the guest user or not to decide if you're displaying your reply button how about adding a method to your User object that returns the markup for your reply button? Then in the guest object make that return some kind of placeholder markup that isn't the reply button. The problem as I see it are complex view files that mix html (yeah, I know there are other templating options) with a whole bunch of conditional programming code. The cognitive load to try and understand if the html is correct (are all those tags closed properly?) combined with all that conditional logic quickly becomes overwhelming.
enraged_camel 12y ago
As a Rails newbie (~3 months), I found this list very useful. I'm very mindful of the difference between doing something vs. doing that thing correctly, and even when I write a piece of code that works, I often stop and ask myself, "OK, but is this the best way to do it?" I try to avoid dirty hacks, and if I'm on a time crunch where I don't have time to figure out the right way, I postpone the feature to the next release rather than putting in a temporary dirty implementation. One thing I've been having trouble with as my project gets more complex is when to refactor. I guess I'm not yet at the stage where I can accurately judge the benefits of refactoring a code vs. the cost. To use an analogy, am I just re-arranging the furniture to change the look-and-feel of the room (superficial), or am I actually making it easier for the guests to make themselves comfortable (real benefit)? I've read books like Pragmatic Programming [1] that advise refactoring whenever possible to avoid technical debt build-up. But it seems to be that there are times when working on a new feature would result in more value-add for the project as a whole. Another thing is motivating myself to write test cases. The Hartl tutorial taught TDD, but when I started my project from scratch I quickly realized that TDD would slow down my progress significantly, and I didn't yet have enough experience to assess how a feature should work to be able to write tests for it. This did bite me in the ass just last night though, as I spent three hours troubleshooting a bug that a test case would have caught. Oh well, live and learn, right? :)
- WestCoastJustin 12y ago
  > Rails newbie (~3 months) Don't know if you have seen http://railscasts.com http://railscasts.com yet. But it really helped me.
  - enraged_camel 12y ago
    Yeah, I love Railscasts. I'm sad Ryan stopped doing them.
- danneu 12y ago
  > One thing I've been having trouble with as my project gets more complex is when to refactor. I usually wait until (A) something is painful and (B) I have enough instances of that pain to see the pattern that my refactor/abstraction will address/consolidate. Else I'm just rearranging furniture or making it harder to rearrange in the future because my assumptions were wrong -- I've built a fridge and stove in a room that never became the kitchen when I could've just fetched a bucket of ice and a bunsen burner as I needed them. But now all the doors are too small to migrate my appliances because I thought this was going to be a hobbit house. Turns out the guests are only half hobbit. It's easier to refactor naive, repetitive, even dirty code than it is to refactor failed and premature refactors/abstractions. Seeking correctness is cool, but often I can't see far enough ahead to know what "correct" actually entails. Writing what feels like a simple/obvious hack now can be better down the road than trying to prematurely engineer something that seems correct.
hmans 12y ago
This is the first article of this kind that I actually agree with. Usually, these things are "5 Popular Rails Mistakes: 1) Your code Book Of Fours the SRP concurrency something something don't use the database in tests blah."
pothibo 12y ago
I agree with the gem list. I think it should have been #1. Using OpenStruct as a replacement for a user doesn't remove the conditionnal, it just removes it from that use case. You still need to know if the user is logged in or not.
- BrianVanLoo 12y ago
  Glad to see some support on the gem list. That was one of the items I really hadn't seen discussed anywhere else and was wondering if it only bothered me. I did some dev ops for a project that turned out to be what would have sounded like a fairly simple, small Rails app on a rather low-traffic site. I kept having to increase the server instance size as the team cargo-culted more and more gems into the Gemfile and the multiple Rails executables run under Unicorn continued to grow in size and eventually run out of memory on the server. I agree that my example using OpenStruct only moves the conditional but in this case it moves it out of a place where I find conditional logic much harder to reason about. Also, if there were more attributes than just name that come out of that object you could potentially be eliminating many more conditionals with just the one in current_user which also seems like a win.
sanderjd 12y ago
Perhaps I'm in the minority, but I think "too much automated testing" is a more common problem in rails apps than "too little automated testing". With a corollary being that "too little non-automated testing" seems to be a common problem with rails teams.

58 points by djug 12y ago

alphadevx 12y ago

Not a bad list, but I think many of those are not unique to Rails and just look like common anti-patterns.

deleted 12y ago

[deleted]

3pt14159 12y ago

Personally, I wouldn't make current_user return "guest". That would be surprising. I would just return nil. There are lots of places in an application that you'll want to conditionally show logic, like whether or not to show a "reply" button, and it is more natural to say "if current_user" than "if current_user.name != 'Guest'".

taylorfausak 12y ago
I would much rather have it return a null object instead of nil. You could make it better by adding a #guest? method to your User class. if current_user.guest? Although I don't like how the example given returns an OpenStruct instead of a NullUser, for instance.
jstin 12y ago
I've been using the CanCan(Can) gem to handle all permissions logic, both in controllers and views. It abstracts all user state logic to a single file https://github.com/CanCanCommunity/cancancan https://github.com/CanCanCommunity/cancancan I find this pattern to be the most manageable especially in large projects. But I agree with your point. "if current_user" is miles better than "if current_user.name != 'Guest'". Edited to add: As far as this articles use of current_user.name being guest, it makes sense to use a presenter to display the data in the view. Conditionally displaying a user name or 'Guest' in a view is kinda gross. Again I'll suggest a gem for this pattern https://github.com/drapergem/draper https://github.com/drapergem/draper All objects coming into the view should be wrapped, especially and ActiveRecord objects.
- michaelmior 12y ago
  Draper is great in general for keeping display logic away from controllers and views (as per mistakes 1 and 2).
rubiquity 12y ago
nil is annoying and should be avoided when possible. nil gives no useful feedback when blowing up and communicates nothing about the intent of the code. Using a Guest object is a great use of polymorphism and duck typing, which are great features of Ruby (and object orientation). Having nils all over the place and having to do `if some_obj` all of the time is messy. Your example is pretty simple, so my solution will sound like overkill, but I would use some other type of object such as a Presenter to handle the rendering logic.
- 3pt14159 12y ago
  Naw, you're complicating things. Nil should not be avoided, it should be accepted. If there isn't a current user then don't pretend that there is. Anywhere you are doing `if some_obj` in view code you'll probably be doing `if some_obj.is_a? Guest` or `if some_obj.guest?` anyway. I've worked on very large and well used applications (billions of loads, multiple data centers, hundreds of thousands of lines of code), not that that should really matter to the argument (logical fallacy and all that), but the thing that I've found messes up Rails applications is large model objects ("god objects"), poorly thought out data relationships, and inconsistent assumptions across systems that lead to hacks. Not nils.
  - rubiquity 12y ago
    > Anywhere you are doing `if some_obj` in view code you'll probably be doing `if some_obj.is_a? Guest` or `if some_obj.guest?` anyway. Sorry, you're completely wrong. Polymorphism and duck typing exist specifically so that you don't have to do `is_a?` checks. > but the thing that I've found messes up Rails applications is large model objects ("god objects"), How do you think god object models get created in the first place? By not extracting out logic. When you have models that have very different state, you actually have very different objects. Those differences should be extracted into different classes.
- Finbarr 12y ago
  Agree that `if some_obj` all the time is annoying. Instead, better to introduce semantic helper methods like `if signed_in?` (which incidentally is included with gems like Devise).
awj 12y ago
As taylorfusak mentioned, a User#guest? method is a more natural way to handle that problem. You also could make use of ActiveModel::Conversion#to_partial_path, like so: <%= render partial: current_user %> This implicitly calls to_partial_path on the current user object and looks up a template to render based on that result. So the user model would return "users/user" and your guest object could return "users/guest". The problem with using nil here is that you have to remember to check it everywhere, all of your tests have to exercise both the nil and not-nil cases, and most of the "else" clauses are just going to substitute default values anyways. In my opinion if your views know so much about your user model that substituting a "guest" object is hard, maybe it's a sign you need to pull some logic back out of your views.
- 3pt14159 12y ago
  I disagree. Generally you will have sections of your app that are only for logged in users, so your ACL logic in your controller handily takes care of that, while still giving you the convenience of being able to call current_user and know that you actually have a user, while still being able to say things like `if current_user` where you need it. I do like your render partial example, but in most cases you will want to be doing something like showing a like button if the user is signed in, in which case you can do: <%= render partial: current_user %> And if the current_user is nil the partial will just be skipped.
  - awj 12y ago
    I'm not sure I understand your point. If your controller logic ensures that users will always be logged in, why would you need to test for the presence of current_user?
    - 3pt14159 12y ago
      Sometimes we want to just say current_user.username
logicallee 12y ago
This is just responding to your specific example: >There are lots of places in an application that you'll want to conditionally show logic, like whether or not to show a "reply" button, and it is more natural to say "if current_user" than "if current_user.name != 'Guest'". "if current_user.name != 'Guest'" is way more natural (not saying that is good or bad.) I read that as "if the current user is 'Guest'" whereas the former reads, "if there is a current user". There is obviously a user. When isn't there one? Don't read too much into this, I am just speaking about what looks natural in your specific comment.
- mercer 12y ago
  I think what partly complicates this is our 'semantic'(?) understanding of a 'user' in the context of a Rails app, where it might make more (or less) sense to return nil, but where this is not immediately obvious. If instead you'd have a car object in the context of an app that sells junkyard items, it would make sense to return '0' instead of 'nil' when it concerns wheels (wow, this might be a terrible example...). It seems to me that using too much of this implicit 'semantic thinking' is to be avoided, especially when the semantic meaning is not entirely clear or always agreed upon, like with user objects. And if you look at this that way, it might be best to avoid returning nil in general (except perhaps for 'primitives?), and instead always go for a boolean return value. (I have little experience in these matters, so please correct me if I'm wrong!)
oelmekki 12y ago
> it is more natural to say "if current_user" than "if current_user.name != 'Guest'". It is even more natural to say `if signed_in?` :) Devise provides such a method that checks for effective auth in warden, afaik (so it should know to make difference between a NullObject and an actual User). It would be easy to implement it yourself : def signed_in? current_user.is_a? User end
BrianVanLoo 12y ago
The point of that particular item in the article was to eliminate the conditional logic for these display items from the place that it occurs: in the views. Make the goal to get rid of all the places where you want to put <% if ... %> in your view code. So rather than add even more logic to determine if it's the guest user or not to decide if you're displaying your reply button how about adding a method to your User object that returns the markup for your reply button? Then in the guest object make that return some kind of placeholder markup that isn't the reply button. The problem as I see it are complex view files that mix html (yeah, I know there are other templating options) with a whole bunch of conditional programming code. The cognitive load to try and understand if the html is correct (are all those tags closed properly?) combined with all that conditional logic quickly becomes overwhelming.

enraged_camel 12y ago

As a Rails newbie (~3 months), I found this list very useful. I'm very mindful of the difference between doing something vs. doing that thing correctly, and even when I write a piece of code that works, I often stop and ask myself, "OK, but is this the best way to do it?" I try to avoid dirty hacks, and if I'm on a time crunch where I don't have time to figure out the right way, I postpone the feature to the next release rather than putting in a temporary dirty implementation. One thing I've been having trouble with as my project gets more complex is when to refactor. I guess I'm not yet at the stage where I can accurately judge the benefits of refactoring a code vs. the cost. To use an analogy, am I just re-arranging the furniture to change the look-and-feel of the room (superficial), or am I actually making it easier for the guests to make themselves comfortable (real benefit)? I've read books like Pragmatic Programming [1] that advise refactoring whenever possible to avoid technical debt build-up. But it seems to be that there are times when working on a new feature would result in more value-add for the project as a whole. Another thing is motivating myself to write test cases. The Hartl tutorial taught TDD, but when I started my project from scratch I quickly realized that TDD would slow down my progress significantly, and I didn't yet have enough experience to assess how a feature should work to be able to write tests for it. This did bite me in the ass just last night though, as I spent three hours troubleshooting a bug that a test case would have caught. Oh well, live and learn, right? :)

WestCoastJustin 12y ago
> Rails newbie (~3 months) Don't know if you have seen http://railscasts.com http://railscasts.com yet. But it really helped me.
- enraged_camel 12y ago
  Yeah, I love Railscasts. I'm sad Ryan stopped doing them.
danneu 12y ago
> One thing I've been having trouble with as my project gets more complex is when to refactor. I usually wait until (A) something is painful and (B) I have enough instances of that pain to see the pattern that my refactor/abstraction will address/consolidate. Else I'm just rearranging furniture or making it harder to rearrange in the future because my assumptions were wrong -- I've built a fridge and stove in a room that never became the kitchen when I could've just fetched a bucket of ice and a bunsen burner as I needed them. But now all the doors are too small to migrate my appliances because I thought this was going to be a hobbit house. Turns out the guests are only half hobbit. It's easier to refactor naive, repetitive, even dirty code than it is to refactor failed and premature refactors/abstractions. Seeking correctness is cool, but often I can't see far enough ahead to know what "correct" actually entails. Writing what feels like a simple/obvious hack now can be better down the road than trying to prematurely engineer something that seems correct.

hmans 12y ago

This is the first article of this kind that I actually agree with. Usually, these things are "5 Popular Rails Mistakes: 1) Your code Book Of Fours the SRP concurrency something something don't use the database in tests blah."

pothibo 12y ago

I agree with the gem list. I think it should have been #1. Using OpenStruct as a replacement for a user doesn't remove the conditionnal, it just removes it from that use case. You still need to know if the user is logged in or not.

BrianVanLoo 12y ago
Glad to see some support on the gem list. That was one of the items I really hadn't seen discussed anywhere else and was wondering if it only bothered me. I did some dev ops for a project that turned out to be what would have sounded like a fairly simple, small Rails app on a rather low-traffic site. I kept having to increase the server instance size as the team cargo-culted more and more gems into the Gemfile and the multiple Rails executables run under Unicorn continued to grow in size and eventually run out of memory on the server. I agree that my example using OpenStruct only moves the conditional but in this case it moves it out of a place where I find conditional logic much harder to reason about. Also, if there were more attributes than just name that come out of that object you could potentially be eliminating many more conditionals with just the one in current_user which also seems like a win.

sanderjd 12y ago

Perhaps I'm in the minority, but I think "too much automated testing" is a more common problem in rails apps than "too little automated testing". With a corollary being that "too little non-automated testing" seems to be a common problem with rails teams.