Daily Refactor #45: Extract Method for Kanban panes – Part 1

I’m starting on some major internal refactoring of the Kanban class now. There are seven core methods that it uses and they all contain a lot of copy and paste duplication. Today I’m starting on the Finished (#get_finished_issues) and Canceled (#get_canceled_issues) panes since they are almost mirror images.

The Refactoring

I used extract method with a few tweaks to handle the local variables in each method that come from the settings.

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

# app/models/kanban.rb
class Kanban
  def get_finished_issues
    return [[]] if missing_settings('finished')
 
    days = @settings['panes']['finished']['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 > ?", @settings['panes']['finished']['status'], days.to_f.days.ago])
 
    return issues.group_by(&:assigned_to)
  end
 
  def get_canceled_issues
    return [[]] if missing_settings('canceled')
 
    days = @settings['panes']['canceled']['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 > ?", @settings['panes']['canceled']['status'], days.to_f.days.ago])
 
    return issues.group_by(&:assigned_to)
  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

# app/models/kanban.rb
class Kanban
  def get_finished_issues
    get_issues_for_pane(:finished)
  end
 
  def get_canceled_issues
    get_issues_for_pane(:canceled)
  end
 
  private
 
  def get_issues_for_pane(pane)
    return [[]] unless [:finished, :canceled].include?(pane)
    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)
  end
end

Review

Now that all of the logic for #get_finished_issues and #get_canceled_issues has been extracted, they now read at a very high level. This also makes me wonder if I can extract the other get_pane methods into #get_issues_for_pane. That way, everything will be isolated in one method instead of duplicated across seven different methods.

Reference commit