Daily Refactor #1: Extract Method in the BulkTimeEntriesController – saving a new TimeEntry

I’m having two problems right now:

  1. Not enough time to write to my blogs
  2. 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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21

  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  '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 =&gt; @time_entry.hours, :target =&gt; 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:

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

  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 =&gt; 'time_entry', :object =&gt; @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 =&gt; @time_entry.hours, :target =&gt; 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 &lt;= 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.

Reference commit