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.