Daily Refactor #9: Move Issue#by methods

By on February 4, 2010 in General

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

Share

  • Facebook
  • Twitter
  • HackerNews
  • Reddit
  • Tumblr
  • Delicious
  • Email
  • RSS

Related posts:

  1. Daily Refactor #81: Move last_invoice_number to Model
  2. Daily Refactor #55: Move Pane Configuration To KanbanPane
  3. Daily Refactor #54: Move Method to KanbanPane
  4. Daily Refactor #31: Extract Method in IssuesHelper#show_detail
  5. Daily Refactor #8: Move method in the ReportsController

About Eric Davis

I founded Little Stream Software where I provide Redmine and ChiliProject services to help projects teams. I also created an ebook, Redmine Tips, were I show you how to become more productive using Redmine. I am also the author of Refactoring Redmine, where I go about refactoring Rails using Redmine as an example.
View all posts by Eric Davis →

, , ,

Chirk HR     Reuse your existing job applicants »
WP Socializer Aakash Web