Hacker News Reader

Comment by 3pt14159 | Hacker News Reader

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'".

View on HN

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.

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.

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.

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?

Sometimes we want to just say

    current_user.username

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.

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.

> 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.

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).

helpers are awful

embrace OO

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.

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.

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!)

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

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

All objects coming into the view should be wrapped, especially and ActiveRecord objects.

Draper is great in general for keeping display logic away from controllers and views (as per mistakes 1 and 2).

> 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

Reply on news.ycombinator.com