written by Eric Davis on September 02, 2010
0 Comments
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
written by Eric Davis on September 01, 2010
0 Comments
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:
- To show the form that's used to upload a new file
- 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
written by Eric Davis on August 31, 2010
0 Comments
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
written by Eric Davis on August 30, 2010
0 Comments
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
written by Eric Davis on August 27, 2010
0 Comments
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
written by Eric Davis on August 27, 2010
0 Comments
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
- 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 26, 2010
0 Comments
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
written by Eric Davis on August 25, 2010
0 Comments
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
written by Eric Davis on August 24, 2010
0 Comments
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
written by Eric Davis on August 24, 2010
0 Comments
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:
- the team who was working on the new cutting edge application
- the team stuck working on the legacy version
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
- 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
