thr3ads.net - Rails - Can this be refactored? [Dec 2006]

If this information is useful, please help other people find it:
Share via:

Taylor Strait

2006-Dec-19 08:49 UTC

Can this be refactored?

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 %>
      <p><em>Please log in to add a referral for <%=
@person.full_name
%></em></p>
    <% end %>

2) is there anything wrong with having this in a view?  Alternatives?

-- 
Posted via http://www.ruby-forum.com/.

--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Gábor Sebestyén

2006-Dec-19 09:00 UTC

head link

Re: Can this be refactored?

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 %>
<p><em>Please log in to add a referral for <%= @person.full_name
%></em></p>
<% end %>

2. I think this is ok in view but sometimes it is better to hide
complex conditions like this in model.

Best regards,
Gábor


On 12/19/06, Taylor Strait <rails-mailing-list@andreas-s.net>
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]} %>
>           <%= render :partial => 'referrals/form' %>
>         <% end %>
>       <% end %>
>     <% else %>
>       <p><em>Please log in to add a referral for <%=
@person.full_name
> %></em></p>
>     <% end %>
>
> 2) is there anything wrong with having this in a view?  Alternatives?
--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to rubyonrails-talk@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

dblack-TKXtfPMJ4Ozk1uMJSBkQmQ@public.gmane.org

2006-Dec-19 13:06 UTC

head link

Re: Can this be refactored?

Hi --

On Tue, 19 Dec 2006, Taylor Strait 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 %>
>      <p><em>Please log in to add a referral for <%=
@person.full_name
> %></em></p>
>    <% end %>
>
> 2) 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 :-)

and then in the view:

   <% if @referee %>
     <%= render :partial => ''referrals/form''
   <% else %>
     <p> ...
   <% end %>


David

-- 
Q. What''s a good holiday present for the serious Rails developer?
A. RUBY FOR RAILS by David A. Black (http://www.manning.com/black)
    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)

--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Alan Francis

2006-Dec-19 13:52 UTC

head link

Re: Can this be refactored?

Taylor Strait 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 %>
>       <p><em>Please log in to add a referral for <%=
@person.full_name
> %></em></p>
>     <% end %>
> 
> 2) 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

-- 
Posted via http://www.ruby-forum.com/.

--~--~---------~--~----~------------~-------~--~----~
 You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Rails - Dec 2006 - Can this be refactored?

Can this be refactored?

Re: Can this be refactored?

Re: Can this be refactored?

Re: Can this be refactored?