Daily Refactor #48: Consolidate Duplicate Conditional Fragments in Kanban

The Refactoring

In order to remove the duplication in Kanban#get_issues_for_pane, I need to first consolidate some of the duplication in each case statement. This refactoring pulls the ActiveRecord conditions out of the finder and into the ARCondition object.

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
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    case pane
    when :finished, :canceled
 
      return [[]] if missing_settings(pane.to_s)
 
      status_id = @settings['panes'][pane.to_s]['status']
      days = @settings['panes'][pane.to_s]['limit'] || 7
 
      issues = Issue.visible.all(:include => :assigned_to,
                                 :order => "#{Issue.table_name}.updated_on DESC",
                                 :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", status_id, days.to_f.days.ago])
 
      return issues.group_by(&:assigned_to)
 
    when :quick
      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)
 
    when :backlog
      return [[]] if missing_settings('backlog')
 
      exclude_ids = options.delete(:exclude_ids)
 
      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)
 
    when :incoming
      return [[]] if missing_settings('incoming')
 
      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => {:status_id => @settings['panes']['incoming']['status']})
    else
      return [[]]
    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
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    conditions = ARCondition.new
 
    case pane
    when :finished, :canceled
 
      return [[]] if missing_settings(pane.to_s)
 
      status_id = @settings['panes'][pane.to_s]['status']
      days = @settings['panes'][pane.to_s]['limit'] || 7
 
      conditions.add ["#{Issue.table_name}.status_id = ?", status_id]
      conditions.add ["#{Issue.table_name}.updated_on > ?", days.to_f.days.ago]
 
      issues = Issue.visible.all(:include => :assigned_to,
                                 :order => "#{Issue.table_name}.updated_on DESC",
                                 :conditions => conditions.conditions)
 
      return issues.group_by(&:assigned_to)
 
    when :quick
      return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')
 
      conditions.add ["status_id = ?", @settings['panes']['backlog']['status']]
      conditions.add "estimated_hours IS null"
 
      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 => conditions.conditions)
 
      return group_by_priority_position(issues)
 
    when :backlog
      return [[]] if missing_settings('backlog')
 
      exclude_ids = options.delete(:exclude_ids)
 
      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)
 
    when :incoming
      return [[]] if missing_settings('incoming')
 
      conditions.add ["status_id = ?", @settings['panes']['incoming']['status']]
 
      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => conditions.conditions)
    else
      return [[]]
    end
  end
end

Review

With the conditions consolidated, some of the duplication is now easy to see: like all of the case statements are adding a condition based on status_id. In a few more steps, #get_issues_for_pane will end up as a simple query builder.

Reference commit