I'm having two problems right now:
- Not enough time to write to my blogs
- Not enough time to keep my Redmine plugins maintained
To try and fix both of these problems, I'm going to try and start something new here: a daily refactoring. I'm going do a small refactoring to my Redmine plugins or Redmine each day and post my thoughts about it here.
The Refactoring
I was working on my Bulk Time Entry plugin earlier today and found some very bad code in a controller.
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
@time_entry = TimeEntry.new(entry)
@time_entry.hours = nil if @time_entry.hours.blank? or @time_entry.hours <= 0
@time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
@time_entry.user = User.current
unless @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
Without getting into all of the smells (e.g. rendering and building model objects in the controller), the one that really stood out to me was that it was creating and saving a new TimeEntries in a loop. Ideally, this would be done in a model method but first I needed to extract out all of the parts used. So I used extract method with gave me this:
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = save_time_entry_from_params(entry)
unless @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# ...
def save_time_entry_from_params(entry)
next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
helper_method :save_time_entry_from_params
Review
This code still is a bit smelly, but it's getting better. Now I can decide if I want to push save_time_entry_from_params to the model or clean up save's RJS rendering.
