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

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

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

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

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

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

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

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

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