Daily Refactor #77: Extract and Move Method in InvoiceController

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.

Reference commit