Daily Refactor #33: Extract Method in IssuesController

I’ve decided to try something different with the next refactorings. Instead of working on a single section of code until it’s very well factored, I’m going to jump around a bit and tackle the sections that smell the worst. This will be good because I can work in different sections of Redmine and remove the worst code across the entire codebase.

The Refactoring

Using the flay tool again, I discovered a code duplication in the IssuesController.

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

# app/controller/issues_controller.rb
class IssuesController  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
 
  def move
    # ... inside of an if statement
 
      if unsaved_issue_ids.empty?
        flash[:notice] = l(:notice_successful_update) unless @issues.empty?
      else
        flash[:error] = l(:notice_failed_to_save_issues, :count => unsaved_issue_ids.size,
                                                         :total => @issues.size,
                                                         :ids => '#' + unsaved_issue_ids.join(', #'))
      end
 
   # ...
  end
 
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

# app/controller/issues_controller.rb
class IssuesController  'issues', :action => 'index', :project_id => @project})
      return
    end
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end
 
  def move
    # ... inside of an if statement
 
      set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
 
   # ...
  end
 
  def set_flash_from_bulk_issue_save(issues, unsaved_issue_ids)
    if unsaved_issue_ids.empty?
      flash[:notice] = l(:notice_successful_update) unless issues.empty?
    else
      flash[:error] = l(:notice_failed_to_save_issues,
                        :count => unsaved_issue_ids.size,
                        :total => issues.size,
                        :ids => '#' + unsaved_issue_ids.join(', #'))
    end
  end
 
end

Review

This was a simple extract method refactoring to remove duplication. I’m finding that extract method is one of the best ways to remove duplication in a single class.

(Sorry about missing a refactor yesterday, an emergency came up that kept me off the computer all day.)

Reference commit