The Refactoring
Working on the flay report for StuffToDo I tackled the #available
method this morning.
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 |
# app/models/stuff_to_do.rb class StuffToDo :status, :conditions => ["assigned_to_id = ? AND #{IssueStatus.table_name}.is_closed = ?",user.id, false ], :order => 'created_on DESC') elsif filter[:status] status = filter[:status] issues = Issue.find(:all, :include => :status, :conditions => ["#{IssueStatus.table_name}.id = (?) AND #{IssueStatus.table_name}.is_closed = ?", status.id, false ], :order => 'created_on DESC') elsif filter[:priority] priority = filter[:priority] issues = Issue.find(:all, :include => [:status, :priority], :conditions => ["#{Enumeration.table_name}.id = (?) AND #{IssueStatus.table_name}.is_closed = ?", priority.id, false ], :order => 'created_on DESC') elsif filter[:projects] # TODO: remove 'issues' naming issues = active_and_visible_projects.sort end next_issues = StuffToDo.find(:all, :conditions => { :user_id => user.id }).collect(&:stuff) return issues - next_issues 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 |
class StuffToDo :status, :conditions => conditions_for_available(:user, user.id), :order => 'created_on DESC') elsif filter[:status] status = filter[:status] issues = Issue.find(:all, :include => :status, :conditions => conditions_for_available(:status, status.id), :order => 'created_on DESC') elsif filter[:priority] priority = filter[:priority] issues = Issue.find(:all, :include => [:status, :priority], :conditions => conditions_for_available(:priority, priority.id), :order => 'created_on DESC') elsif filter[:projects] # TODO: remove 'issues' naming issues = active_and_visible_projects.sort end next_issues = StuffToDo.find(:all, :conditions => { :user_id => user.id }).collect(&:stuff) return issues - next_issues end private def self.conditions_for_available(filter_by, record_id) case filter_by when :user ["assigned_to_id = ? AND #{IssueStatus.table_name}.is_closed = ?", record_id, false ] when :status ["#{IssueStatus.table_name}.id = (?) AND #{IssueStatus.table_name}.is_closed = ?", record_id, false ] when :priority ["#{Enumeration.table_name}.id = (?) AND #{IssueStatus.table_name}.is_closed = ?", record_id, false ] end end end |
Review
I really like these kind of refactorings. They make it easy to see the similar areas of code and make the next refactorings easy. Next I could refactor the entire finder in #available
or this new #conditions_for_available
.