The Refactoring
Today’s refactoring cleans up some of the stubs left from last Friday.
Before
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
| # app/views/invoices/autofill.js.rjs
page['invoice_amount'].value = @total
page['invoice_project_id'].value = @p.id
# Invoice description
description = ''
description << "h3. " + h(@p.name) + "\n\n"
description << h(@p.description) + "\n\n"
description << "h3. Work Completed\n\n"
@issues.each do |issue|
description << "* #{issue.subject} (#{issue.status} ##{issue.id})\n"
end
# Total time
description << "\n"
description << "Total billable time: " + pluralize(@total_time, "hour")
description << " (" + h(@date_from) + " to " + h(@date_to) + ")"
page['invoice_description'].value = description
# Customer
if @customer.blank?
page.alert l(:label_no_customer_on_project)
else
page['invoice_customer_id'].value = @customer.id
page.replace_html 'customer_name', h(@customer.name)
end |
After
1
2
3
4
5
6
7
8
9
| class InvoiceController < ApplicationController
def autofill
@autofill = Autofill.new_from_params(params[:autofill])
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
| # app/views/invoices/autofill.js.rjs
page['invoice_amount'].value = @autofill.total
page['invoice_project_id'].value = @autofill.p.id
# Invoice description
description = ''
description << "h3. " + h(@autofill.p.name) + "\n\n"
description << h(@autofill.p.description) + "\n\n"
description << "h3. Work Completed\n\n"
@autofill.issues.each do |issue|
description << "* #{issue.subject} (#{issue.status} ##{issue.id})\n"
end
# Total time
description << "\n"
description << "Total billable time: " + pluralize(@autofill.total_time, "hour")
description << " (" + h(@autofill.date_from) + " to " + h(@autofill.date_to) + ")"
page['invoice_description'].value = description
# Customer
if @autofill.customer.blank?
page.alert l(:label_no_customer_on_project)
else
page['invoice_customer_id'].value = @autofill.customer.id
page.replace_html 'customer_name', h(@autofill.customer.name)
end |
Review
By changing the data access so it goes through the @autofill instance, I don’t have to track as many instance variables in the controller and have the flexibility to change any of the variables’ implementation later. This will let me refactor that RJS away into a cleaner interface later.
Reference commit
Share
Related posts:
- Daily Refactor #49: Replace Incoming Pane’s Data Value with Object
About Eric Davis
I founded
Little Stream Software where I provide Redmine and ChiliProject services to help projects teams. I also created an ebook,
Redmine Tips, were I show you how to become more productive using Redmine.
I am also the author of Refactoring Redmine, where I go about
refactoring Rails using Redmine as an example.
