written by Eric Davis on April 07, 2010
0 Comments
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
written by Eric Davis on April 06, 2010
0 Comments
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
written by Eric Davis on April 05, 2010
0 Comments
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
written by Eric Davis on April 02, 2010
0 Comments
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
written by Eric Davis on April 01, 2010
0 Comments
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:
- Setup parallel_tests just like in the Readme, but stop at step 4.
- Load the parallel_test rake file into your plugin (below)
- 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
written by Eric Davis on April 01, 2010
0 Comments
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
written by Eric Davis on March 31, 2010
0 Comments
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
written by Eric Davis on March 30, 2010
0 Comments
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
written by Eric Davis on March 29, 2010
0 Comments
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
written by Eric Davis on March 26, 2010
0 Comments
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
