Daily Refactor #42: Consolidate Conditionals in StuffToDo#available

Last night I merged a large work in progress branch into StuffToDo so the refactoring I wanted to do wasn’t available anymore. But there’s always a replacement to be found (aka: always some bad code to fix).

The Refactoring

A few days ago I did some refactoring to StuffToDo#available in order to remove some of the duplication. Today I finally got sick of looking at the remaining duplication and overhauled the method’s logic.

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

# app/models/stuff_to_do.rb
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
 
  def self.conditions_for_available(filter_by, record_id)
    conditions_builder = ARCondition.new(["#{IssueStatus.table_name}.is_closed = ?", false ])
 
    case filter_by
    when :user
      conditions_builder.add(["assigned_to_id = ?", record_id])
    when :status
      conditions_builder.add(["#{IssueStatus.table_name}.id = (?)", record_id])
    when :priority
      conditions_builder.add(["#{Enumeration.table_name}.id = (?)", record_id])
    end
 
    conditions_builder.conditions
  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

class StuffToDo  [:status, :priority],
                          :conditions => conditions_for_available(filter, filter.id),
                          :order => 'created_on DESC')
    end
 
    next_issues = StuffToDo.find(:all, :conditions => { :user_id => user.id }).collect(&:stuff)
 
    return issues - next_issues
  end
 
  def self.conditions_for_available(filter_by, record_id)
    conditions_builder = ARCondition.new(["#{IssueStatus.table_name}.is_closed = ?", false ])
 
    case 
    when filter_by.is_a?(User)
      conditions_builder.add(["assigned_to_id = ?", record_id])
    when filter_by.is_a?(IssueStatus)
      conditions_builder.add(["#{IssueStatus.table_name}.id = (?)", record_id])
    when filter_by.is_a?(Enumeration)
      conditions_builder.add(["#{Enumeration.table_name}.id = (?)", record_id])
    end
 
    conditions_builder.conditions
  end
end

Review

The major change was to switch from using a Hash as the input for the “Record type” and “Record” to just using the Record itself. #available only checked the record type to figure out what to pass to #conditions_for_available. But each record already has it’s type embedded into it (called a Class) so this was just extra data. Now #available only branches if the record is a Project, otherwise it ships the data down to #conditions_for_available.

Also note that this did change #available‘s method signature. I didn’t include the changes in the caller but it also made things a lot simpler there too. If you’re interested, the commit shows the controller changes.

Reference commit