How to refactor this Rails code?

Question

Consider this helper method:

module SomeHelper

  def display_button
    Foo.find_by_id params[:id] and Foo.find(params[:id]).organizer.name != current_name and Foo.find(params[:id]).friends.find_by_name current_name
  end

end

How to refactor into something more readable?

Rails 3.2.2

Yes, it is boolean. I should have added that to the question. — B Seven
– B Seven, Commented Sep 6, 2012 at 14:56
Please note that and/or is not the same as &&/|| in Ruby. — Andrew Marshall
– Andrew Marshall, Commented Sep 6, 2012 at 14:57
@AndrewMarshall - Actually, that was causing a problem. I want the and/&& to be evaluated last. and is lower precedence. techotopia.com/index.php/Ruby_Operator_Precedence — B Seven
– B Seven, Commented Sep 6, 2012 at 15:00

Kyle · Accepted Answer · 2012-09-06 14:51:34Z

4

Something like this?

module SomeHelper

  def display_button?
    if foo = Foo.find(params[:id])
      foo.organizer.name != current_name if foo.friends.find_by_name(current_name)
    end
  end

end

Note: if the helper method is returning a boolean, I would append the name with a ? ... ruby convention.

answered Sep 6, 2012 at 14:51

Kyle

22.3k2 gold badges63 silver badges63 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Baldrick · Accepted Answer · 2012-09-06 15:15:56Z

2

You can factorize the call to Foo.find(params[:id]) and use exists? for the third condition

module SomeHelper
  def display_button
    foo = foo.find_by_id params[:id]
    foo and foo.organizer.name != current_name and foo.friends.where(:name => current_name).exists?
  end
end

You can also create several methods to gain on reusability (and will save trouble if you model changes):

module SomeHelper
  def display_button
    foo = foo.find_by_id params[:id]
    foo && !is_organizer?(foo, current_name) && has_friend?(foo, current_name)
  end

  def is_organizer?(foo, name)
    foo.organizer.name == name
  end 

  def has_friend?(foo, name)
    foo.friends.where(:name => name).exists?
  end
end

edited Sep 6, 2012 at 15:15

answered Sep 6, 2012 at 14:52

Baldrick

24.4k6 gold badges76 silver badges80 bronze badges

1 Comment

prusswan Over a year ago

Breaking into multiple methods is really the key to refactoring the original method, which was doing too much.

Mark Bolusmjak · Accepted Answer · 2012-09-06 14:51:51Z

1

try invokes the passed block on non-nil objects. Returns nil otherwise. So the return will be nil,true,false depending on your data.

def display_button
    Foo.find_by_id(params[:id]).try do |foo|
       foo.organizer.name != current_name && 
         foo.friends.find_by_name current_name
    end
  end

answered Sep 6, 2012 at 14:51

Mark Bolusmjak

24.6k10 gold badges77 silver badges133 bronze badges

Collectives™ on Stack Overflow

How to refactor this Rails code?

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related