CODEWITHSUNDEEP
rubyrefactoring
shanahobo86
shanahobo86

Reputation: 497

Ruby refactoring

I am making a simple rails site that will store some date and perform basic conditional checks. I wrote a few methods below and was told I could make them more efficient. I have been scratching my head and I'm not sure how to do this. Should I make entry.find global? or is there a more obvious solution? Thanks in advance

 def name
     @fname = params[:fst_name]
     @lname = params[:lst_name]
     @entry = Entry.create({:first_name => @fname, :last_name =>  @lname})
  end

  def attribs
     @person = Entry.find(:last)
     @fname = @person.first_name
     @lname = @person.last_name
     @person.update_attributes({:address => params[:st_name], 
         :salary => params[:salary], :loan => params[:loan], 
         :loan_reason => params[:reason]})
     if [email protected]? then render "show" end
  end

 def show
     @person = Entry.find(:last)
 end

  def modify
     @person = Entry.find(:last)
     @fname = @person.first_name
     @lname = @person.last_name
     @entry = Entry.create({:first_name => @fname, :last_name =>  @lname,  
                            :salary => params[:salary], :loan => params[:loan]})
  end

 def borrow
     @person = Entry.find(:last)
     if [email protected]? then
       if (@person.salary *  3) < @person.loan
            then @message = "You have asked for too much"
       else  @message = "No problem" 
       end
    else @message = "empty record?"
    end
 end


end

Upvotes: 1

Views: 327

Answers (1)

doesterr
doesterr

Reputation: 3965

  • Use a before_filter for the cases where you are repeatedly using @person = Entry.find(:last)

  • Don't make every variable an instance variable: for name: you don't need to acces @fname in the view, you can do @entry.first_name if you need to.

  • Don't use inline if then, use do_something if condition. Also remove the then from your other ifs too.

  • You may want to move the Event creation into the model. Something like self.create_from_person_and_modify_params

  • Use Event.last instead of Event.find(:last)

  • Use if @person.salary instead of if [email protected]?

  • Move the if (@person.salary * 3) < @person.loan condition into the model. Something like asks_for_reasonable_raise?

Upvotes: 4

Related Questions