written by Eric Davis on March 25, 2010
0 Comments
The Refactoring
Today I refactored the conditional of StuffToDo#conditions_for_available using a little known library in Redmine called ARCondition.
Before
# app/models/stuff_to_do.rb
class StuffToDo < ActiveRecord::Base
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
After
class StuffToDo < ActiveRecord::Base
def self.conditions_for_available(filter_by, record_id)
conditions_builder = ARCondition.new
conditions_builder.add(["#{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
Review
Using ARCondition, I was able split up how the conditions where being generated. So instead of creating three similar Arrays, I can create separate Arrays for each part of the conditions. Redmine uses this pattern in several places where the conditions need to be generated dynamically.
There is one more refactoring I want to try from the flay report, by combining the Redmine core patches to Issue and Project.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on March 24, 2010
0 Comments
The Refactoring
Working on the flay report for StuffToDo I tackled the #available method this morning.
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 => ["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
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
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.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on March 23, 2010
0 Comments
Since I'm planning to release an update to my StuffToDo plugin next, I decided it could use a week of refactoring in order to clean it up.
The Refactoring
Today I did the second part of the refactoring from yesterday to clean up the StuffToDo plugin's #using_projects_as_items? and #using_issues_as_items? methods. This time I used "substitute algorithm" to refactor the inner logic.
Before
# app/models/stuff_to_do.rb
class StuffToDo < ActiveRecord::Base
def self.using_projects_as_items?
use_setting == 'All' || use_setting == 'Only Projects'
end
def self.using_issues_as_items?
use_setting == 'All' || use_setting == 'Only Issues'
end
end
After
class StuffToDo < ActiveRecord::Base
def self.using_projects_as_items?
['All', 'Only Projects'].include?(use_setting)
end
def self.using_issues_as_items?
['All', 'Only Issues'].include?(use_setting)
end
end
Review
This was a simple refactoring but it really made the intent of the methods clear. Removing the extra method calls and using an Array of valid values will make it easier to modify later. This refactoring also made flay happy enough to report that the duplication has been completely removed.
Reference commit
Tagged: ruby redmine refactoring substitute-algorithm
written by Eric Davis on March 22, 2010
0 Comments
Since I'm planning to release an update to my StuffToDo plugin next, I decided it could use a week of refactoring in order to clean it up.
The Refactoring
Since this plugin doesn't have a lot of code (349 lines), there aren't as many code smells as in the Redmine core. But according to Caliper there are still a few sections that are worse than the community averages. Starting with my trusted friend flay, I found this refactoring.
Before
# app/models/stuff_to_do.rb
class StuffToDo < ActiveRecord::Base
USE = {
'All' => '0',
'Only Issues' => '1',
'Only Projects' => '2'
}
def self.using_projects_as_items?
using = USE.index(Setting.plugin_stuff_to_do_plugin['use_as_stuff_to_do'])
using == 'All' || using == 'Only Projects'
end
def self.using_issues_as_items?
using = USE.index(Setting.plugin_stuff_to_do_plugin['use_as_stuff_to_do'])
using == 'All' || using == 'Only Issues'
end
end
After
class StuffToDo < ActiveRecord::Base
USE = {
'All' => '0',
'Only Issues' => '1',
'Only Projects' => '2'
}
def self.using_projects_as_items?
use_setting == 'All' || use_setting == 'Only Projects'
end
def self.using_issues_as_items?
use_setting == 'All' || use_setting == 'Only Issues'
end
private
def self.use_setting
USE.index(Setting.plugin_stuff_to_do_plugin['use_as_stuff_to_do'])
end
end
Review
This alone wasn't a big enough refactoring for flay, it still sees #using_projects_as_items? and #using_issues_as_items? as similar enough code to flag. I already have the next refactoring planned which should remove that duplication. Do you have an idea of what it could be? Post your idea to the comments below.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on March 19, 2010
0 Comments
The Refactoring
I'm still going off of the flay report for today, this time I'm tackling some duplication in the IssuesController.
Before
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def index
retrieve_query
sort_init(@query.sort_criteria.empty? ? [['id', 'desc']] : @query.sort_criteria)
sort_update({'id' => "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update({'id' => "#{Issue.table_name}.id"}.merge(@query.available_columns.inject({}) {|h, c| h[c.name.to_s] = c.sortable; h}))
# ...
end
end
# app/models/query.rb
class Query < ActiveRecord::Base
# ..
end
After
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def index
retrieve_query
sort_init(@query.sort_criteria.empty? ? [['id', 'desc']] : @query.sort_criteria)
sort_update(@query.sortable_columns)
# ...
end
def changes
retrieve_query
sort_init 'id', 'desc'
sort_update(@query.sortable_columns)
# ...
end
end
# app/models/query.rb
class Query < ActiveRecord::Base
# Returns a Hash of columns and the key for sorting
def sortable_columns
{'id' => "#{Issue.table_name}.id"}.merge(available_columns.inject({}) {|h, column|
h[column.name.to_s] = column.sortable
h
})
end
end
Review
Since the controller code depended on the state of the @query instance, it made sense to move it into the Query model itself. I also split up the single line into multiple lines so it's easier to understand what's going on. I would like to factor out that train wreck but there's not a lot of benefit to that since it's not used that much.
I think I'll be doing my next set of refactorings to some of my Redmine plugins. I have a few code smells in them I want to clean up for the next few releases.
Reference commit
Tagged: ruby redmine refactoring extract-method
A new release of the Redmine Timesheet plugin has been tested and is ready for download. This release includes over 30 changes since the last release including 17 bug fixes and some new features like the CSV export. This will be the final version that is supported on Redmine 0.8, the next versions will require Redmine 0.9.
Download
The plugin can be download from the Little Stream Software project or from GitHub.
Changes
Help
If you need help, my Redmine bug tracker is open to the public and you are welcome to ask for help there.
Eric
Tagged: open source redmine redmine plugins
written by Eric Davis on March 18, 2010
0 Comments
The Refactoring
Going back to the flay report, I used a combination of extract method and move method to move some duplication from a Controller to a shared Model method.
Before
# app/controller/users_controller.rb
class UsersController < ApplicationController
def edit_membership
@user = User.find(params[:id])
@membership = params[:membership_id] ? Member.find(params[:membership_id]) : Member.new(:principal => @user)
@membership.attributes = params[:membership]
@membership.save if request.post?
respond_to do |format|
format.html { redirect_to :controller => 'users', :action => 'edit', :id => @user, :tab => 'memberships' }
format.js {
render(:update) {|page|
page.replace_html "tab-content-memberships", :partial => 'users/memberships'
page.visual_effect(:highlight, "member-#{@membership.id}")
}
}
end
end
end
# app/controller/groups_controller.rb
class GroupsController < ApplicationController
def edit_membership
@group = Group.find(params[:id])
@membership = params[:membership_id] ? Member.find(params[:membership_id]) : Member.new(:principal => @group)
@membership.attributes = params[:membership]
@membership.save if request.post?
respond_to do |format|
format.html { redirect_to :controller => 'groups', :action => 'edit', :id => @group, :tab => 'memberships' }
format.js {
render(:update) {|page|
page.replace_html "tab-content-memberships", :partial => 'groups/memberships'
page.visual_effect(:highlight, "member-#{@membership.id}")
}
}
end
end
end
# app/models/member.rb
class Member < ActiveRecord::Base
# ..
end
After
# app/controller/users_controller.rb
class UsersController < ApplicationController
def edit_membership
@user = User.find(params[:id])
@membership = Member.edit_membership(params[:membership_id], params[:membership], @user)
@membership.save if request.post?
respond_to do |format|
format.html { redirect_to :controller => 'users', :action => 'edit', :id => @user, :tab => 'memberships' }
format.js {
render(:update) {|page|
page.replace_html "tab-content-memberships", :partial => 'users/memberships'
page.visual_effect(:highlight, "member-#{@membership.id}")
}
}
end
end
end
# app/controller/groups_controller.rb
class GroupsController < ApplicationController
def edit_membership
@group = Group.find(params[:id])
@membership = Member.edit_membership(params[:membership_id], params[:membership], @group)
@membership.save if request.post?
respond_to do |format|
format.html { redirect_to :controller => 'groups', :action => 'edit', :id => @group, :tab => 'memberships' }
format.js {
render(:update) {|page|
page.replace_html "tab-content-memberships", :partial => 'groups/memberships'
page.visual_effect(:highlight, "member-#{@membership.id}")
}
}
end
end
end
# app/models/member.rb
class Member < ActiveRecord::Base
# Find or initilize a Member with an id, attributes, and for a Principal
def self.edit_membership(id, new_attributes, principal=nil)
@membership = id.present? ? Member.find(id) : Member.new(:principal => principal)
@membership.attributes = new_attributes
@membership
end
end
Review
This was a simple refactoring that shows how you can combine extract method and move method in Rails to move duplicated code out of the controllers and into the models.
Reference commit
Tagged: ruby redmine refactoring pull-up-method
written by Eric Davis on March 17, 2010
0 Comments
The Refactoring
Now that yesterday's split method is done, I can use pull up method to remove several similar before_filters. I had to add a declarative method (#model_object) so each controller can state which model should be used in the finders.
Before
# app/controller/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end
# app/controller/documents_controller
class DocumentsController < ApplicationController
before_filter :find_document, :except => [:index, :new]
private
def find_document
@document = @object = Document.find(params[:id])
rescue ActiveRecord::RecordNotFound
render_404
end
end
# app/controller/issue_categories_controller
class IssueCategoriesController < ApplicationController
before_filter :find_category, :except => :new
private
def find_category
@category = @object = IssueCategory.find(params[:id])
rescue ActiveRecord::RecordNotFound
render_404
end
end
After
# app/controller/application_controller.rb
class ApplicationController < ActionController::Base
def find_model_object
model = self.class.read_inheritable_attribute('model_object')
if model
@object = model.find(params[:id])
self.instance_variable_set('@' + controller_name.singularize, @object) if @object
end
rescue ActiveRecord::RecordNotFound
render_404
end
def self.model_object(model)
write_inheritable_attribute('model_object', model)
end
end
# app/controller/documents_controller
class DocumentsController < ApplicationController
model_object Document
before_filter :find_model_object, :except => [:index, :new]
end
# app/controller/issue_categories_controller
class IssueCategoriesController < ApplicationController
model_object IssueCategory
before_filter :find_model_object, :except => :new
private
# Wrap ApplicationController's find_model_object method to set
# @category instead of just @issue_category
def find_model_object
super
@category = @object
end
end
Review
As a result of this refactoring, five controllers were able to throw away their custom method to find an object. This will also let me throw away some custom methods in my plugins that do the exact same thing.
You might notice that IssueCategoryController is different. It uses the instance variable @category instead of @issue_category. By using Ruby's inheritance I was able to wrap #find_model_object and set the correct instance variable.
Reference commit
Tagged: ruby redmine refactoring pull-up-method
written by Eric Davis on March 16, 2010
0 Comments
Back after a short personal break, I'm going to start on another set of refactorings in Redmine to remove some more duplication.
The Refactoring
This time I used split method to separate two behaviors in six similar before_filters.
Before
# app/controller/issue_relations_controller.rb
class IssueRelationsController < ApplicationController
before_filter :find_project, :authorize
private
def find_project
@issue = Issue.find(params[:issue_id])
@project = @issue.project
rescue ActiveRecord::RecordNotFound
render_404
end
end
After
# app/controller/application_controller.rb
class ApplicationController < ActionController::Base
# Finds and sets @project based on @object.project
def find_project_from_association
render_404 unless @object.present?
@project = @object.project
rescue ActiveRecord::RecordNotFound
render_404
end
end
# app/controller/issue_relations_controller.rb
class IssueRelationsController < ApplicationController
before_filter :find_issue, :find_project_from_association, :authorize
private
def find_issue
@issue = @object = Issue.find(params[:issue_id])
rescue ActiveRecord::RecordNotFound
render_404
end
end
Review
This cleans up the #find_issue before_filter so it's no longer setting project directly. It's not a very useful refactoring by itself, but it's set me up to do larger refactoring on all six of the #find_thing filters.
Reference commit
Tagged: ruby redmine refactoring split-method
written by Eric Davis on March 11, 2010
0 Comments
I've decided to try something different with the next refactorings. Instead of working on a single section of code until it's very well factored, I'm going to jump around a bit and tackle the sections that smell the worst. This will be good because I can work in different sections of Redmine and remove the worst code across the entire codebase.
The Refactoring
Using the flay tool again, I discovered a code duplication in the IssuesController.
Before
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def bulk_edit
if request.post?
# ...
if unsaved_issue_ids.empty?
flash[:notice] = l(:notice_successful_update) unless @issues.empty?
else
flash[:error] = l(:notice_failed_to_save_issues, :count => unsaved_issue_ids.size,
:total => @issues.size,
:ids => '#' + unsaved_issue_ids.join(', #'))
end
redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
return
end
@available_statuses = Workflow.available_statuses(@project)
@custom_fields = @project.all_issue_custom_fields
end
def move
# ... inside of an if statement
if unsaved_issue_ids.empty?
flash[:notice] = l(:notice_successful_update) unless @issues.empty?
else
flash[:error] = l(:notice_failed_to_save_issues, :count => unsaved_issue_ids.size,
:total => @issues.size,
:ids => '#' + unsaved_issue_ids.join(', #'))
end
# ...
end
end
After
# app/controller/issues_controller.rb
class IssuesController < ApplicationController
def bulk_edit
if request.post?
# ...
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
return
end
@available_statuses = Workflow.available_statuses(@project)
@custom_fields = @project.all_issue_custom_fields
end
def move
# ... inside of an if statement
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
# ...
end
def set_flash_from_bulk_issue_save(issues, unsaved_issue_ids)
if unsaved_issue_ids.empty?
flash[:notice] = l(:notice_successful_update) unless issues.empty?
else
flash[:error] = l(:notice_failed_to_save_issues,
:count => unsaved_issue_ids.size,
:total => issues.size,
:ids => '#' + unsaved_issue_ids.join(', #'))
end
end
end
Review
This was a simple extract method refactoring to remove duplication. I'm finding that extract method is one of the best ways to remove duplication in a single class.
(Sorry about missing a refactor yesterday, an emergency came up that kept me off the computer all day.)
Reference commit
Tagged: ruby redmine refactoring extract-method