Best practice question...

rubyonrails-talk
1

Hello guys!

Question:

Views should be "dumb" and models should deal with db queries...

but what if you have a situation like so

class User < ActiveRecord::Base

  def has_relationship_with?( other_user )     !relationships.find_by_other_user_id( other_user.id ).nil?   end end

and then in your view:

<%= "Display this message" if @user.has_relationship_with? (@other_user) %>

Is it cool to call model methods which, in turn, query the db within the view or is there a better way of implementing this?

Thanks

ASH

2

This should be fine, but you might want to think about whether you need to "eager load" the relationships assosciation for performance.

The benefit is that your model contains the code and you can potentially refactor without touching your view.

-Rob

Rob Biedenharn http://agileconsultingllc.com Rob@AgileConsultingLLC.com

3

You might want to memoize the results of that function to prevent unnecessary hits to the database during the same request.

4

Hi Guys!

Thanks for both of your input.

I've opted for a helper method that looks something like:

module UsersHelper

  def has_relationship_with?( other_user )     var_name = "@has_relationship_with_#{other_user.id}"     instance_variable_get(var_name) || instance_variable_set(var_name, current_user.has_relationship_with?( other_user )   end

end

Loading all of the relationships was a little unnecessary in this case but making the same db query more than once per request was too so I've stuck the value in an instance variable

you reckon that's ok?

5

Hi Guys!

Thanks for both of your input.

I've opted for a helper method that looks something like:

module UsersHelper

def has_relationship_with?( other_user )    var_name = "@has_relationship_with_#{other_user.id}"    instance_variable_get(var_name) || instance_variable_set(var_name, current_user.has_relationship_with?( other_user ) end

end

Loading all of the relationships was a little unnecessary in this case but making the same db query more than once per request was too so I've stuck the value in an instance variable

you reckon that's ok?

Uh, a helper on the view? In this case, I'd say no.

I'm assuming that you really want to have several:

<%= "Interesting stuff" if @user.has_relationship_with?(@other_user) %>

<%= "Other stuff" if @user.has_relationship_with?(@other_user) %>

<%= "Boring stuff" if @user.has_relationship_with?(@other_user) %>

pieces in your view and you don't want 3 calls, only 1. Is that right?

If so, then your original solution may have done it. (ActiveRecord is
pretty smart most times.)

class User < ActiveRecord::Base    def has_relationship_with?( other_user )      relationships.find_by_other_user_id( other_user.id )    end end

Once the relationships association is loaded, it might just get later
results from what's already in memory. Take a look at your log/ development.log and see if you find entries like: (but for
Relationship, of course)

   School Load (0.000439) SELECT * FROM "schools" WHERE
("schools"."id" = 40882)    CACHE (0.000000) SELECT * FROM "schools" WHERE ("schools"."id" =
40882)    CACHE (0.000000) SELECT * FROM "schools" WHERE ("schools"."id" =
40882)

These are from a view helper (which in my case is actually an
ActiveRecord model backed by a real *database view* rather than a
table) that needs to find a record for the School model.

One thing to note is that you never have to say: !object.nil?                                         Just say: object Since ruby only considers false and nil to be "false" in a boolean
context, it is idiomatic for predicates (i.e., methods that provide a
yes/no answer) to return false/true or nil/object. Since
find_by_other_user_id is already going to give you the Relationship
object when it exists and nil otherwise, you don't need to turn that
into false/true.

-Rob