Daily Refactor #22: Move method in IssuesController#bulk_edit

I started to refactor the IssuesController today. Like I said yesterday, this class is full of bad smells and could really use some major rework. Picking a good place to start is overwhelming so I turned to a useful tool called flog. Flog is a tool that I use to find out how complex a class or method is. It checks for assignments (abc = 42), branches (if), and calls (@foo.bar) and gives each method a total score.

Using the rough numbers from flog, I can see which methods in IssuesController need the most work:

flog app/controllers/issues_controller.rb
  1423.2: flog total
    67.8: flog/method average

   194.9: IssuesController#bulk_edit
   145.6: IssuesController#edit
   136.4: IssuesController#retrieve_query
   131.6: IssuesController#move
   127.3: IssuesController#new
    98.3: IssuesController#index
    84.8: IssuesController#context_menu

The #bulk_edit method looks the worst according to flog so that’s a good place to start.

The Refactoring

The easiest way to reduce the flog score for one method is to move a similar chunk of code to a new method. This won’t affect the overall score since the complexity is just moved to the new method, but it can make it easier to refactor and reuse the code in other methods. For this refactoring, I used both extract method and move method to move a chunk of code from #bulk_edit to the Issue objects themselves.

Before

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

# app/controllers/issues_controller.rb
class IssuesController  params, :issue => issue })
        # Don't save any change to the issue if the user is not authorized to apply the requested status
        unless (status.nil? || (issue.new_statuses_allowed_to(User.current).include?(status) && issue.status = status)) && issue.save
          # Keep unsaved issue ids to display them in flash error
          unsaved_issue_ids < unsaved_issue_ids.size,
                                                         :total => @issues.size,
                                                         :ids => '#' + unsaved_issue_ids.join(', #'))
      end
      redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
      return
    end
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end
  #...
end
 
1
2
3
4

# app/models/issue.rb
class Issue < ActiveRecord::Base
  #...
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

# app/controllers/issues_controller.rb
class IssuesController  tracker,
                                     :status => status,
                                     :priority => priority,
                                     :assigned_to => assigned_to,
                                     :category => category,
                                     :fixed_version => fixed_version,
                                     :custom_field_values => custom_field_values
                                   })
 
      unsaved_issue_ids = []      
      @issues.each do |issue|
        unless issue.bulk_edit(merged_params)
          # Keep unsaved issue ids to display them in flash error
          unsaved_issue_ids < unsaved_issue_ids.size,
                                                         :total => @issues.size,
                                                         :ids => '#' + unsaved_issue_ids.join(', #'))
      end
      redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
      return
    end
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end
  # ...
end
 
1
2
3
4
5
6
7
8
9

# app/models/issue.rb
class Issue  params, :issue => self })
 
    # Don't save any change to the issue if the user is not authorized to apply the requested status
    return (params[:status].nil? || (new_statuses_allowed_to(User.current).include?(params[:status]) && self.status = params[:status])) && save
  end
 
  #...
end

Review

I had to put a small shim into #bulk_edit in order for the extraction to work without changing how the code works. This is shown by the merged_params variable. Like I’ve said previously, an inline comment is an easy way to spot a new refactoring. This time, I added a comment to myself to remind me later.

Now that the #bulk_edit method has been moved to the Issue class, the flog score for IssuesController is looking a lot better now.

flog app/controllers/issues_controller.rb 
  1354.9: flog total
    64.5: flog/method average

   145.6: IssuesController#edit
   136.4: IssuesController#retrieve_query
   131.6: IssuesController#move
   127.3: IssuesController#new
   126.6: IssuesController#bulk_edit
    98.3: IssuesController#index
    84.8: IssuesController#context_menu

If I run flog on the Issue model, you can still see the 60 points. This is because the complexity was just moved around, not actually removed.

flog app/models/issue.rb 
   788.5: flog total
    15.8: flog/method average

    ...
    61.1: Issue#bulk_edit
    ...

Now there are a few things I can refactor in the IssuesController and Issue classes. I think I’ll stick with the controller for now, since it’s in worse condition.

Reference commit