Since I’m all hopped-up on white tea from an early morning meeting, this refactoring is a two in one.
The Refactoring
I used move method and extract method on the InvoiceController to move the business logic down into the model where it belongs.
Before
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 | class InvoiceController ['time_entries.spent_on >= :from AND time_entries.spent_on @date_from, :to => @date_to, :activities => @activities }], :include => [:time_entries]) @total_time = @issues.collect(&:time_entries).flatten.collect(&:hours).sum # Time logged without an issue @time_entries = @p.time_entries.find(:all, :conditions => ['issue_id IS NULL AND spent_on >= :from AND spent_on @date_from, :to => @date_to, :activities => @activities }]) @total_time += @time_entries.collect(&:hours).sum @total = @total_time.to_f * params[:autofill][:rate].to_f respond_to do |format| format.js end end end |
1 2 | class Autofill end |
After
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 | class InvoiceController < ApplicationController def autofill @autofill = Autofill.new_from_params(params[:autofill]) # TODO: should just access @autofill directly @p = @autofill.p @customer = @autofill.customer @date_from = @autofill.date_from @date_to = @autofill.date_to @activities = @autofill.activities @issues = @autofill.issues @total_time = @autofill.total_time @time_entries = @autofill.time_entries @total = @autofill.total respond_to do |format| format.js end end end |
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 | class Autofill def self.new_from_params(params) autofill = Autofill.new # Get project autofill.p = Project.find_by_id(params[:project_id]) # Get customer autofill.customer = Customer.find_by_id(autofill.p.customer_id) # Customer plugin only has a 1-way relationship # Build date range autofill.date_from = params[:date_from] autofill.date_to = params[:date_to] # Build activities if params[:activities] autofill.activities = params[:activities].collect {|p| p.to_i } end autofill.activities ||= [] # Fetch issues autofill.issues = autofill.p.issues.find(:all, :conditions => ['time_entries.spent_on >= :from AND time_entries.spent_on autofill.date_from, :to => autofill.date_to, :activities => autofill.activities }], :include => [:time_entries]) autofill.total_time = autofill.issues.collect(&:time_entries).flatten.collect(&:hours).sum # Time logged without an issue autofill.time_entries = autofill.p.time_entries.find(:all, :conditions => ['issue_id IS NULL AND spent_on >= :from AND spent_on autofill.date_from, :to => autofill.date_to, :activities => autofill.activities }]) autofill.total_time += autofill.time_entries.collect(&:hours).sum autofill.total = autofill.total_time.to_f * params[:rate].to_f autofill end end |
Review
With this code now in the model, the controller action is a lot cleaner and I can really start to work on refining the Autofill logic. This also opens up the potential for scripting the Model with a Rake tasks (e.g. automated invoices?).
One thing I wanted to point out:
This Autofill code is horrible, ugly, and only had user testing (no unit tests). And I wrote it as an experienced Rails developer who knew better. Point: everyone writes bad code, what makes you a better developer is keeping an open mind and trying to improve that code as much as you can. Just like what I’m trying to do with this Refactoring series.
Share
Related posts:
- Daily Refactor #22: Move method in IssuesController#bulk_edit
- Redmine Refactor #140: Extract Method WikiController#edit to #update
- Redmine Refactor #88: Extract Method from move and perform_move
- Daily Refactor #81: Move last_invoice_number to Model
- Daily Refactor #31: Extract Method in IssuesHelper#show_detail
