Daily Refactor #3: Behavior Change in the Bulk Time Entry patch

Today’s “refactoring” isn’t a refactoring in the classic sense, since it includes a behavior change. But if you squint your eyes enough to only see the Controller as the “public interface”, then we can call it a refactor.

The Refactoring

Yesterday I moved the create_bulk_time_entry into the TimeEntry class where it fit better. Today I wanted to refine it more, since the Controller is still doing the actual save on the Model.

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
25
26
27
28
29
30
31
32

# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          unless @time_entry && @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 =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# time_entry_patch.rb
def create_bulk_time_entry(entry)
  return false 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

After

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

# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          if @time_entry.new_record?
            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
 
# time_entry_patch.rb
def create_bulk_time_entry(entry)
  time_entry = TimeEntry.new(entry)
  time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
  if BulkTimeEntriesController.allowed_project?(entry[:project_id])
    time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
  end
  time_entry.user = User.current
  time_entry.save
  time_entry
end

Review

The big behavior change is that instead of returning false or an unsaved TimeEntry, the method will now return a TimeEntry (saved if it’s valid). This is a better design, since the caller (Controller) will not have to worry about the different types of objects that are returned. It will still need to check that the TimeEntry was saved, but the code is a lot easier to understand (if @time_entry.new_record?).

This refactoring really cleaned up the major flaws in the model method. I think it’s now time to fix the controller/view interaction now, especially the inline RJS rendering.

Reference commit.