written by Eric Davis on August 23, 2010
0 Comments
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
written by Eric Davis on August 23, 2010
0 Comments
Tagged: redmine
written by Eric Davis on August 20, 2010
0 Comments
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
written by Eric Davis on August 20, 2010
0 Comments
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
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
- Problems when you don't Refactor Rails: Code Duplication
- Problems when you don't Refactor Rails: Test Duplication
- Problems when you don't Refactor Rails: Wild Estimates
- Problems when you don't Refactor Rails: Lowered morale
- Problems when you don't Refactor Rails: Lower Participation
Tagged: ruby refactoring
written by Eric Davis on August 19, 2010
0 Comments
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
written by Eric Davis on August 18, 2010
0 Comments
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
written by Eric Davis on August 17, 2010
0 Comments
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
written by Eric Davis on August 16, 2010
0 Comments
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
written by Eric Davis on August 13, 2010
0 Comments
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.
- index
- changes
- show
- new
- create
- edit
- update
- reply
- bulk_edit
- destroy
- context_menu
- update_form
- preview
- auto_complete
Reference commit
Tagged: ruby redmine refactoring pull-up-method
written by Eric Davis on August 12, 2010
0 Comments
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
