Daily Refactor #9: Move Issue#by methods

Yesterday’s refactoring to the ReportsController helped to move some of the ActiveRecord::Base#select_all methods out of the controller but I was still left with the duplication in the Issue model. Today, I’ll dig into those model methods and remove most of the duplication.

The Refactoring

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
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85

# app/models/issue.rb
class Issue < ActiveRecord::Base
  def self.by_tracker(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                t.id as tracker_id,
                                                count(i.id) as total 
                                              from 
                                                  #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t
                                              where 
                                                i.status_id=s.id 
                                                and i.tracker_id=t.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, t.id")
  end
 
  def self.by_version(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                v.id as fixed_version_id,
                                                count(i.id) as total 
                                              from 
                                                #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v
                                              where 
                                                i.status_id=s.id 
                                                and i.fixed_version_id=v.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, v.id")
  end
 
  def self.by_priority(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                p.id as priority_id,
                                                count(i.id) as total 
                                              from 
                                                #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p
                                              where 
                                                i.status_id=s.id 
                                                and i.priority_id=p.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, p.id")
  end
 
  def self.by_category(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                c.id as category_id,
                                                count(i.id) as total 
                                              from 
                                                #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c
                                              where 
                                                i.status_id=s.id 
                                                and i.category_id=c.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, c.id")
  end
 
  def self.by_assigned_to(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                a.id as assigned_to_id,
                                                count(i.id) as total 
                                              from 
                                                #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
                                              where 
                                                i.status_id=s.id 
                                                and i.assigned_to_id=a.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, a.id")
  end
 
  def self.by_author(project)
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                a.id as author_id,
                                                count(i.id) as total 
                                              from 
                                                #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a
                                              where 
                                                i.status_id=s.id 
                                                and i.author_id=a.id
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, a.id")    
  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
65
66

# app/models/issue.rb
class Issue  project,
                       :field => 'tracker_id',
                       :joins => Tracker.table_name)
  end
 
  def self.by_version(project)
    count_and_group_by(:project => project,
                       :field => 'fixed_version_id',
                       :joins => Version.table_name)
  end
 
  def self.by_priority(project)
    count_and_group_by(:project => project,
                       :field => 'priority_id',
                       :joins => IssuePriority.table_name)
  end
 
  def self.by_category(project)
    count_and_group_by(:project => project,
                       :field => 'category_id',
                       :joins => IssueCategory.table_name)
  end
 
  def self.by_assigned_to(project)
    count_and_group_by(:project => project,
                       :field => 'assigned_to_id',
                       :joins => User.table_name)
  end
 
  def self.by_author(project)
    count_and_group_by(:project => project,
                       :field => 'author_id',
                       :joins => User.table_name)
  end
 
  # ...
 
  private
  # Query generator for selecting groups of issue counts for a project
  # based on specific criteria
  #
  # Options
  # * project - Project to search in.
  # * field - String. Issue field to key off of in the grouping.
  # * joins - String. The table name to join against.
  def self.count_and_group_by(options)
    project = options.delete(:project)
    select_field = options.delete(:field)
    joins = options.delete(:joins)
 
    where = "i.#{select_field}=j.id"
 
    ActiveRecord::Base.connection.select_all("select    s.id as status_id, 
                                                s.is_closed as closed, 
                                                j.id as #{select_field},
                                                count(i.id) as total 
                                              from 
                                                  #{Issue.table_name} i, #{IssueStatus.table_name} s, #{joins} as j
                                              where 
                                                i.status_id=s.id 
                                                and #{where}
                                                and i.project_id=#{project.id}
                                              group by s.id, s.is_closed, j.id")
  end
end

Review

The big duplication was from the 6 select_all statements that only differed in what fields are used and what table to join. So I moved the general code to a separate method, provided parameters for the parts of the statement that would change, and built the select_all statement based on a simpler set of options. This allowed me to slim down the public methods considerably.

So now it would be trivial to add new public methods to count and group based on other fields. Also if the SQL needs to be rewritten at any point in the future, it only needs to be done in one place, not six.

This is as far as I want to take the model refactoring at this time. I’ll be heading back up to the controllers to refactor some more duplication up there. Duplication in controllers will affect an application more than duplication in models because a user tends to touch the controllers directly.

Reference commit