Starting a fresh week, it's time to finish up refactoring IssuesController to remove the last of the extra actions. Using move method I was able to move the #changes method to the JournalsController.

Before

class IssuesController < ApplicationController
  before_filter :authorize, :except => [:index, :changes]
  before_filter :find_optional_project, :only => [:index, :changes]

  def changes
    retrieve_query
    sort_init 'id', 'desc'
    sort_update(@query.sortable_columns)
    
    if @query.valid?
      @journals = @query.journals(:order => "#{Journal.table_name}.created_on DESC", 
                                  :limit => 25)
    end
    @title = (@project ? @project.name : Setting.app_title) + ": " + (@query.new_record? ? l(:label_changes_details) : @query.name)
    render :layout => false, :content_type => 'application/atom+xml'
  rescue ActiveRecord::RecordNotFound
    render_404
  end

end
class JournalsController < ApplicationController
  # ...
end

After

class IssuesController < ApplicationController
  before_filter :authorize, :except => [:index]
  before_filter :find_optional_project, :only => [:index]

  # ...
end
class JournalsController < ApplicationController
  before_filter :find_optional_project, :only => [:index]
  accept_key_auth :index

  helper :issues
  helper :queries
  include QueriesHelper
  helper :sort
  include SortHelper

  def index
    retrieve_query
    sort_init 'id', 'desc'
    sort_update(@query.sortable_columns)
    
    if @query.valid?
      @journals = @query.journals(:order => "#{Journal.table_name}.created_on DESC", 
                                  :limit => 25)
    end
    @title = (@project ? @project.name : Setting.app_title) + ": " + (@query.new_record? ? l(:label_changes_details) : @query.name)
    render :layout => false, :content_type => 'application/atom+xml'
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

Since the only thing #changes did was to render an Atom feed of journal updates, moving it to the JournalsController makes it fit perfectly into the REST design. It will need a few more formats added eventually, instead of the single hardcoded Atom format. The important thing is that IssuesController now only has one final non RESTful action left, #bulk_edit. I'm still trying to think how I want to handle it, keep it on the IssuesController or move it to a new controller.

Reference commit

Tagged: ruby redmine refactoring move-method

Redmine 1.0.1 has been released this past weekend.

Tagged: redmine

To wrap up this week's refactoring of IssuesController, I used merge method to merge the #update_form action into #new. The #update_form method is an Ajax endpoint that is used to refresh an issue form's fields. For example, when an issue form changes from Bug to Feature it needs to load the fields for a Feature.

Before

class IssuesController < ApplicationController
  before_filter :find_project, :only => [:new, :create, :update_form]

  def new
    render :action => 'new', :layout => !request.xhr?
  end

  def update_form
    if params[:id].blank?
      @issue = Issue.new
      @issue.project = @project
    else
      @issue = @project.issues.visible.find(params[:id])
    end
    @issue.attributes = params[:issue]
    @allowed_statuses = ([@issue.status] + @issue.status.find_new_statuses_allowed_to(User.current.roles_for_project(@project), @issue.tracker)).uniq
    @priorities = IssuePriority.all
    
    render :partial => 'attributes'
  end

  private

  def build_new_issue_from_params
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return false
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
    @issue.start_date ||= Date.today
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
  end

end

After

class IssuesController < ApplicationController
  before_filter :find_project, :only => [:new, :create]

  def new
    respond_to do |format|
      format.html { render :action => 'new', :layout => !request.xhr? }
      format.js { render :partial => 'attributes' }
    end
  end

  # No more update_form method

  # TODO: Refactor, lots of extra code in here
  def build_new_issue_from_params
    if params[:id].blank?
      @issue = Issue.new
      @issue.copy_from(params[:copy_from]) if params[:copy_from]
      @issue.project = @project
    else
      @issue = @project.issues.visible.find(params[:id])
    end
    
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return false
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
    @issue.start_date ||= Date.today
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
  end

end

This was a good refactoring for a few reasons.

First, it lets me remove another non REST method from IssuesController.

Second, #update_form was using some older queries that were refactored some time ago. For example this query was moved to Issue#new_statuses_allowed over a year ago but #update_form was never updated.

@allowed_statuses = ([@issue.status] + @issue.status.find_new_statuses_allowed_to(User.current.roles_for_project(@project), @issue.tracker)).uniq

Third, on a conceptual level, #update_form was loading the same resource as #new but with a different response format (:js). This meant using respond_to with the two different formats would make perfect sense and would also let both formats share all of the setup code.

Reference commit

Tagged: ruby redmine refactoring merge-method

I've looked at two problems that are caused when you don't refactor, code duplication and test duplication. Both of these are mainly technical problems that affect developers. Today I want to look at a problem that affects project managers and everyone else on a team, wild estimates

Wild Estimates

When an application is well refactored, with short and clear methods, it's easy to estimate how long a change will take. This is because there will be only a few places in the code that would need to be changed and there would be less unintentional changes (bugs).

But, if an application hasn't been refactored it becomes harder to accurately estimate the impact of a change. What might look like a small change to one method, ends up requiring you to duplicate that change to six other methods (from code duplication). Or a simple code change suddenly breaks half of your tests and you spend half a day fixing them (brittle tests and test duplication).

This leads to wildly inaccurate estimates (more like guesses). What might only have taken an hour to do last month, might take three hours this month. So your original estimate of "maybe an hour" is now "maybe an hour, but it could take three or four after I rewrite then entire class". This uncertainly makes it extremely difficult for the team to plan.

The one way I was taught to counter these wild estimates is to always estimate the "worst case" and pad it. So a task that I think would take one hour would be estimated as 2-4 hours. I found some big problems with this, so I stopped using it:

  • it's dishonest
  • when you are outside of the range, your estimating skills are questioned
  • managers have learned about this "technique" and will reverse pad it

Dishonest estimates

Every developer who is paid for their work needs to be honest. If you are a professional, then you need to act like one. Giving yourself a padding is fine for planning as long as you communicate this padding to who ever is going to use it.

This means I'd tell my client "I think if everything goes as planned, I can complete this task in one hour. But in something goes wrong, we should budget 2-4 hours for this just in case". This way, my client can make the decision if they want to base their schedule on my optimistic or pessimistic estimate. This requires a lot of trust on both sides of the conversation, trust that can be eroded very quickly when things go wrong.

Questioning your estimating skills

If I give an estimate of 2-4 hours several times and every time I finish them in an hour; my project manager is going to question my estimating skills. Do this enough and their trust in your skills will vanish.

Once the trust is gone, when you say "2-4 hours" they will hear "1 hour". This is very dangerous to the project because when a task really does take 4 hours, their expectations will be shattered and along with any remaining trust in you. I've seen several project managers formalize this practice into a something called reverse padding.

Reverse padding

Dilbert estimates

Project managers aren't stupid, not matter how often Dilbert says so. When they start noticing that you are padding your estimates by doubling them, they will thank you for the estimate and reverse pad it by dividing it by two. In the most extreme cases I've seen, inexperienced project managers might latch onto the lowest estimate said:

What a developer hears:

Project Manager: How long will it take you to streamline the dot-com platforms?

Developer: Well, it took us two days last time to do this on the Blue Platform but the dot-com platform is more complex so it will take about three to five weeks.

So what's the estimate here? Three to five weeks. But what does the project manager hear?

Project Manager: How long will it take you to streamline the dot-com platforms?

Developer: Well, it took us two days last time but blah blah blah three to blah weeks.

Project Manager (thinking): So the estimate is two days to three weeks and since they are probably padding the three weeks number, lets just use two days.

Good thing both the developer and project manager are close huh. Three to five weeks versus two days.... that's going to cause a conflict later.

Solving the wild estimates

To get back to the root of the problem, there are a few things developers can do to prevent wild estimates or diffuse the problem early on. The ones I've used are:

  • spend extra time communicating what an estimate actually includes
  • talk honestly about the quality of the code and any existing risks that might come from changing that code
  • don't estimate in time and use story points or tomatoes (this is an extremely hard thing to do in a client/consultant relationship)
  • spend the extra time to refactor code as you go

Of all of these, the last is the easiest for a developer to do over time. Refactoring only takes a few extra minutes and you'll earn back that time the next time you're asked to estimate a feature. The hardest thing is being able to recognize potential refactorings and know the difference between a 5 minute refactoring and a 5 hours refactoring.

Like this post? Read the rest of the Problems When You Don't Refactor Rails series

  1. Problems when you don't Refactor Rails: Code Duplication
  2. Problems when you don't Refactor Rails: Test Duplication
  3. Problems when you don't Refactor Rails: Wild Estimates
  4. Problems when you don't Refactor Rails: Lowered morale
  5. Problems when you don't Refactor Rails: Lower Participation
Tagged: ruby refactoring

Using extract class on the IssuesController, I was able to create another controller called ContextMenusController. This controller will handle Redmine's right click context menu that is used on the issues list.

Before

class IssuesController < ApplicationController
  before_filter :authorize, :except => [:index, :changes, :context_menu]

  def context_menu
    @issues = Issue.find_all_by_id(params[:ids], :include => :project)
    if (@issues.size == 1)
      @issue = @issues.first
      @allowed_statuses = @issue.new_statuses_allowed_to(User.current)
    end
    projects = @issues.collect(&:project).compact.uniq
    @project = projects.first if projects.size == 1

    @can = {:edit => (@project && User.current.allowed_to?(:edit_issues, @project)),
            :log_time => (@project && User.current.allowed_to?(:log_time, @project)),
            :update => (@project && (User.current.allowed_to?(:edit_issues, @project) || (User.current.allowed_to?(:change_status, @project) && @allowed_statuses && !@allowed_statuses.empty?))),
            :move => (@project && User.current.allowed_to?(:move_issues, @project)),
            :copy => (@issue && @project.trackers.include?(@issue.tracker) && User.current.allowed_to?(:add_issues, @project)),
            :delete => (@project && User.current.allowed_to?(:delete_issues, @project))
            }
    if @project
      @assignables = @project.assignable_users
      @assignables << @issue.assigned_to if @issue && @issue.assigned_to && !@assignables.include?(@issue.assigned_to)
      @trackers = @project.trackers
    end
    
    @priorities = IssuePriority.all.reverse
    @statuses = IssueStatus.find(:all, :order => 'position')
    @back = back_url
    
    render :layout => false
  end
end

After

class IssuesController < ApplicationController
  before_filter :authorize, :except => [:index, :changes]

  # ...
end
class ContextMenusController < ApplicationController
  helper :watchers
  
  def issues
    @issues = Issue.find_all_by_id(params[:ids], :include => :project)
    if (@issues.size == 1)
      @issue = @issues.first
      @allowed_statuses = @issue.new_statuses_allowed_to(User.current)
    end
    projects = @issues.collect(&:project).compact.uniq
    @project = projects.first if projects.size == 1

    @can = {:edit => (@project && User.current.allowed_to?(:edit_issues, @project)),
            :log_time => (@project && User.current.allowed_to?(:log_time, @project)),
            :update => (@project && (User.current.allowed_to?(:edit_issues, @project) || (User.current.allowed_to?(:change_status, @project) && @allowed_statuses && !@allowed_statuses.empty?))),
            :move => (@project && User.current.allowed_to?(:move_issues, @project)),
            :copy => (@issue && @project.trackers.include?(@issue.tracker) && User.current.allowed_to?(:add_issues, @project)),
            :delete => (@project && User.current.allowed_to?(:delete_issues, @project))
            }
    if @project
      @assignables = @project.assignable_users
      @assignables << @issue.assigned_to if @issue && @issue.assigned_to && !@assignables.include?(@issue.assigned_to)
      @trackers = @project.trackers
    end
    
    @priorities = IssuePriority.all.reverse
    @statuses = IssueStatus.find(:all, :order => 'position')
    @back = back_url
    
    render :layout => false
  end
  
end

After this refactoring, only three non-REST actions remain in IssuesController. Once they are refactored into separate controllers, then I can start going back through everything again to refactor their internal code.

Reference commit

Tagged: ruby redmine refactoring extract-controller extract-class

Today I used extract class on IssuesController once again, this time to create PreviewsController. For the same reasons as yesterday, putting all of the preview actions together into a single controller will make things easier to maintain and might even give me some code reuse.

Before

class IssuesController < ApplicationController
  before_filter :find_project, :only => [:new, :create, :update_form, :preview]

  def preview
    @issue = @project.issues.find_by_id(params[:id]) unless params[:id].blank?
    if @issue
      @attachements = @issue.attachments
      @description = params[:issue] && params[:issue][:description]
      if @description && @description.gsub(/(\r?\n|\n\r?)/, "\n") == @issue.description.to_s.gsub(/(\r?\n|\n\r?)/, "\n")
        @description = nil
      end
      @notes = params[:notes]
    else
      @description = (params[:issue] ? params[:issue][:description] : nil)
    end
    render :layout => false
  end
end

After

class IssuesController < ApplicationController
  before_filter :find_project, :only => [:new, :create, :update_form]

  # ...
end
class PreviewsController < ApplicationController
  before_filter :find_project

  def issue
    @issue = @project.issues.find_by_id(params[:id]) unless params[:id].blank?
    if @issue
      @attachements = @issue.attachments
      @description = params[:issue] && params[:issue][:description]
      if @description && @description.gsub(/(\r?\n|\n\r?)/, "\n") == @issue.description.to_s.gsub(/(\r?\n|\n\r?)/, "\n")
        @description = nil
      end
      @notes = params[:notes]
    else
      @description = (params[:issue] ? params[:issue][:description] : nil)
    end
    render :layout => false
  end

  private
  
  def find_project
    project_id = (params[:issue] && params[:issue][:project_id]) || params[:project_id]
    @project = Project.find(project_id)
  rescue ActiveRecord::RecordNotFound
    render_404
  end
  
end

Nothing too complex in this refactoring; just creating the new controller class, moving the method over, and updating all of the callers to use the new controller. The reference commit shows some other updates like the routing and view changes.

Little by little the IssuesController is getting a long overdue cleanup. Now there are only a few non-REST actions left to move:

  • index
  • changes - non-REST
  • show
  • new
  • create
  • edit
  • update
  • bulk_edit - non-REST
  • destroy
  • context_menu - non-REST
  • update_form - non-REST

Reference commit

Tagged: ruby redmine refactoring extract-controller extract-class

Continuing on the IssuesController refactoring, I used extract class to create a new controller for the #auto_complete action. This action is used to search for issues that match the text from a user. Redmine has several auto_complete actions scattered throughout different controllers that I've been wanting to unite into a single controller. Now AutoCompletesController can become that single controller.

Before

class IssuesController < ApplicationController
  before_filter :find_project, :only => [:new, :create, :update_form, :preview, :auto_complete]

  def auto_complete
    @issues = []
    q = params[:q].to_s
    if q.match(/^\d+$/)
      @issues << @project.issues.visible.find_by_id(q.to_i)
    end
    unless q.blank?
      @issues += @project.issues.visible.find(:all, :conditions => ["LOWER(#{Issue.table_name}.subject) LIKE ?", "%#{q.downcase}%"], :limit => 10)
    end
    render :layout => false
  end
end

After

class IssuesController < ApplicationController
  # ...
end
class AutoCompletesController < ApplicationController
  before_filter :find_project
  
  def issues
    @issues = []
    q = params[:q].to_s
    if q.match(/^\d+$/)
      @issues << @project.issues.visible.find_by_id(q.to_i)
    end
    unless q.blank?
      @issues += @project.issues.visible.find(:all, :conditions => ["LOWER(#{Issue.table_name}.subject) LIKE ?", "%#{q.downcase}%"], :limit => 10)
    end
    render :layout => false
  end

  private

  def find_project
    project_id = (params[:issue] && params[:issue][:project_id]) || params[:project_id]
    @project = Project.find(project_id)
  rescue ActiveRecord::RecordNotFound
    render_404
  end

end

This refactoring makes it easier for other controllers to find and understand how to use the auto complete. It also removes another action from IssuesController, finally getting it under 400 lines of code.

Reference commit

Tagged: ruby redmine refactoring extract-controller extract-class

Looking over the public methods in IssuesController now, I see that the #reply method is out of place. It's used to quote a new reply on an issue but it does this via a journal. Since Redmine already has a JournalsController that is used when a journal is edited, it makes more sense to move #reply to that controller.

Before

class IssuesController < ApplicationController
  before_filter :find_issue, :only => [:show, :edit, :update, :reply]

  def reply
    journal = Journal.find(params[:journal_id]) if params[:journal_id]
    if journal
      user = journal.user
      text = journal.notes
    else
      user = @issue.author
      text = @issue.description
    end
    # Replaces pre blocks with [...]
    text = text.to_s.strip.gsub(%r{<pre>((.|\s)*?)</pre>}m, '[...]')
    content = "#{ll(Setting.default_language, :text_user_wrote, user)}\n> "
    content << text.gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n"
      
    render(:update) { |page|
      page.<< "$('notes').value = \"#{escape_javascript content}\";"
      page.show 'update'
      page << "Form.Element.focus('notes');"
      page << "Element.scrollTo('update');"
      page << "$('notes').scrollTop = $('notes').scrollHeight - $('notes').clientHeight;"
    }
  end
  
end
class JournalsController < ApplicationController
  # ...
end

After

class IssuesController < ApplicationController
  # ...
end
class JournalsController < ApplicationController
  before_filter :find_issue, :only => [:new]
  
  def new
    journal = Journal.find(params[:journal_id]) if params[:journal_id]
    if journal
      user = journal.user
      text = journal.notes
    else
      user = @issue.author
      text = @issue.description
    end
    # Replaces pre blocks with [...]
    text = text.to_s.strip.gsub(%r{<pre>((.|\s)*?)</pre>}m, '[...]')
    content = "#{ll(Setting.default_language, :text_user_wrote, user)}\n> "
    content << text.gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n"
      
    render(:update) { |page|
      page.<< "$('notes').value = \"#{escape_javascript content}\";"
      page.show 'update'
      page << "Form.Element.focus('notes');"
      page << "Element.scrollTo('update');"
      page << "$('notes').scrollTop = $('notes').scrollHeight - $('notes').clientHeight;"
    }
  end
end

After using move method I used rename method to rename #reply to #new. This is to make it match the Rails REST conventions, where the #new action is used with a HTTP GET request to render a new form. Little by little, IssuesController is shrinking.

Reference commit

Tagged: ruby redmine refactoring move-method

This is the final refactoring I want to do on IssueMovesController for now. Using pull up method, I moved the last duplicated method from IssueMovesController to ApplicationController.

Before

class IssuesController < ApplicationController
  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
class IssueMovesController < ApplicationController
  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
class ApplicationController < ActionController::Base
  # ...
end

After

class IssuesController < ApplicationController
  # ...
end
class IssueMovesController < ApplicationController
  # ...
end
class ApplicationController < ActionController::Base
  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

Now that IssueMovesController is done, I can go back to IssuesController and work on the next set of refactorings. It's finally starting to really shrink, there are only 14 public actions left which should only take a few more weeks to resolve.

  1. index
  2. changes
  3. show
  4. new
  5. create
  6. edit
  7. update
  8. reply
  9. bulk_edit
  10. destroy
  11. context_menu
  12. update_form
  13. preview
  14. auto_complete

Reference commit

Tagged: ruby redmine refactoring pull-up-method

Now, in order to finish up IssueMovesController, I need to refactor the duplicated methods that I copied from IssuesController. Pull up method is a great refactoring to use in this case.

Before

class IssuesController < ApplicationController
  # Filter for bulk operations
  def find_issues
    @issues = Issue.find_all_by_id(params[:id] || params[:ids])
    raise ActiveRecord::RecordNotFound if @issues.empty?
    projects = @issues.collect(&:project).compact.uniq
    if projects.size == 1
      @project = projects.first
    else
      # TODO: let users bulk edit/move/destroy issues from different projects
      render_error 'Can not bulk edit/move/destroy issues from different projects'
      return false
    end
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end
class IssueMovesController < ApplicationController
  # Filter for bulk operations
  # TODO: duplicated in IssuesController
  def find_issues
    @issues = Issue.find_all_by_id(params[:id] || params[:ids])
    raise ActiveRecord::RecordNotFound if @issues.empty?
    projects = @issues.collect(&:project).compact.uniq
    if projects.size == 1
      @project = projects.first
    else
      # TODO: let users bulk edit/move/destroy issues from different projects
      render_error 'Can not bulk edit/move/destroy issues from different projects'
      return false
    end
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end
class ApplicationController < ActionController::Base
  # ...
end

After

class IssuesController < ApplicationController
  # ...
end
class IssueMovesController < ApplicationController
  # ...
end
class ApplicationController < ActionController::Base
  # Filter for bulk issue operations
  def find_issues
    @issues = Issue.find_all_by_id(params[:id] || params[:ids])
    raise ActiveRecord::RecordNotFound if @issues.empty?
    projects = @issues.collect(&:project).compact.uniq
    if projects.size == 1
      @project = projects.first
    else
      # TODO: let users bulk edit/move/destroy issues from different projects
      render_error 'Can not bulk edit/move/destroy issues from different projects'
      return false
    end
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

Now that this method has been moved, there is only one more duplicate method in IssueMovesController, set_flash_from_bulk_issue_save. I'll tackle this refactoring tomorrow.

Reference commit

Tagged: ruby redmine refactoring pull-up-method