Can this be refactored?

Rails
1

Below is code for a user referral form displayed only if several
conditions are met.

  1. can this be refactored?

    <% if session[:person_id] %>
    <% unless session[:person_id] == @person.id %>
    <% unless @referrals.any? {|referral| referral.giver_id ==
    session[:person_id]} %>
    <%= render :partial => ‘referrals/form’ %>
    <% end %>
    <% end %>
    <% else %>

    Please log in to add a referral for <%= @person.full_name

%>


<% end %>
  1. is there anything wrong with having this in a view? Alternatives?
2
  1. Lazy evaluation helps you eliminating nested conditions.

<% if session[:person_id] and @person.id != session[:person_id] and
not @referrals.any? {|r| r.giver_id == session[:person_id]} %>
<%= render :partial => ‘referrals/form’ %>
<% else %>

Please log in to add a referral for <%= @person.full_name %>

<% end %>
  1. I think this is ok in view but sometimes it is better to hide
    complex conditions like this in model.

Best regards,
Gábor

3

Taylor S. wrote:

Below is code for a user referral form displayed only if several
conditions are met.

  1. can this be refactored?

    <% if session[:person_id] %>
    <% unless session[:person_id] == @person.id %>
    <% unless @referrals.any? {|referral| referral.giver_id ==
    session[:person_id]} %>
    <% end %>
    <% end %>
    <% else %>

    Please log in to add a referral for <%= @person.full_name

%>


<% end %>
  1. is there anything wrong with having this in a view? Alternatives?

From looking at this it seems that @person is “the other person”, and
you’re checking to see if they already have a referral from “this
person” (the one logged in in the session) ?

I’d assume then that pewrson and referral have an association? person
has_many referrals?

If so, could you just say

<% unless @person.has_referral_from( session[:person_id] ) %>
<%= render :partial => ‘referrals/form’ %>
<% end %>

and in Person.rb

def has_referral_from( person )
referrals.any? {|referral| referral.giver == person }
end

I must confess to being sketchy on the interchangeability of an
ActiveRecord model and an id. It seems in some places you can pass
either. You may need to figure out if you’re being passed a model or an
id and adjust accordingly.

Alan

4

Hi –

On Tue, 19 Dec 2006, Taylor S. wrote:

Below is code for a user referral form displayed only if several
conditions are met.

  1. can this be refactored?

    <% if session[:person_id] %>
    <% unless session[:person_id] == @person.id %>
    <% unless @referrals.any? {|referral| referral.giver_id ==
    session[:person_id]} %>

You could conceivably push this down into a model. However…

     <%= render :partial => 'referrals/form' %>
   <% end %>
 <% end %>

<% else %>

Please log in to add a referral for <%= @person.full_name
%>


<% end %>

  1. is there anything wrong with having this in a view? Alternatives?

I would tend to advocate putting this logic in the controller.
Accessing the session directly in the view always feels a bit
transgressive to me – no real harm in it, but session always suggests
controller logic, since the controller rather than the view is in
charge of the session. This is of course more a concern with writing
to the session than reading from it, but I still tend to think of it
as the controller’s job.

I’m thinking of something like (give or take any flaws in the logic
I’ve introduced):

def my_action
…
@referee = true unless
session[:person_id] == @person.id or
@referrals.any? {|r| r.giver_id == session[:person_id] }

end

(You don’t need the explicit assignment to ‘true’, but I like the way
it reads :slight_smile:

and then in the view:

<% if @referee %>
<%= render :partial => ‘referrals/form’
<% else %>

…
<% end %>

David

–
Q. What’s a good holiday present for the serious Rails developer?
A. RUBY FOR RAILS by David A. Black (Ruby for Rails)
aka The Ruby book for Rails developers!
Q. Where can I get Ruby/Rails on-site training, consulting, coaching?
A. Ruby Power and Light, LLC (http://www.rubypal.com)