The Refactoring
Today I used move method to refactor part of Redmine that had a TODO comment since 2007.
Before
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
|
# app/controllers/application_controller.rb
class ApplicationController 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached << a)
end
if unsaved.any?
flash[:warning] = l(:warning_attachments_not_saved, unsaved.size)
end
end
attached
end
end |
# app/controllers/application_controller.rb
class ApplicationController 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached << a)
end
if unsaved.any?
flash[:warning] = l(:warning_attachments_not_saved, unsaved.size)
end
end
attached
end
end
1
2
3
4
|
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
# ...
end |
# app/models/attachment.rb
class Attachment < ActiveRecord::Base
# ...
end
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
|
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end |
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = attach_files(@issue, params[:attachments])
attachments.each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
1
2
3
4
|
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end |
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end
1
2
3
4
5
6
7
8
9
|
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached <<a> attached, :flash => flash, :unsaved => unsaved}
end
end |
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached <<a> attached, :flash => flash, :unsaved => unsaved}
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
|
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end |
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
I’m not completely happy with the results of this refactoring. The previous ApplicationController#attach_files
accessed the flash
directly so I had to do some tricks to keep the flash messages in the model. This is why the callers have to check attachments[:flash]
themselves. It would be ideal if the model could handle this directly so no one forgets to check the flash value, but that will have to wait until later.
Reference commit