Redmine Refactor #115: Split Method in VersionsController

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?)

Reference commit