Daily Refactor #44: Extract Method in Kanban.

My StuffToDo plugin has now been improved and refactored quite a bit so it’s time to move onto a different plugin. Since I’m starting to do a lot of work in the Kanban plugin again, refactoring that plugin would be extremely useful.

The Refactoring

The first step I use when refactoring a new codebase is to use extract method to remove any copy and paste duplication. Kanban suffers from this in a few places, probably because of the rapid prototyping that was done early on.

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
33
34
35
36
37
38
39

# app/models/kanban.rb
class Kanban
  def get_backlog_issues(exclude_ids=[])
    return [[]] if missing_settings('backlog')
 
    conditions = ARCondition.new
    conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
    conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?
 
    issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => conditions.conditions)
 
    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position  b[0].position # Sorted based on IssuePriority#position
    }
  end
 
  # TODO: similar to backlog issues
  def get_quick_issues
    return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')
 
    issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})
 
    # Returns as nested arrays
    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position  b[0].position # Sorted based on IssuePriority#position
    }
  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
29
30
31
32
33
34
35
36
37
38

  def get_backlog_issues(exclude_ids=[])
    return [[]] if missing_settings('backlog')
 
    conditions = ARCondition.new
    conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
    conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?
 
    issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => conditions.conditions)
 
    return group_by_priority_position(issues)
  end
 
  # TODO: similar to backlog issues
  def get_quick_issues
    return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')
 
    issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})
 
    return group_by_priority_position(issues)
  end
 
  private
 
  # Sort and group a set of issues based on IssuePriority#position
  def group_by_priority_position(issues)
    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position  b[0].position
    }
  end
end

Review

Now that the grouping method has been extracted, we can see some more duplication in the finders of these methods. And if you look at the entire class, there are even more methods that are very similar to #get_backlog_issues and #get_quick_issues. Refactoring those methods will be the next things I do.

Reference commit