Redmine's ProjectsController is starting to shed actions pretty quickly now. There are two more actions that should belong to a different controller though, #save_activities and #reset_activities. I'm starting on #save_activities today.

These two actions affect a project's Time Entry Activities, basically the "types" of time that is logged to Redmine (e.g. frontend development, testing, database design). Since they are saved as enumeration records in Redmine, I used extract class move method to create a new controller for the #save_activities method.

Before

class ProjectsController < ApplicationController
  def save_activities
    if request.post? && params[:enumerations]
      Project.transaction do
        params[:enumerations].each do |id, activity|
          @project.update_or_create_time_entry_activity(id, activity)
        end
      end
      flash[:notice] = l(:notice_successful_update)
    end
    
    redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project
  end
end

After

class ProjectsController < ApplicationController
  # Removed save_activities method
end
class ProjectEnumerationsController < ApplicationController
  before_filter :find_project
  before_filter :authorize
  
  def save
    if request.post? && params[:enumerations]
      Project.transaction do
        params[:enumerations].each do |id, activity|
          @project.update_or_create_time_entry_activity(id, activity)
        end
      end
      flash[:notice] = l(:notice_successful_update)
    end
    
    redirect_to :controller => 'projects', :action => 'settings', :tab => 'activities', :id => @project
  end

end

I decided to rename the method to #save for now. This action is weird because it's a bulk update/create. (Or in Rails terms, it updates a collection and not a member of a resource) After the next refactoring of #reset_activities I think I'll have a better idea of how to update ProjectEnumerationsController to the REST style, or if it should be made to match REST.

Reference commit

Tagged: ruby redmine refactoring extract-class move-method

Since I created a new FilesController yesterday, I can now move another method over to it from the ProjectsController. ProjectsController#add_file is used for two things:

  1. To show the form that's used to upload a new file
  2. To receive a new file upload

Since both of these are basic descriptions of what I'd expect a File resource to be, they fit perfectly on the FilesController.

Before

class ProjectsController < ApplicationController
  menu_item :files, :only => [:add_file]

  def add_file
    if request.post?
      container = (params[:version_id].blank? ? @project : @project.versions.find_by_id(params[:version_id]))
      attachments = Attachment.attach_files(container, params[:attachments])
      render_attachment_warning_if_needed(container)

      if !attachments.empty? && Setting.notified_events.include?('file_added')
        Mailer.deliver_attachments_added(attachments[:files])
      end
      redirect_to :controller => 'files', :action => 'index', :id => @project
      return
    end
    @versions = @project.versions.sort
  end
end
class FilesController < ApplicationController
end

After

class ProjectsController < ApplicationController
  # No menu_item for :files
  # Removed add_file method
end
class FilesController < ApplicationController
  # TODO: split method into new (GET) and create (POST)
  def new
    if request.post?
      container = (params[:version_id].blank? ? @project : @project.versions.find_by_id(params[:version_id]))
      attachments = Attachment.attach_files(container, params[:attachments])
      render_attachment_warning_if_needed(container)

      if !attachments.empty? && Setting.notified_events.include?('file_added')
        Mailer.deliver_attachments_added(attachments[:files])
      end
      redirect_to :controller => 'files', :action => 'index', :id => @project
      return
    end
    @versions = @project.versions.sort
  end
end

As part of this refactor, I also added a TODO note for later. Since the method is used for two separate actions, it needs to be split into #new and #create actions to function like a Rails REST controller. This splitting is common in older Rails code that predates REST.

I could do this split in tomorrow's refactor but I'm going to leave it for later and keep focusing on the ProjectsController. Right now Redmine would get more benefit from refactoring it's really bad code smells than to polish this new section of code.

Reference commit

Tagged: ruby redmine refactoring move-method

Looking through Redmine's ProjectsController, I found a few actions that were being used to manage project files. These files are separate resources on a project so using move method and extract class I created a new controller, FilesController.

Before

class ProjectsController < ApplicationController
  menu_item :files, :only => [:list_files, :add_file]

  def list_files
    sort_init 'filename', 'asc'
    sort_update 'filename' => "#{Attachment.table_name}.filename",
                'created_on' => "#{Attachment.table_name}.created_on",
                'size' => "#{Attachment.table_name}.filesize",
                'downloads' => "#{Attachment.table_name}.downloads"
                
    @containers = [ Project.find(@project.id, :include => :attachments, :order => sort_clause)]
    @containers += @project.versions.find(:all, :include => :attachments, :order => sort_clause).sort.reverse
    render :layout => !request.xhr?
  end
end

After

class ProjectsController < ApplicationController
  menu_item :files, :only => [:add_file]
end
class FilesController < ApplicationController
  menu_item :files

  before_filter :find_project
  before_filter :authorize

  helper :sort
  include SortHelper

  def index
    sort_init 'filename', 'asc'
    sort_update 'filename' => "#{Attachment.table_name}.filename",
                'created_on' => "#{Attachment.table_name}.created_on",
                'size' => "#{Attachment.table_name}.filesize",
                'downloads' => "#{Attachment.table_name}.downloads"
                
    @containers = [ Project.find(@project.id, :include => :attachments, :order => sort_clause)]
    @containers += @project.versions.find(:all, :include => :attachments, :order => sort_clause).sort.reverse
    render :layout => !request.xhr?
  end

end

Not only did this refactoring create the new FilesController but it also renamed the #list_files method to the standard #index. This will make it easier to convert the controller into a REST controller with an API in the future.

Reference commit

Tagged: ruby redmine refactoring move-method extract-class

The next refactoring I performed on Redmine's ProjectsController was to move the #roadmap method to VersionsController. The #roadmap is used to list all Versions on a project. Since that fits Rails' RESTful conventions for #index, I used move method to move it to the VersionsController.

Before

class ProjectsController < ApplicationController
  def roadmap
    @trackers = @project.trackers.find(:all, :order => 'position')
    retrieve_selected_tracker_ids(@trackers, @trackers.select {|t| t.is_in_roadmap?})
    @with_subprojects = params[:with_subprojects].nil? ? Setting.display_subprojects_issues? : (params[:with_subprojects] == '1')
    project_ids = @with_subprojects ? @project.self_and_descendants.collect(&:id) : [@project.id]
    
    @versions = @project.shared_versions || []
    @versions += @project.rolled_up_versions.visible if @with_subprojects
    @versions = @versions.uniq.sort
    @versions.reject! {|version| version.closed? || version.completed? } unless params[:completed]
    
    @issues_by_version = {}
    unless @selected_tracker_ids.empty?
      @versions.each do |version|
        issues = version.fixed_issues.visible.find(:all,
                                                   :include => [:project, :status, :tracker, :priority],
                                                   :conditions => {:tracker_id => @selected_tracker_ids, :project_id => project_ids},
                                                   :order => "#{Project.table_name}.lft, #{Tracker.table_name}.position, #{Issue.table_name}.id")
        @issues_by_version[version] = issues
      end
    end
    @versions.reject! {|version| !project_ids.include?(version.project_id) && @issues_by_version[version].blank?}
  end

  private

  def retrieve_selected_tracker_ids(selectable_trackers, default_trackers=nil)
    if ids = params[:tracker_ids]
      @selected_tracker_ids = (ids.is_a? Array) ? ids.collect { |id| id.to_i.to_s } : ids.split('/').collect { |id| id.to_i.to_s }
    else
      @selected_tracker_ids = (default_trackers || selectable_trackers).collect {|t| t.id.to_s }
    end
  end

end
class VersionsController < ApplicationController
  before_filter :find_model_object, :except => [:new, :close_completed]
  before_filter :find_project_from_association, :except => [:new, :close_completed]
  before_filter :find_project, :only => [:new, :close_completed]
end

After

class ProjectsController < ApplicationController
end
class VersionsController < ApplicationController
  before_filter :find_model_object, :except => [:index, :new, :close_completed]
  before_filter :find_project_from_association, :except => [:index, :new, :close_completed]
  before_filter :find_project, :only => [:index, :new, :close_completed]

  def index
    @trackers = @project.trackers.find(:all, :order => 'position')
    retrieve_selected_tracker_ids(@trackers, @trackers.select {|t| t.is_in_roadmap?})
    @with_subprojects = params[:with_subprojects].nil? ? Setting.display_subprojects_issues? : (params[:with_subprojects] == '1')
    project_ids = @with_subprojects ? @project.self_and_descendants.collect(&:id) : [@project.id]
    
    @versions = @project.shared_versions || []
    @versions += @project.rolled_up_versions.visible if @with_subprojects
    @versions = @versions.uniq.sort
    @versions.reject! {|version| version.closed? || version.completed? } unless params[:completed]
    
    @issues_by_version = {}
    unless @selected_tracker_ids.empty?
      @versions.each do |version|
        issues = version.fixed_issues.visible.find(:all,
                                                   :include => [:project, :status, :tracker, :priority],
                                                   :conditions => {:tracker_id => @selected_tracker_ids, :project_id => project_ids},
                                                   :order => "#{Project.table_name}.lft, #{Tracker.table_name}.position, #{Issue.table_name}.id")
        @issues_by_version[version] = issues
      end
    end
    @versions.reject! {|version| !project_ids.include?(version.project_id) && @issues_by_version[version].blank?}
  end

  private

  def retrieve_selected_tracker_ids(selectable_trackers, default_trackers=nil)
    if ids = params[:tracker_ids]
      @selected_tracker_ids = (ids.is_a? Array) ? ids.collect { |id| id.to_i.to_s } : ids.split('/').collect { |id| id.to_i.to_s }
    else
      @selected_tracker_ids = (default_trackers || selectable_trackers).collect {|t| t.id.to_s }
    end
  end

end

After I updated VersionsController's before_filters and moved the utility method, retrieve_selected_tracker_ids, it was easy to move #roadmap. I turned out lucky that the VersionsController didn't try to use #index for anything already. Otherwise I wouldn't have been able to do this refactoring and might have had to put #roadmap into it's own controller.

Reference commit

Tagged: ruby redmine refactoring move-method

Starting on my refactoring of Redmine's ProjectsController, I used extract class to move the #activity method to a new controller. #activity is used to get a timeline of events on a project, so you can see what's recently happened. Since it's not tied to an actual project record, it doesn't fit on ProjectsController.

Before

class ProjectsController < ApplicationController
  menu_item :activity, :only => :activity
  
  before_filter :find_optional_project, :only => :activity
  accept_key_auth :activity, :index

  def activity
    @days = Setting.activity_days_default.to_i
    
    if params[:from]
      begin; @date_to = params[:from].to_date + 1; rescue; end
    end

    @date_to ||= Date.today + 1
    @date_from = @date_to - @days
    @with_subprojects = params[:with_subprojects].nil? ? Setting.display_subprojects_issues? : (params[:with_subprojects] == '1')
    @author = (params[:user_id].blank? ? nil : User.active.find(params[:user_id]))
    
    @activity = Redmine::Activity::Fetcher.new(User.current, :project => @project, 
                                                             :with_subprojects => @with_subprojects,
                                                             :author => @author)
    @activity.scope_select {|t| !params["show_#{t}"].nil?}
    @activity.scope = (@author.nil? ? :default : :all) if @activity.scope.empty?

    events = @activity.events(@date_from, @date_to)
    
    if events.empty? || stale?(:etag => [events.first, User.current])
      respond_to do |format|
        format.html { 
          @events_by_day = events.group_by(&:event_date)
          render :layout => false if request.xhr?
        }
        format.atom {
          title = l(:label_activity)
          if @author
            title = @author.name
          elsif @activity.scope.size == 1
            title = l("label_#{@activity.scope.first.singularize}_plural")
          end
          render_feed(events, :title => "#{@project || Setting.app_title}: #{title}")
        }
      end
    end
    
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

After

class ProjectsController < ApplicationController
  accept_key_auth :index
end
class ActivitiesController < ApplicationController
  menu_item :activity
  before_filter :find_optional_project
  accept_key_auth :index

  def index
    @days = Setting.activity_days_default.to_i
    
    if params[:from]
      begin; @date_to = params[:from].to_date + 1; rescue; end
    end

    @date_to ||= Date.today + 1
    @date_from = @date_to - @days
    @with_subprojects = params[:with_subprojects].nil? ? Setting.display_subprojects_issues? : (params[:with_subprojects] == '1')
    @author = (params[:user_id].blank? ? nil : User.active.find(params[:user_id]))
    
    @activity = Redmine::Activity::Fetcher.new(User.current, :project => @project, 
                                                             :with_subprojects => @with_subprojects,
                                                             :author => @author)
    @activity.scope_select {|t| !params["show_#{t}"].nil?}
    @activity.scope = (@author.nil? ? :default : :all) if @activity.scope.empty?

    events = @activity.events(@date_from, @date_to)
    
    if events.empty? || stale?(:etag => [events.first, User.current])
      respond_to do |format|
        format.html { 
          @events_by_day = events.group_by(&:event_date)
          render :layout => false if request.xhr?
        }
        format.atom {
          title = l(:label_activity)
          if @author
            title = @author.name
          elsif @activity.scope.size == 1
            title = l("label_#{@activity.scope.first.singularize}_plural")
          end
          render_feed(events, :title => "#{@project || Setting.app_title}: #{title}")
        }
      end
    end
    
  rescue ActiveRecord::RecordNotFound
    render_404
  end

  private

  # TODO: refactor, duplicated in projects_controller
  def find_optional_project
    return true unless params[:id]
    @project = Project.find(params[:id])
    authorize
  rescue ActiveRecord::RecordNotFound
    render_404
  end

end

Other than the method move and rename, there isn't that much to this refactoring.

The #find_optional_project method had to be copied over from ProjectsController since it was used as a before_filter, but I find that's common when a controller is first split. Like other code smells I create intentionally, I used a comment with a "TODO" to mark it for later.

There are still quite a few methods in ProjectsController to be refactored. From the looks of it, some will end up in new controllers like this refactoring. While others will be merged into other actions.

Reference commit

Tagged: ruby redmine refactoring extract-class

Another problem unfactored code causes with open source Rails projects, is lower participation.

Before getting into that, why do we even care about project participation?

Why participation matters

Lower participation in open source projects can kill it. Developers care about code quality and will leave if the quality gets too low. Users don't care about code quality, as long as it still works. This means that you can have poor quality code that does a job (i.e. unfactored code).

So the number of users can keep growing, but with a stagnant or declining developer interest, work will start to pile up. Once this starts, it's a difficult trend to reverse since now there are less developers to share the growing workload (the developer to user ratio). This might cause quick "hacks" and "patches" to get added, further decreasing the code quality, decreasing developer participation, cycling down and down.

Developers as a group don't like to work on code that is difficult to understand. Attracting volunteer developers to your project is hard enough, but if your code isn't well factored they might run screaming back to their own project (or write a competitor application). I know this from personal experience; after talking with dozens of developers who were interested in contributing to Redmine and many of them complained about how old and complex Redmine's code is. I don't blame them, if I was asked to help out and saw a bunch of unfactored code that I wasn't familiar with I'd raise complaints also.

I've seen three ways poorly factored code has affected participation:

  • it distracts developers
  • it confuses developers
  • it gives the impression that quality doesn't matter

Distracting developers

Developers want to work on fun and interesting projects. When a potential developer is attracted to your project, they want to enjoy working on it. But if they have to spend their time wading through a bunch of unfactored code, this will interrupt their fun and will prevent them from contributing. No one wants to spend four hours going through a mess of code just to find where to put a five minute feature in.

Getting that first contribution is a major hurdle already. You want to do everything you can to keep from distracting someone from making that first contribution.

Confusing developers

When a developer starts on a new project, they know nothing about how the project is setup. The project documentation and processes might help them get a feel for how things are organized but there will always be parts of the code that aren't documented or informal processes passed around verbally.

This means that new developers will have to read the code in order to learn about how the system is organized. Unfactored code, as a whole, will confuse developers and throw them off track. Even simple things like method names that don't match what the method does will cause confusion. You might expect a method called #visible_by to check if something is visible to someone, but not in Redmine. It's used to build a SQL string so you can query the database yourself.

Quality matters, or does it?

The third blow to participation from poorly factored code is that it will give the impression that the project doesn't care about quality. There isn't any real metric for this but it's easy to find out just by listening to the comments people say (or aren't saying) about your project. If you're getting a lot of comments like "I'd like to contribute, but the code was confusing", then you might have an underlying problem.

A collateral problem from these opinions about quality is that they will eventually make their way to the users. The users will have no experience to judge the code quality themselves, so they might take these opinions as fact. And no one wants to use low quality software.

Keeping your project well factored is a difficult task, but as a software developer you don't really have many choices.

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

For my 100th refactoring of Redmine, I decided to go big. Today I refactored Redmine's Issue routes from standard Rails routes to actual REST resources.

Before

# config/routes.rb
ActionController::Routing::Routes.draw do |map|
  map.with_options :controller => 'issues' do |issues_routes|
    issues_routes.with_options :conditions => {:method => :get} do |issues_views|
      issues_views.connect 'issues', :action => 'index'
      issues_views.connect 'issues.:format', :action => 'index'
      issues_views.connect 'projects/:project_id/issues', :action => 'index'
      issues_views.connect 'projects/:project_id/issues.:format', :action => 'index'
      issues_views.connect 'projects/:project_id/issues/new', :action => 'new'
      issues_views.connect 'projects/:project_id/issues/gantt', :controller => 'gantts', :action => 'show'
      issues_views.connect 'projects/:project_id/issues/calendar', :controller => 'calendars', :action => 'show'
      issues_views.connect 'projects/:project_id/issues/:copy_from/copy', :action => 'new'
      issues_views.connect 'issues/:id', :action => 'show', :id => /\d+/
      issues_views.connect 'issues/:id.:format', :action => 'show', :id => /\d+/
      issues_views.connect 'issues/:id/edit', :action => 'edit', :id => /\d+/
    end
    issues_routes.with_options :conditions => {:method => :post} do |issues_actions|
      issues_actions.connect 'issues', :action => 'index'
      issues_actions.connect 'projects/:project_id/issues', :action => 'create'
      issues_actions.connect 'projects/:project_id/issues/gantt', :controller => 'gantts', :action => 'show'
      issues_actions.connect 'projects/:project_id/issues/calendar', :controller => 'calendars', :action => 'show'
      issues_actions.connect 'issues/:id/quoted', :controller => 'journals', :action => 'new', :id => /\d+/
      issues_actions.connect 'issues/:id/:action', :action => /edit|destroy/, :id => /\d+/
      issues_actions.connect 'issues.:format', :action => 'create', :format => /xml/
      issues_actions.connect 'issues/bulk_edit', :action => 'bulk_update'
    end
    issues_routes.with_options :conditions => {:method => :put} do |issues_actions|
      issues_actions.connect 'issues/:id/edit', :action => 'update', :id => /\d+/
      issues_actions.connect 'issues/:id.:format', :action => 'update', :id => /\d+/, :format => /xml/
    end
    issues_routes.with_options :conditions => {:method => :delete} do |issues_actions|
      issues_actions.connect 'issues/:id.:format', :action => 'destroy', :id => /\d+/, :format => /xml/
    end
    issues_routes.connect 'issues/gantt', :controller => 'gantts', :action => 'show'
    issues_routes.connect 'issues/calendar', :controller => 'calendars', :action => 'show'
    issues_routes.connect 'issues/:action'
  end
end

After

# config/routes.rb
ActionController::Routing::Routes.draw do |map|
  map.bulk_edit_issue 'issues/bulk_edit', :controller => 'issues', :action => 'bulk_edit', :conditions => { :method => :get }
  map.bulk_update_issue 'issues/bulk_edit', :controller => 'issues', :action => 'bulk_update', :conditions => { :method => :post }
  map.quoted_issue '/issues/:id/quoted', :controller => 'journals', :action => 'new', :id => /\d+/, :conditions => { :method => :post }
  map.connect '/issues/:id/destroy', :controller => 'issues', :action => 'destroy', :conditions => { :method => :post } # legacy

  map.resource :gantt, :path_prefix => '/issues', :controller => 'gantts', :only => [:show, :update]
  map.resource :gantt, :path_prefix => '/projects/:project_id/issues', :controller => 'gantts', :only => [:show, :update]
  map.resource :calendar, :path_prefix => '/issues', :controller => 'calendars', :only => [:show, :update]
  map.resource :calendar, :path_prefix => '/projects/:project_id/issues', :controller => 'calendars', :only => [:show, :update]

  # Following two routes conflict with the resources because #index allows POST
  map.connect '/issues', :controller => 'issues', :action => 'index', :conditions => { :method => :post }
  map.connect '/issues/create', :controller => 'issues', :action => 'index', :conditions => { :method => :post }
  
  map.resources :issues, :member => { :edit => :post }, :collection => {}
  map.resources :issues, :path_prefix => '/projects/:project_id', :collection => { :create => :post }
end

Refactoring complex routes is always difficult, especially in Redmine where the default route is still used. In this case, to handle all of the routes I had to add a few shims:

  • Named routes for bulk_edit and bulk_update. These can probably be converted over to :collection actions on the Issues resource later.
  • Named route for quoting an issue (map.quoted_issue). I think this can be removed once Journals are converted to a sub resource of Issues.
  • A Legacy route for destroying an issue.
  • Two resources for the Gantt charts, a global one and a project specific one.
  • Two resources for the Calendars, a global one and a project specific one.
  • Two routes to override the Issue resource so posting to '/issues' will work for submitting an issue query. This required moving the create route to '/issues/create'.

You'll also notice that there are two resources for Issues, one globally and one for the project. Once Projects are converted to REST resources, I'll be able to nest Issues under them and use shallow routing to handle most of the paths.

I'm now finished refactoring IssuesController for now. My goal with refactoring Redmine's IssuesController was to convert it to a REST resource. Next it's time to start refactoring the ProjectsController, with the end goal of converting it to a REST resource also.

Reference commit

Tagged: ruby redmine refactoring

Now that I refactored Redmine and split the #bulk_edit method, I'm ready to start to refactor the #bulk_update method. Starting with the block of code that's setting attributes, I used extract method to pull it out into a utility method.

Before

class IssuesController < ApplicationController
  def bulk_update
    @issues.sort!

    attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
    attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
    attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
    
    unsaved_issue_ids = []
    @issues.each do |issue|
      issue.reload
      journal = issue.init_journal(User.current, params[:notes])
      issue.safe_attributes = attributes
      call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
      unless issue.save
        # Keep unsaved issue ids to display them in flash error
        unsaved_issue_ids << issue.id
      end
    end
    set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
    redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
  end

end

After

class IssuesController < ApplicationController
  def bulk_update
    @issues.sort!
    attributes = parse_params_for_bulk_issue_attributes(params)

    unsaved_issue_ids = []
    @issues.each do |issue|
      issue.reload
      journal = issue.init_journal(User.current, params[:notes])
      issue.safe_attributes = attributes
      call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
      unless issue.save
        # Keep unsaved issue ids to display them in flash error
        unsaved_issue_ids << issue.id
      end
    end
    set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
    redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
  end

  private

  def parse_params_for_bulk_issue_attributes(params)
    attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
    attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
    attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
    attributes
  end
end

By extracting this dense method into a utility method, the #bulk_update action is now a little easier to read.

There is still a pretty complex block of code in the @issues iterator that I will need to refactor soon. The big problem will be making sure that the call_hook method still works for plugins.

Reference commit

Tagged: ruby redmine refactoring extract-method

After thinking about how IssuesController#bulk_edit works, I decided to leave it in IssuesController. It doesn't match Rail's REST conventions but it does match the REST principles for resource representations. IssuesController returns representations of issue collections and issue objects. Since #bulk_edit works on an issue collection, keeping it in the IssuesController makes sense.

There still is refactoring that needs to happen to #bulk_edit though. The single method is responsible for displaying the bulk edit form and also for performing the bulk edit, which is one too many responsibilities.

Before

class IssuesController < ApplicationController
  before_filter :find_issues, :only => [:bulk_edit, :move, :perform_move, :destroy]

  def bulk_edit
    @issues.sort!
    if request.post?
      attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
      attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
      attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
      
      unsaved_issue_ids = []
      @issues.each do |issue|
        issue.reload
        journal = issue.init_journal(User.current, params[:notes])
        issue.safe_attributes = attributes
        call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
        unless issue.save
          # Keep unsaved issue ids to display them in flash error
          unsaved_issue_ids << issue.id
        end
      end
      set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
      redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
      return
    end
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end
end

After

class IssuesController < ApplicationController
  before_filter :find_issues, :only => [:bulk_edit, :bulk_update, :move, :perform_move, :destroy]

  verify :method => :post, :only => :bulk_update, :render => {:nothing => true, :status => :method_not_allowed }

  def bulk_edit
    @issues.sort!
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end

  def bulk_update
    @issues.sort!

    attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
    attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
    attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
    
    unsaved_issue_ids = []
    @issues.each do |issue|
      issue.reload
      journal = issue.init_journal(User.current, params[:notes])
      issue.safe_attributes = attributes
      call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
      unless issue.save
        # Keep unsaved issue ids to display them in flash error
        unsaved_issue_ids << issue.id
      end
    end
    set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
    redirect_back_or_default({:controller => 'issues', :action => 'index', :project_id => @project})
  end

end

By splitting #bulk_edit and #bulk_update with extract method, I now have greater control over how each method works. For example, using the routing and verify, only POST requests are sent to #bulk_update so the if request.post? guard isn't needed anymore. Eventually even that will be refactored once IssuesController becomes a full Rails resource.

Reference commit

Tagged: ruby redmine refactoring extract-method

I last talked about how wild estimates affects a team with unfactored code. Now I want to look at another impact on the team that is even more dangerous.

Lowered morale

In order to produce a great application, every team needs to work as a team. They should want to help each other and go out of the way to make it easier on another team member. When code isn't refactored it puts a burden on the other developers on the team.

I've been on a team who was hit really hard by this. The codebase was huge, with tons of legacy code and written in a programming language that is barely used anymore.

One of my first assignments was to add a standard header and footer to the reporting engine, about 70 reports in total. The code for the header and footer were simple; just the report name, page number, and total number of pages. But because the system was never refactored, it took me several days of work in order to finish the task. I can really get into my development and focus, but I have never been so bored "developing" in my entire life.

This feeling was common with the rest of the development team. There were many smart people there, the majority of them way smarter than me, but they were chronically depressed working on the code. Everyday someone would make a joke about how bad the code was or what stupid function they found the day before.

One solution the company had was to rewrite the application in a modern language with newer tools. Unfortunately, that would take a couple of years to complete and several of the developers would still have to maintain the legacy version until the rewrite was done. The rewrite was still started but it caused a major split in the development group:

  1. the team who was working on the new cutting edge application
  2. the team stuck working on the legacy version

WTFs/m

The big problem the original application had was, the massive code duplication caused everyone on the team to be bored and angry working on it. There were frequent outbursts of "Who wrote this $%#^@& code?!?!" and the code reviews where a constant stream of WTFs. This put the development team in conflict with everyone: the other developers, the testing staff, the support staff, and even the customers who were asking for a change. This caused every change request to be immediately rejected by the developers, even if it was in the best interest of the application.

A lot of these problems could have been fixed or improved by just taking a few extra minutes every day to refactor the code. But they never did and the entire team is still struggling.

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