In yesterday's post I used Replace Data Value with Object to start converting much of the Kanban plugin's code to be more object oriented. I finished up that refactoring in some other commits if you're interested in how it went.

The Refactoring

As a result of this refactoring, I was left with a bunch of wrapper methods that would just delgate method calls over to the new KanbanPane classes. This made sense when the main Kanban class was managing the data but now it's just overhead. Using Remove Middle Man, I threw away a lot of those wrapper methods and accessed the KanbanPane directly.

Before

# app/models/kanban.rb
class Kanban
  def self.find
    kanban = Kanban.new
    kanban.settings = Setting.plugin_redmine_kanban
    kanban.users = kanban.get_users
    kanban.incoming_issues = kanban.get_incoming_issues
    kanban.quick_issues = kanban.get_quick_issues
    kanban.backlog_issues = kanban.get_backlog_issues(kanban.quick_issues.to_a.flatten.collect(&:id))
    kanban.selected_issues = KanbanIssue.find_selected
    kanban.active_issues = kanban.get_active
    kanban.testing_issues = kanban.get_testing
    kanban.finished_issues = kanban.get_finished_issues
    kanban.canceled_issues = kanban.get_canceled_issues
    kanban
  end

  def get_incoming_issues
    get_issues_for_pane(:incoming)
  end

  def get_backlog_issues(exclude_ids=[])
    get_issues_for_pane(:backlog, :exclude_ids => exclude_ids)
  end

  def get_quick_issues
    get_issues_for_pane(:quick)
  end

  def get_finished_issues
    get_issues_for_pane(:finished)
  end

  def get_canceled_issues
    get_issues_for_pane(:canceled)
  end

  def get_active
    KanbanPane::ActivePane.new.get_issues(:users => @users)
  end

  def get_testing
    KanbanPane::TestingPane.new.get_issues(:users => @users)
  end
  
  private

  def get_issues_for_pane(pane, options = {})
    case pane
    when :finished
      KanbanPane::FinishedPane.new.get_issues(options)
    when :canceled
      KanbanPane::CanceledPane.new.get_issues(options)
    when :quick
      KanbanPane::QuickPane.new.get_issues(options)
    when :backlog
      KanbanPane::BacklogPane.new.get_issues(options)
    when :incoming
      KanbanPane::IncomingPane.new.get_issues(options)
    else
      return [[]]
    end
    
  end
end

After

# app/models/kanban.rb
class Kanban
  attr_reader :incoming_pane, :backlog_pane, :quick_pane, :canceled_pane, :finished_pane, :active_pane, :testing_pane

  def initialize
    @incoming_pane = KanbanPane::IncomingPane.new
    @backlog_pane = KanbanPane::BacklogPane.new
    @quick_pane = KanbanPane::QuickPane.new
    @canceled_pane = KanbanPane::CanceledPane.new
    @finished_pane = KanbanPane::FinishedPane.new
    @active_pane = KanbanPane::ActivePane.new
    @testing_pane = KanbanPane::TestingPane.new
  end

  def self.find
    kanban = Kanban.new
    kanban.settings = Setting.plugin_redmine_kanban
    kanban.users = kanban.get_users
    kanban.incoming_issues = kanban.incoming_pane.get_issues
    kanban.quick_issues = kanban.quick_pane.get_issues
    kanban.backlog_issues = kanban.backlog_pane.get_issues(:exclude_ids => kanban.quick_issue_ids)
    kanban.selected_issues = KanbanIssue.find_selected
    kanban.active_issues = kanban.active_pane.get_issues(:users => kanban.users)
    kanban.testing_issues = kanban.testing_pane.get_issues(:users => kanban.users)
    kanban.finished_issues = kanban.finished_pane.get_issues
    kanban.canceled_issues = kanban.canceled_pane.get_issues
    kanban
  end

  # No more "get_incoming_issues", "get_backlog_issues", "get_quick_issues",
  # "get_finished_issues", "get_canceled_issues", "get_active", "get_testing",
  # or "get_issues_for_pane"

end

Review

By making Kanban manage it's own KanbanPane, it becomes easy to change Kanban#find to use those panes directly to get the issues on them. This removes at least two levels of delegation and also makes the class more flexible.

Next I'd like to add a new feature I've been wanting, lazy loading the issues for each pane.

Reference commit

Tagged: ruby redmine refactoring remove-middle-man

I've made a mistake.

The last few refactorings I've been doing to the Kanban plugin have been useful but they haven't really been improving the design of the plugin. I thought about it yesterday and discovered that I was just swapping out bits of procedural code for more procedural code. Once I saw this, I found a way to convert a lot of the procedural code into more object oriented code using Replace Data Value with Object.

The Refactoring

This refactoring creates a new class to store the behavior for one of the Kanban panes.

Before

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    # ...
    when :incoming
      return [[]] if missing_settings('incoming')
    
      conditions.add ["status_id = ?", @settings['panes']['incoming']['status']]

      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => conditions.conditions)
    # ...
  end
end

After

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    # ...
    when :incoming
      KanbanPane::IncomingPane.new.get_issues(options)
    # ...
  end
end
# app/models/kanban_pane.rb
class KanbanPane
  def settings
    Setting.plugin_redmine_kanban
  end

  def get_issues(options={})
    nil
  end

  private

  # TODO: Wrapper until moved from Kanban
  def missing_settings(pane, options={})
    kanban = Kanban.new
    kanban.settings = Setting.plugin_redmine_kanban
    kanban.send(:missing_settings, pane, options)
  end
end
# app/models/kanban_pane/incoming_pane.rb
class KanbanPane::IncomingPane < KanbanPane
  def get_issues(options={})
    return [[]] if missing_settings('incoming')
    
    conditions = ARCondition.new
    conditions.add ["status_id = ?", settings['panes']['incoming']['status']]

    return Issue.visible.find(:all,
                              :limit => settings['panes']['incoming']['limit'],
                              :order => "#{Issue.table_name}.created_on ASC",
                              :conditions => conditions.conditions)

  end
  
end

Review

This is the kind of refactoring I've been looking for. By creating a new set of classes (KanbanPane, IncomingPane), I can start to advantage of Ruby's object orientation. Once I finish extracting the other panes out from Kanban#get_issues_for_pane, I can start to move all of the other Pane behavior over to KanbanPane. This is going to help out in the long term, since I'm planning to add a few more Panes to Kanban in the future.

(I'm going to go ahead and do the other pane extractions today, so there will be some fresh code to look at tomorrow.)

Reference commit

Tagged: ruby redmine refactoring replace-data-value-with-object

The Refactoring

In order to remove the duplication in Kanban#get_issues_for_pane, I need to first consolidate some of the duplication in each case statement. This refactoring pulls the ActiveRecord conditions out of the finder and into the ARCondition object.

Before

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    case pane
    when :finished, :canceled
    
      return [[]] if missing_settings(pane.to_s)

      status_id = @settings['panes'][pane.to_s]['status']
      days = @settings['panes'][pane.to_s]['limit'] || 7
      
      issues = Issue.visible.all(:include => :assigned_to,
                                 :order => "#{Issue.table_name}.updated_on DESC",
                                 :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", status_id, days.to_f.days.ago])

      return issues.group_by(&:assigned_to)
      
    when :quick
      return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

      issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})

      return group_by_priority_position(issues)
      
    when :backlog
      return [[]] if missing_settings('backlog')

      exclude_ids = options.delete(:exclude_ids)
      
      conditions = ARCondition.new
      conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
      conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

      issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => conditions.conditions)

      return group_by_priority_position(issues)
      
    when :incoming
      return [[]] if missing_settings('incoming')
    
      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => {:status_id => @settings['panes']['incoming']['status']})
    else
      return [[]]
    end
    
  end

end

After

# app/models/kanban.rb
class Kanban
  def get_issues_for_pane(pane, options = {})
    conditions = ARCondition.new

    case pane
    when :finished, :canceled
    
      return [[]] if missing_settings(pane.to_s)

      status_id = @settings['panes'][pane.to_s]['status']
      days = @settings['panes'][pane.to_s]['limit'] || 7

      conditions.add ["#{Issue.table_name}.status_id = ?", status_id]
      conditions.add ["#{Issue.table_name}.updated_on > ?", days.to_f.days.ago]
      
      issues = Issue.visible.all(:include => :assigned_to,
                                 :order => "#{Issue.table_name}.updated_on DESC",
                                 :conditions => conditions.conditions)

      return issues.group_by(&:assigned_to)
      
    when :quick
      return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

      conditions.add ["status_id = ?", @settings['panes']['backlog']['status']]
      conditions.add "estimated_hours IS null"

      issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => conditions.conditions)

      return group_by_priority_position(issues)
      
    when :backlog
      return [[]] if missing_settings('backlog')

      exclude_ids = options.delete(:exclude_ids)
      
      conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
      conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

      issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => conditions.conditions)

      return group_by_priority_position(issues)
      
    when :incoming
      return [[]] if missing_settings('incoming')
    
      conditions.add ["status_id = ?", @settings['panes']['incoming']['status']]

      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => conditions.conditions)
    else
      return [[]]
    end
  end
end

Review

With the conditions consolidated, some of the duplication is now easy to see: like all of the case statements are adding a condition based on status_id. In a few more steps, #get_issues_for_pane will end up as a simple query builder.

Reference commit

Tagged: ruby redmine refactoring consolidate-duplicate-conditional-fragments

Here is the third refactoring to clean up how the Kanban class gets it's pane data.

The Refactoring

This refactoring is brutal to read through but it's just moving the body of #get_incoming_issues, #get_backlog_issues, and #get_quick_issues into #get_issues_for_pane.

Before

# app/models/kanban.rb
class Kanban
  def get_incoming_issues
    return [[]] if missing_settings('incoming')
    
    return Issue.visible.find(:all,
                              :limit => @settings['panes']['incoming']['limit'],
                              :order => "#{Issue.table_name}.created_on ASC",
                              :conditions => {:status_id => @settings['panes']['incoming']['status']})
  end

  def get_backlog_issues(exclude_ids=[])
    return [[]] if missing_settings('backlog')
    
    conditions = ARCondition.new
    conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
    conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

    issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => conditions.conditions)

    return group_by_priority_position(issues)
  end

  # TODO: similar to backlog issues
  def get_quick_issues
    return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

    issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})

    return group_by_priority_position(issues)
  end

  private

  def get_issues_for_pane(pane)
    return [[]] unless [:finished, :canceled].include?(pane)
    return [[]] if missing_settings(pane.to_s)

    status_id = @settings['panes'][pane.to_s]['status']
    days = @settings['panes'][pane.to_s]['limit'] || 7
    
    issues = Issue.visible.all(:include => :assigned_to,
                               :order => "#{Issue.table_name}.updated_on DESC",
                               :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", status_id, days.to_f.days.ago])

    return issues.group_by(&:assigned_to)
  end

end

After

# app/models/kanban.rb
class Kanban
  def get_incoming_issues
    get_issues_for_pane(:incoming)
  end

  def get_backlog_issues(exclude_ids=[])
    get_issues_for_pane(:backlog, :exclude_ids => exclude_ids)
  end

  def get_quick_issues
    get_issues_for_pane(:quick)
  end

  private

  def get_issues_for_pane(pane, options = {})
    case pane
    when :finished, :canceled
    
      return [[]] if missing_settings(pane.to_s)

      status_id = @settings['panes'][pane.to_s]['status']
      days = @settings['panes'][pane.to_s]['limit'] || 7
      
      issues = Issue.visible.all(:include => :assigned_to,
                                 :order => "#{Issue.table_name}.updated_on DESC",
                                 :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", status_id, days.to_f.days.ago])

      return issues.group_by(&:assigned_to)
      
    when :quick
      return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

      issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})

      return group_by_priority_position(issues)
      
    when :backlog
      return [[]] if missing_settings('backlog')

      exclude_ids = options.delete(:exclude_ids)
      
      conditions = ARCondition.new
      conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
      conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

      issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                                 :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                                 :include => :priority,
                                 :conditions => conditions.conditions)

      return group_by_priority_position(issues)
      
    when :incoming
      return [[]] if missing_settings('incoming')
    
      return Issue.visible.find(:all,
                                :limit => @settings['panes']['incoming']['limit'],
                                :order => "#{Issue.table_name}.created_on ASC",
                                :conditions => {:status_id => @settings['panes']['incoming']['status']})
    else
      return [[]]
    end
    
  end

end

Review

Now that all of the extractions are done, all of the Kanban#get_* methods are using either #get_issues_for_panes or #issues_from_kanban_issue. Though this just shuffled statements around, it really cleared up how the public methods behave. Now I can dig into #get_issues_for_panes and #issues_from_kanban_issue and remove the duplication in them.

Reference commit

Tagged: ruby redmine refactoring extract-method

About 90% of my development is done on Redmine and well over half of that is done on Redmine plugins. That means that I write tests; a lot of tests. Over time though, they get slower and slower and start to become a drain on productivity ("5 minutes to run the tests on a feature that took 30 seconds?!?!").

The limiting factor is that Rails runs tests sequentially, so it only takes advantage of a single CPU core. I've started to use a great tool called parallel_tests to use all four cores but it hasn't been working on tests for Rails plugins' due to how it loads.

Until now...

With a bit of hacking, I've figured out how to load parallel_tests inside my plugin's test suite. The setup was actually really simple:

  1. Setup parallel_tests just like in the Readme, but stop at step 4.
  2. Load the parallel_test rake file into your plugin (below)
  3. Run the tests

Loading parallel_tests

Just pop this into your plugin's Rakefile (assuming you installed the plugin version of parallel_tests)

parallel_tests = (File.join(File.dirname(__FILE__), '..', 'parallel_tests','lib','tasks','parallel_tests.rake'))
if File.exists? parallel_tests
  RAILS_ROOT = File.dirname(__FILE__)
  import parallel_tests
end

Using rake's import statement, the parallel_tests.rake file gets loaded directly into the plugin's Rakefile. This makes the rake parallel tasks available.

I also had to define RAILS_ROOT because I monkey around with it in my plugin. You may be able to remove it in your plugin.

Results

I set this up for my redmine_kanban plugin and it cut my test suite's running time from 4 minutes down to 2 minutes 38 seconds, a savings of 1 minute 22 seconds. That's a massive improvement when you start running the test suite 10, 20, or 50 times a day.

$ time rake
(in /home/edavis/dev/redmine/redmine-core/vendor/plugins/redmine_kanban)
/usr/bin/ruby1.8 -I"lib:test" "/usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/unit/kanban_issue_test.rb" "test/unit/helpers/kanbans_helper_test.rb" "test/unit/issue_test.rb" "test/unit/sanity_test.rb" "test/unit/kanban_test.rb"
  * DEFERRED: #update_issue_attributes should return false if the issue didn't save.
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
...................................................................................
Finished in 63.503572 seconds.

83 tests, 267 assertions, 0 failures, 0 errors
/usr/bin/ruby1.8 -I"lib:test" "/usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/functional/kanbans_controller_test.rb"
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
................................
Finished in 139.429962 seconds.

32 tests, 79 assertions, 0 failures, 0 errors
/usr/bin/ruby1.8 -I"lib:test" "/usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/integration/kanban_board_test.rb"
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
..
Finished in 9.012205 seconds.

2 tests, 12 assertions, 0 failures, 0 errors

real    3m59.730s
user    3m32.413s
sys     0m21.733s
$ time rake parallel:test
(in /home/edavis/dev/redmine/redmine-core/vendor/plugins/redmine_kanban)
/home/edavis/dev/redmine/redmine-core/vendor/plugins/parallel_tests/lib/tasks/../parallel_tests.rb:4: warning: already initialized constant VERSION
4 processes for 7 tests, ~ 1 tests per process
Loaded suite -e
Started
.  * DEFERRED: #update_issue_attributes should return false if the issue didn't save.
Loaded suite -e
Started
.Loaded suite -e
Started
.Loaded suite -e
Started
.............................................
Finished in 14.261201 seconds.

31 tests, 56 assertions, 0 failures, 0 errors
...
Finished in 14.744914 seconds.

14 tests, 43 assertions, 0 failures, 0 errors
........................................
Finished in 52.973448 seconds.

41 tests, 180 assertions, 0 failures, 0 errors
...........................
Finished in 147.211034 seconds.

32 tests, 79 assertions, 0 failures, 0 errors

Results:
41 tests, 180 assertions, 0 failures, 0 errors
14 tests, 43 assertions, 0 failures, 0 errors
31 tests, 56 assertions, 0 failures, 0 errors
32 tests, 79 assertions, 0 failures, 0 errors

Took 156.279382 seconds

real    2m38.223s
user    0m1.600s
sys     0m0.416s

Don't settle for Rails only using one core, put your quad core behemoth back to work.

Eric Davis

Tagged: ruby rails testing optimization

This is the second in a series of refactorings to clean up how the Kanban class gets it's pane data.

The Refactoring

Using extract method again, I was able to merge #get_active and #get_testing so they both use a new method #issues_from_kanban_issue.

Before

# app/models/kanban.rb
class Kanban
  def get_active
    active = {}
    @users.each do |user|
      active[user] = KanbanIssue.find_active(user.id)
    end unless @users.blank?
    active
  end

  def get_testing
    testing = {}
    @users.each do |user|
      testing[user] = KanbanIssue.find_testing(user.id)
    end unless @users.blank?
    testing
  end
end

After

# app/models/kanban.rb
class Kanban
  def get_active
    issues_from_kanban_issue(:active)
  end

  def get_testing
    issues_from_kanban_issue(:testing)
  end

  private
  
  def issues_from_kanban_issue(pane)
    return {} unless [:active, :testing].include?(pane)
    
    issues = {}
    @users.each do |user|
      issues[user] = KanbanIssue.send('find_' + pane.to_s, user.id)
    end unless @users.blank?
    issues

  end
end

Review

This gets me one step closer to uniting all of the Kanban#get_* methods.

Reference commit

Tagged: ruby redmine refactoring extract-method

I'm starting on some major internal refactoring of the Kanban class now. There are seven core methods that it uses and they all contain a lot of copy and paste duplication. Today I'm starting on the Finished (#get_finished_issues) and Canceled (#get_canceled_issues) panes since they are almost mirror images.

The Refactoring

I used extract method with a few tweaks to handle the local variables in each method that come from the settings.

Before

# app/models/kanban.rb
class Kanban
  def get_finished_issues
    return [[]] if missing_settings('finished')
    
    days = @settings['panes']['finished']['limit'] || 7
    issues = Issue.visible.all(:include => :assigned_to,
                               :order => "#{Issue.table_name}.updated_on DESC",
                               :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", @settings['panes']['finished']['status'], days.to_f.days.ago])

    return issues.group_by(&:assigned_to)
  end

  def get_canceled_issues
    return [[]] if missing_settings('canceled')
    
    days = @settings['panes']['canceled']['limit'] || 7
    issues = Issue.visible.all(:include => :assigned_to,
                               :order => "#{Issue.table_name}.updated_on DESC",
                               :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", @settings['panes']['canceled']['status'], days.to_f.days.ago])

    return issues.group_by(&:assigned_to)
  end
end

After

# app/models/kanban.rb
class Kanban
  def get_finished_issues
    get_issues_for_pane(:finished)
  end

  def get_canceled_issues
    get_issues_for_pane(:canceled)
  end

  private

  def get_issues_for_pane(pane)
    return [[]] unless [:finished, :canceled].include?(pane)
    return [[]] if missing_settings(pane.to_s)

    status_id = @settings['panes'][pane.to_s]['status']
    days = @settings['panes'][pane.to_s]['limit'] || 7
    
    issues = Issue.visible.all(:include => :assigned_to,
                               :order => "#{Issue.table_name}.updated_on DESC",
                               :conditions => ["#{Issue.table_name}.status_id = ? AND #{Issue.table_name}.updated_on > ?", status_id, days.to_f.days.ago])

    return issues.group_by(&:assigned_to)
  end
end

Review

Now that all of the logic for #get_finished_issues and #get_canceled_issues has been extracted, they now read at a very high level. This also makes me wonder if I can extract the other get_pane methods into #get_issues_for_pane. That way, everything will be isolated in one method instead of duplicated across seven different methods.

Reference commit

Tagged: ruby redmine refactoring extract-method

My StuffToDo plugin has now been improved and refactored quite a bit so it's time to move onto a different plugin. Since I'm starting to do a lot of work in the Kanban plugin again, refactoring that plugin would be extremely useful.

The Refactoring

The first step I use when refactoring a new codebase is to use extract method to remove any copy and paste duplication. Kanban suffers from this in a few places, probably because of the rapid prototyping that was done early on.

Before

# app/models/kanban.rb
class Kanban
  def get_backlog_issues(exclude_ids=[])
    return [[]] if missing_settings('backlog')
    
    conditions = ARCondition.new
    conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
    conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

    issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => conditions.conditions)

    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position <=> b[0].position # Sorted based on IssuePriority#position
    }
  end

  # TODO: similar to backlog issues
  def get_quick_issues
    return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

    issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})

    # Returns as nested arrays
    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position <=> b[0].position # Sorted based on IssuePriority#position
    }
  end

end

After

  def get_backlog_issues(exclude_ids=[])
    return [[]] if missing_settings('backlog')
    
    conditions = ARCondition.new
    conditions.add ["#{Issue.table_name}.status_id IN (?)", @settings['panes']['backlog']['status']]
    conditions.add ["#{Issue.table_name}.id NOT IN (?)", exclude_ids] unless exclude_ids.empty?

    issues = Issue.visible.all(:limit => @settings['panes']['backlog']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => conditions.conditions)

    return group_by_priority_position(issues)
  end

  # TODO: similar to backlog issues
  def get_quick_issues
    return [[]] if missing_settings('quick-tasks', :skip_status => true) || missing_settings('backlog')

    issues = Issue.visible.all(:limit => @settings['panes']['quick-tasks']['limit'],
                               :order => "#{RedmineKanban::KanbanCompatibility::IssuePriority.klass.table_name}.position ASC, #{Issue.table_name}.created_on ASC",
                               :include => :priority,
                               :conditions => {:status_id => @settings['panes']['backlog']['status'], :estimated_hours => nil})

    return group_by_priority_position(issues)
  end

  private

  # Sort and group a set of issues based on IssuePriority#position
  def group_by_priority_position(issues)
    return issues.group_by {|issue|
      issue.priority
    }.sort {|a,b|
      a[0].position <=> b[0].position
    }
  end
end

Review

Now that the grouping method has been extracted, we can see some more duplication in the finders of these methods. And if you look at the entire class, there are even more methods that are very similar to #get_backlog_issues and #get_quick_issues. Refactoring those methods will be the next things I do.

Reference commit

Tagged: ruby redmine refactoring extract-method

The Refactoring

Today I used extract method to start cleaning up the new TimeGrid code I recently merged into the StuffToDo plugin.

Before

# app/controllers/stuff_to_do_controller.rb
class StuffToDoController < ApplicationController
  def get_time_grid
    @date = Date.parse(params[:date]) if params[:date]
    @date ||= Date.civil(params[:year].to_i, params[:month].to_i, params[:day].to_i) if params[:year] && params[:month] && params[:day]
    @date ||= Date.today
    
    @calendar = Redmine::Helpers::Calendar.new(@date, current_language, :week)
    @issues = User.current.time_grid_issues.visible.all(:order => "#{Issue.table_name}.id ASC")
    @time_entry = TimeEntry.new
  end
end

After

# app/controllers/stuff_to_do_controller.rb
class StuffToDoController < ApplicationController
  def get_time_grid
    @date = parse_date_from_params
    @calendar = Redmine::Helpers::Calendar.new(@date, current_language, :week)
    @issues = User.current.time_grid_issues.visible.all(:order => "#{Issue.table_name}.id ASC")
    @time_entry = TimeEntry.new
  end

  def parse_date_from_params
    date = Date.parse(params[:date]) if params[:date]
    date ||= Date.civil(params[:year].to_i, params[:month].to_i, params[:day].to_i) if params[:year] && params[:month] && params[:day]
    date ||= Date.today
  end
end

Review

This was a simple refactoring which helps to separate the code into better parts. The three lines to parse the date are needed by the TimeGrid, but it isn't a core part of how the TimeGrid so it's better to have it in a utility method.

Reference commit

Tagged: ruby redmine refactoring extract-method

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

# app/models/stuff_to_do.rb
class StuffToDo < ActiveRecord::Base
  def self.available(user, filter = { })
    if filter.nil? || filter.empty?
      return []
    elsif filter[:user]
      user = filter[:user]
      issues = Issue.find(:all,
                          :include => :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

class StuffToDo < ActiveRecord::Base
  def self.available(user, filter=nil)
    return [] if filter.blank?

    if filter.is_a?(Project)
      # TODO: remove 'issues' naming
      issues = active_and_visible_projects.sort
    else
      issues = Issue.find(:all,
                          :include => [: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

Tagged: ruby redmine refactoring