Still focusing on the VersionsController, I need to split another method before I can refactor this controller’s routing. This time #new needs to be split into #new and #create.
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 28 29 30 31 32 33 34 35 36 37 | class VersionsController [:index, :new, :close_completed] before_filter :find_project_from_association, :except => [:index, :new, :close_completed] before_filter :find_project, :only => [:index, :new, :close_completed] before_filter :authorize def new @version = @project.versions.build if params[:version] attributes = params[:version].dup attributes.delete('sharing') unless attributes.nil? || @version.allowed_sharings.include?(attributes['sharing']) @version.attributes = attributes end if request.post? if @version.save respond_to do |format| format.html do flash[:notice] = l(:notice_successful_create) redirect_to :controller => 'projects', :action => 'settings', :tab => 'versions', :id => @project end format.js do # IE doesn't support the replace_html rjs method for select box options render(:update) {|page| page.replace "issue_fixed_version_id", content_tag('select', '' + version_options_for_select(@project.shared_versions.open, @version), :id => 'issue_fixed_version_id', :name => 'issue[fixed_version_id]') } end end else respond_to do |format| format.html format.js do render(:update) {|page| page.alert(@version.errors.full_messages.join('\n')) } end end end end 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 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 | class VersionsController [:index, :new, :create, :close_completed] before_filter :find_project_from_association, :except => [:index, :new, :create, :close_completed] before_filter :find_project, :only => [:index, :new, :create, :close_completed] before_filter :authorize def new @version = @project.versions.build if params[:version] attributes = params[:version].dup attributes.delete('sharing') unless attributes.nil? || @version.allowed_sharings.include?(attributes['sharing']) @version.attributes = attributes end end def create # TODO: refactor with code above in #new @version = @project.versions.build if params[:version] attributes = params[:version].dup attributes.delete('sharing') unless attributes.nil? || @version.allowed_sharings.include?(attributes['sharing']) @version.attributes = attributes end if request.post? if @version.save respond_to do |format| format.html do flash[:notice] = l(:notice_successful_create) redirect_to :controller => 'projects', :action => 'settings', :tab => 'versions', :id => @project end format.js do # IE doesn't support the replace_html rjs method for select box options render(:update) {|page| page.replace "issue_fixed_version_id", content_tag('select', '' + version_options_for_select(@project.shared_versions.open, @version), :id => 'issue_fixed_version_id', :name => 'issue[fixed_version_id]') } end end else respond_to do |format| format.html { render :action => 'new' } format.js do render(:update) {|page| page.alert(@version.errors.full_messages.join('\n')) } end end end end end end |
I’ve done similar split methods before but I want to point out two things to keep in mind.
The first change is deep in the respond_to block. In the before code, the failure state just uses format.html which by default would render the current action’s view. But when it’s refactored into #create, the current action becomes #create which doesn’t have a view. So I had to add an explicit render :action => 'new' to make sure the form is rendered with the errors. This is an edge case bug that could appear from your refactorings but it might be missed. Redmine, unfortunately, doesn’t have a test case for that so I would have missed it if I wasn’t paying attention.
The second change is more visible. When splitting #create out, I had to duplicate the six lines of setup code at the start of the method. Normally you don’t want to duplicate code at all, but in this case I am willing to take on a little technical debt now in order to move forward with my route refactoring. Ideally, you would refactor that setup code into a utility method first and then split the method.
With refactoring, you will always have trade offs. Sometimes you have to make decisions in the short term in order to have a better product in the long term. This refactoring shows one of those decisions I made.
(But, since Redmine is Open Source… I’d be happy to have a reader “fix” my refactoring and submit the patch to me. Don’t you want to say you contributed to the bug tracker that powers the Ruby language?)
No comments yet.