Whenever I work with legacy code I always turn to extract method and move method for most of my refactoring. Redmine’s IssuesController
is no exception.
Using extract method on the #move
action I was able to extract several lines of code deep in the method and also remove a temporary variable.
Before
1 2 3 4 5 6 7 8 9 10 |
class IssuesController params, :issue => issue, :target_project => @target_project, :copy => !!@copy }) if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => changed_attributes}) moved_issues << r else unsaved_issue_ids << issue.id end end # ... end end |
After
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 |
class IssuesController params, :issue => issue, :target_project => @target_project, :copy => !!@copy }) if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)}) moved_issues << r else unsaved_issue_ids << issue.id end end # ... end def extract_changed_attributes_for_move(params) changed_attributes = {} [:assigned_to_id, :status_id, :start_date, :due_date].each do |valid_attribute| unless params[valid_attribute].blank? changed_attributes[valid_attribute] = (params[valid_attribute] == 'none' ? nil : params[valid_attribute]) end end changed_attributes end end |
This refactoring helps to remove some of the smells that metric_fu found with #move
. It also let me remove the changed_attributes
local variable, since it’s only used once outside of extract_changed_attributes_for_move
.