The Refactoring
Mark Thomas pointed out some odd code in Redmine’s IssuesController#build_new_issue_from_params, specifically that it’s doing some error checking in the middle of a method body.
Before
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 | # app/controllers/issues_controller.rb class IssuesController < ApplicationController 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 @issue.status.nil? render_error l(:error_no_default_issue_status) 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
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 | # app/controllers/issues_controller.rb class IssuesController [:new, :create] 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 def check_for_default_issue_status if IssueStatus.default.nil? render_error l(:error_no_default_issue_status) return false end end end |
Review
The error checking almost looks like they should be validations but they are done systemwide. By using extract method I was able to get the error checking out of the action until I can figure out the best way to handle systemwide validations.
Share
Related posts:
