CODEWITHSUNDEEP
ruby-on-railsrubyrefactoring
Alvin Lau
Alvin Lau

Reputation: 101

Rails code block in controller for repeated code

In my Rails application I am using the odf-report gem to generate reports. However I have an if condition in my method with the same 110 lines of code in each clause with one or two changes at the end. I am wondering if there is a way to define the 110 lines that are being repeated in a code block and just call that code block in my main method? Below is a sample of the method:

def print_enrolment_form_completed
  kid = Kid.find(params[:id])
  if kid.not_anaphylactic?
    report = ODFReport::Report.new("#{Rails.root}/app/reports/Student_Enrolment_Completed.odt") do |r|
       #same 110 lines of code 
    end
  else
    report = ODFReport::Report.new("#{Rails.root}/app/reports/Student_Enrolment_Completed_Allergy.odt") do |r|
       #same 110 lines of code 
       r.add_field(:a2, kid.fish ? "Yes" : "No" )
       r.add_field(:a3, kid.eggs ? "Yes" : "No" )
       r.add_field(:a4, kid.milk ? "Yes" : "No" )
    end
  end
end

My goal is to just yield a code block where the comment is listed above and have the 110 lines defined elsewhere in the controller. Any ideas are appreciated!

Upvotes: 0

Views: 500

Answers (3)

Konrad Sotniczuk
Konrad Sotniczuk

Reputation: 13

Can't you just create a method containing that 110 lines of code and use it in the if statement?

    def method_name(z, y)
      puts z + y
    end

    x = 4

    if x > 3 
      method_name(6, 7)
    else
      method_name(1, 4)
    end

Upvotes: 0

Kevin P
Kevin P

Reputation: 106

Definitely agree that that many lines of code in the controller is code smell and not just for the lack of DRYness.

That said, you might not be in a place where you can do the full refactoring now. The only difference between the two branches is the string passed to the new and three lines at the end.

  report = ODFReport::Report.new(kid.not_anaphylactic? ? "#{Rails.root}/app/reports/Student_Enrolment_Completed.odt" : "#{Rails.root}/app/reports/Student_Enrolment_Completed_Allergy.odt") do |r|
   #same 110 lines of code
    If kid.not_anaphylactic?
      r.add_field(:a2, kid.fish ? "Yes" : "No" )
      r.add_field(:a3, kid.eggs ? "Yes" : "No" )
      r.add_field(:a4, kid.milk ? "Yes" : "No" )
    end
 end

Upvotes: 0

sarathprasath
sarathprasath

Reputation: 609

You are doing wrong, if you have 1000s of lines on controller action. I think you should consider delayed jobs / active jobs or sidekiq or resque

Upvotes: 1

Related Questions